Fix unread comments status

[fosterfarrell9]

This fixes #573. I think that the behaviour in all the edge cases considered in #515 should remain unchanged.

[Splines]

Maybe I don't understand what is really going on since I don't see why we need the unread_media_comments method in the first place and what it's relation is to the unseen_comments of the comments_controller. See my comments below.

Edit: Start with the comment containing this symbol (⏺) as some things have clarified in the meantime .

[Splines]

Extract expression to variable, then use it to do the comparison.

[Splines]

Also note that a very similar logic is used in the update method of the ReadersController.

[Splines]

In _navbar.html.erb we use current_user.unread_comments to assign the "new-comment" CSS class accordingly. I don't understand why we need a separate unread_media_comments and what its purpose is.

Furthermore, I'm not too sure if the user model is the place where this methods belongs to as it is related to the comments and not the user itself. But this is a problem generally with the user.rb file: it's so long because it contains lots of stuff that is semantically related to a user but actually deals with something else at its heart, e.g. media comments management. Of course, MaMpf is a user-centric platform, so everything is somehow related to a user. We should still split the functionality. As restructuring this would probably entail bigger refactoring efforts, I'm fine with leaving this method here for now.

[fosterfarrell9]

I agree with the gneral sentiment towards user.rb: it needs a lot of refactorization. But the unread_media_comments method I would at least for the moment leave here. Comments management is currently only done in the Commontator gem models, I would not want to place a method that is central for us in a gem's model.

[Splines]

I don't see how this achieves the goal of this PR. Right now, update_unread_status is only called from the create method of the comments_controller, so why would we want to set the unread_comments to false in the create method? Shouldn't we set the unread_comments to false whenever a user clicks on the "X" button next to the comment to mark it as read? How is this reflected in this PR?

[Splines]

⏺ Update

Maybe I get it now. unseen_comments is actually rather a unseen_new_comments, right? That is, we check whether any other user posted a comment during the time a user drafted and saved a comment. It's not actually if there are any unseen comments, just new ones in that specific timespan.

That's why we need another method to find out if the user generally has any unseen comments and this is the unread_media_comments method which is also used to identify all the unread comments of a user for the @media_array shown in the frontend (on the comments overview page after clicking on the yellow bubble).

We don't want to execute this rather costly operation of searching all unread comments on every pageload to determine which color the bubble should have, right? That's why we have the unread_comments boolean for the user but we didn't set it to false correctly.

---

If what I said is correct so far I'm still not sure why update_unread_status is only called in the create method and why the logic for setting unread_comments to false is in there. Shouldn't this logic be executed only when a user clicks on the "X" next to a comment (on the comments overview page) to dismiss it? Then, we could check whether any unread comment is left and if not, set the flag unread_comments to false.

[fosterfarrell9]

We don't want to execute this rather costly operation of searching all unread comments on every pageload to determine which color the bubble should have, right? That's why we have the unread_comments boolean for the user but we didn't set it to false correctly.

That's it exactly.

If what I said is correct so far I'm still not sure why update_unread_status is only called in the create method and why the logic for setting unread_comments to false is in there. Shouldn't this logic be executed only when a user clicks on the "X" next to a comment (on the comments overview page) to dismiss it? Then, we could check whether any unread comment is left and if not, set the flag unread_comments to false.

That is precisely the logic we had before we introduced #515 (and which we did not change in #515), see the raeders_controller which does exactly what you describe and resets the unread_comments flag to false. However, with the introduction of #515, the case described in #573 (which we missed in #515) occurs: In this situation the coresponding comment will not be listed in the comments index page, so there will be no "X", and no action related to the readers_controller will ever be executed and the unread_comments flag is not set to false. The only other place we can do it is in the create action for the comment.

[Splines]

According to the flow inside the update_unread_status method of the comments_controller, we will never encounter the case where both variables (@update_icon and @no_unread_comments are true), so we won't have the case that we add a CSS class just to then remove it immediately. This would result in a short visual flickering which is unwanted. So, this is not a problem.

But still, maybe we can simplify the logic in the controller such that the frontend only has to deal with one variable determining whether to add the CSS class or do nothing? Or do we really have to remove the CSS class because it is set in the _navbar.html.erb and we need to get rid of it? If so, why not make the changes such that the navbar doesn't add the class in the first place?

[fosterfarrell9]

The navbar is loaded way before. In the situation described in #573, the teacher finds a yellow new_comments icon in the navbar (correctly so), posts their reply, and then the icon has to turn white. During that process, the navbar does not get reloaded. The alternative to what is done in the PR would be to reload the navbar. But the view we have here is a view of the commontator gem which we modify (currently only by the two simple simple statements). Reloading the navbar would be a bigger operation which I suggest we keep out of this "external" view.

[Splines]

I missed this in the last PR. Why do we need a global variable here (@reader)? Instead, pass the reader to unseen_comments as parameter. This way it's even easier to understand what is going on. A smiliar scenario can be found in the update method in readers_controller.

[fosterfarrell9]

I want to point out that the behavior introduced in #515 is pretty inconsistent. Consider the following to scenarios:

• First scenario: Consider a medium M without comments. We have users A and B. By assumption, M has an empty commontator_thread T and there does not exist a reader where the thread is equal to T. Now suppose A makes a comment at time t1. B sees the comment and posts a reply at time t2. What happens in the comments_controller for B's reply? Since there is no reader with thread T, in

@reader = Reader.find_or_create_by(user: current_user, thread: @commontator_thread)

a new reader R is created for user B and thread T at around time t2, lets say for simplictiy at exactly time t2. The updated_at column of R is then filled with t2. The unseen_comments method of #515

def unseen_comments?
   @commontator_thread.comments.any? do |c|
       c.creator != current_user && c.created_at > @reader.updated_at
   end
end

will return false because the only comment in T not created by B was created at time t1<t2.

• Second scenario: Consider a medium M with existing comments. We have users A and B. B has seen the existing comments at some time t0. Hence, M has a nonempty commontator_thread T and there exists a reader R for thread T and user B, whose updated_column is given by t0. Now suppose A makes a comment at time t1. B sees the comment and posts a reply at time t2. What happens in the comments_controller for B's reply? Since there is already reader R for thread T and user B,

@reader = Reader.find_or_create_by(user: current_user, thread: @commontator_thread)

reader ist set to R. Now, the unseen_comments? method will return true because t1>t0.

TLDR: In both scenarios user B has no unread comments for the given medium before, but depending on whether there
are older comments, unseen_comments returns either true or false.

#515 has introduced inconsistencies such as the one above, an issue like #573, the business logic has become more complex and there now some edge cases discussed in #515. I would at least put it up for discussion that we revert #515. Business logic was very simple before and the Reader model was tailored to just adress this simple setting. If we decide not tp revert we should make the code a lot more robust here.

[Splines]

Let COP $\coloneqq$ "comments overview page" (/main/comments/).

@fosterfarrell9 thanks a lot for your detailed description of the scenarios, this actually helped while debugging this issue. Note I've also used pgadmin and it worked like a charm and allowed me to easily see what happens exactly in the Readers table.
After digging around for quite some time, here is my analysis. The respective fix is provided in #576. But I'm sure there will be still some things I've not taken into account 🙈

---

The root cause for our problem is that we mix granularities. The Reader instance and all the checks figuring out whether comments have been read operate on the granularity Thread (which is attached to one medium) and not on the granularity Comment of a thread. Yet still, starting of #515, we create a Reader entry also when we posted any comment. The updated_at column is updated accordingly and this messes everything up later when we try to check on a higher granularity whether there are any unread comments with a simple time < comparison.

mampf/app/controllers/commontator/comments_controller.rb

Lines 205 to 209 in 205a4e7

	# make sure that the thread associated to this comment is marked as read
	# by the comment creator (unless some other user posted a comment in it
	# that has not yet been read)
	@reader = Reader.find_or_create_by(user: current_user,
	thread: @commontator_thread)

The create logic was even introduced in the very first commit of #515 and then later only refined to read find_or_create_by to fix a bug, but still: it has "create" in it and that seems to be the problem.

---

Maybe a fix: The comment you originally wrote above this line states what we want to achieve originally with the update_unread_status method:

make sure that the thread associated to this comment is marked as read by the comment creator (unless some other user posted a comment in it that has not yet been read)

You achieved this by creating a new Reader entry or updated the updated_at timestamp accordingly such that it isn't displayed anymore in the COP (we don't want comments posted by yourself to come up there as unread comment). Instead: why not do exactly this in the controller that is responsible for the comments. Unintuitively, it is called main_controller.rb and here is the crucial part:

mampf/app/controllers/main_controller.rb

Lines 28 to 37 in 9583efb

	def comments
	@media_comments = current_user.media_latest_comments
	@media_comments.select! do |m|
	(Reader.find_by(user: current_user, thread: m[:thread])
	&.updated_at || 1000.years.ago) < m[:latest_comment].created_at &&
	m[:medium].visible_for_user?(current_user)
	end
	@media_array = Kaminari.paginate_array(@media_comments)
	.page(params[:page]).per(10)
	end

Here

mampf/app/controllers/main_controller.rb

Lines 31 to 32 in 9583efb

	(Reader.find_by(user: current_user, thread: m[:thread])
	&.updated_at || 1000.years.ago) < m[:latest_comment].created_at &&

we use a timestamp and compare it to the latest comment created for that media. The timestamp used is either the updated_at entry of the Reader or 1000.years.ago (which is $-\infty$ for practical reasons). If no reader exists, we cannot just randomly take any other value. That's why we instead have to modify what media we are considering here in the first place.

We get the media via this method:

mampf/app/controllers/main_controller.rb

Line 29 in 9583efb

@media_comments = current_user.media_latest_comments

So, let's modify it in the user model:

mampf/app/models/user.rb

Lines 552 to 567 in 9583efb

	def media_latest_comments
	relevant_media = []
	subscribed_commentable_media_with_comments.each do |m|
	latest_comment = m.commontator_thread.comments
	.reject { |c| c.creator == self }
	.max_by(&:created_at)
	next unless latest_comment
	relevant_media << { medium: m,
	thread: m.commontator_thread,
	latest_comment: latest_comment }
	end
	relevant_media.sort_by { |x| x[:latest_comment].created_at }.reverse
	end

For any media, we will now disregard any comments we posted by ourselves. Thus, the latest_comment variable can never refer to any comment/reply we posted. And this is what we want: now the comment method in the MainController is not even considering these kind of comments and therefore they cannot appear in the COP.

Finally, this allows us to not bother creating a Reader entry whenever we post something. Instead we can simply use a find_by:

mampf/app/controllers/commontator/comments_controller.rb

Lines 212 to 213 in 9583efb

	@reader = Reader.find_by(user: current_user, thread: @commontator_thread)
	return unless @reader

---

From my preliminary tests, this seems to work even for your scenarios. I'm not sure if we should get rid of the reader stuff alltogether here:

mampf/app/controllers/commontator/comments_controller.rb

Lines 209 to 220 in 9583efb

	# make sure that the thread associated to this comment is marked as read
	# by the comment creator (unless some other user posted a comment in it
	# that has not yet been read)
	@reader = Reader.find_by(user: current_user, thread: @commontator_thread)
	return unless @reader
	if unseen_comments?
	@update_icon = true
	return
	end
	@reader.touch
	end

• At least, the @reader.touch shouldn't be there anymore, right? The reader updated_at field should be only changed whenever I - as a user - click on the dismiss button or the "Clear all" button in the COP. And this is already done in the ReadersController alongside setting the unread_comments to false accordingly. In which scenario, do we have to update the unread comments icon directly after creating a new comment? Is there any?
• The reason for the lines after the find_by is this comment of yours. But I have to dig deeper there again.
• Some logic is copy and pasted!, e.g. see the call to the Reader in ReaderController.update and MainController.comments.
• Check that the bug I encountered in #515 is not present here.
• Currently in Fix unread comments status (new trial) #576, when we post a reply, the comment is still marked as unread in the COP. However, this behavior was the same beforehand (based on a short test, but this is true, right?) -> I think that one can assume someone has seen the comment when that person even replies to it, so we could mark the respective comment we are replying to as "read" using the same logic executed when the person clicks on the "dismiss" button in the COP to mark a comment as read. But this behavioral change is something for another PR I guess.

Please don't comment over at #576 and instead here. If #576 actually works out, we can still incorporate it into this PR such that we still have all the discussion and comments in one place.

[fosterfarrell9]

At least, the @reader.touch shouldn't be there anymore, right? The reader updated_at field should be only changed whenever I - as a user - click on the dismiss button or the "Clear all" button in the COP. And this is already done in the ReadersController alongside setting the unread_comments to false accordingly. In which scenario, do we have to update the unread comments icon directly after creating a new comment? Is there any?

I agree that (probably) the @reader.touch can be omitted now. As for the update of the commentsIcon: The code change in comments_controller.rb to

   # make sure that the thread associated to this comment is marked as read 
   # by the comment creator (unless some other user posted a comment in it 
   # that has not yet been read) 
   @reader = Reader.find_by(user: current_user, thread: @commontator_thread) 
   return unless @reader 
  
   if unseen_comments? 
     @update_icon = true 
     return 
   end 
   @reader.touch 
 end 

leads to the following minor inconsistency:
Assume you have a medium without any coment yet, your commentsIcon is white and you are writing a comment. During the time you are writing, some other user posts a comment on the same medium. After you post your comment, your commentsIcon remains white. After the next page reload, it turns yellow. Now repeat the same procedure with a medium that already has a read comment (e.g. the medium just used). Then the commentsIcon will turn yellow after you post your comment. The reason is that in the first case, no reader is found and the method returns immediately, while in the second case the update_icon flag is set to true.

I think we should decide on what the wanted behaviour should be in this situation.

Currently in #576, when we post a reply, the comment is still marked as unread in the COP. However, this behavior was the same beforehand (based on a short test, but this is true, right?)

This is part of the issue with #576. It is as you describe with a medium that already has comments, but with a medium without comments the scenario you describe (assume you have no toher unread comments) results in a yellow commentsIcon, but an empty list in the COP page.

Another minor inconsistency (that I could live with) that results from the rewriting of the media_latest_comments method is the following: In the list of comments on the COP, the "Latest post" column now ignores posts by the user themself so it should actually be "Latest post by other users".

[Splines]

• With regards to the early return. You're right, we should not have that early return and still check whether the icon should be updated in that method. I can fix that.
• For the line starting with "It is as you describe with..." I need a bit more clarification on what is not working right now. I cannot reproduce your original scenario anymore with my changes on Fix unread comments status (new trial) #576.
• With regards to the "latest post" column: A simple fix would be to create a separate method and not filter the media array there such that this column is still accurate and might also show your own posts. This solution could be constructed such that we don't have to query twice.

[fosterfarrell9]

With regards to the early return. You're right, we should not have that early return and still check whether the icon should be updated in that method. I can fix that.

Since @reader.touch seems no longer necessary, the lines after the comment should probably simplify to

return unless unseen_comments?

@update_icon = true 

or something like that.

For the line starting with "It is as you describe with..." I need a bit more clarification on what is not working right now. I cannot reproduce #573 (comment) anymore with my changes on #576.

We probably misunderstood each other. I understood your question

However, this behavior was the same beforehand (based on a short test, but this is true, right?)

as a question on the behaviour in #515. So, in my answer

This is part of the issue with #576. It is as you describe with a medium that already has comments, but with a medium without comments the scenario you describe (assume you have no toher unread comments) results in a yellow commentsIcon, but an empty list in the COP page.

I refer to #515, but mistakenly wrote #576.
Does that clear it up?

With regards to the "latest post" column: A simple fix would be to create a separate method and not filter the media array there such that this column is still accurate and might also show your own posts. This solution could be constructed such that we don't have to query twice.

That is fine with me.

[Splines]

However, this behavior was the same beforehand (based on a short test, but this is true, right?)

This was indeed a question referring to the issue #515. What I'm refering to with "this behavior" is the one denoted the line before (though I agree my sentence was really not the best and easy to misunderstand)

when we post a reply, the comment is still marked as unread in the COP.

As far as I know, this was already the case before #515 and might be a behavior we don't want. It's different from the issue we are dealing with here. It's just something that I wanted to bring up to the table: since I would expect that whenever I post a reply to a comment that I clicked on in the COP, then this comment should be marked as "read" for me and not appear in the COP anymore. However, this requires a finer granularity; namely, this requires to store for each comment -- and not just whole threads -- whether this comment was seen by a user. As this is probably something for another PR, I asked if that behavior was the same even before #515, and I'm sure it was (based on a short test by checking out a commit before the one in #515). Just wanted to make sure that I didn't accidentally break something that was working beforehand.

---

With regards to the issue we're actually dealing with here: did I understand it correctly that #515 is not reproducible anymore in #576? If so, I could go ahead and refine #576. As to how to proceed: if #576 is the way to go, I've changed my mind and think it's better to continue working over there as reverting the commits here might be too tedious. And we also have all the open conversations here that clutter the space and might make a review for #576 (if incorporated into this PR) a bit of an annoying process.

[fosterfarrell9]

With regards to the issue we're actually dealing with here: did I understand it correctly that #515 is not reproducible anymore in #576?

Yes.

If so, I could go ahead and refine #576. As to how to proceed: if #576 is the way to go

I think #576 is the way to go.

since I would expect that whenever I post a reply to a comment that I clicked on in the COP, then this comment should be marked as "read" for me and not appear in the COP anymore. However, this requires a finer granularity; namely, this requires to store for each comment -- and not just whole threads -- whether this comment was seen by a user. As this is probably something for another PR, I asked if that behavior was the same even before #515, and I'm sure it was (based on a short test by checking out a commit before the one in #515). Just wanted to make sure that I didn't accidentally break something that was working beforehand.

I agree that this would be something for a different PR. Personally, I am honestly rather indifferent towards the current behaviour (recall that I also did not mind the behaviour before #515), so if introducing this finer granularity requires some work, we should (at a later point) at least discuss if the additional work would be worth it.

[Splines]

Closed in favor of #585, will delete this branch once #585 is merged into dev.