Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unread comments status #574

Closed
wants to merge 4 commits into from
Closed

Fix unread comments status #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3afe796
fix incorrect unread_comments flag
fosterfarrell9 Dec 19, 2023
5ffa192
use select instead of select!
fosterfarrell9 Dec 19, 2023
975b5cc
run ESLinit
fosterfarrell9 Dec 19, 2023
72cc2db
use local variable and rename method
fosterfarrell9 Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions app/controllers/commontator/comments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,18 +209,22 @@ def update_unread_status
# 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)
if unseen_comments?
reader = Reader.find_or_create_by(user: current_user,
thread: @commontator_thread)
if unseen_new_comments?(reader)
@update_icon = true
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
@reader.touch
reader.touch
return if current_user.unread_media_comments.any?

current_user.update(unread_comments: false)
@no_unread_comments = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@Splines Splines Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⏺ 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

def unseen_comments?
def unseen_new_comments?(reader)
@commontator_thread.comments.any? do |c|
c.creator != current_user && c.created_at > @reader.updated_at
c.creator != current_user && c.created_at > reader.updated_at
end
end
end
Expand Down
7 changes: 1 addition & 6 deletions app/controllers/main_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,7 @@ def sponsors
end

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_comments = current_user.unread_media_comments
@media_array = Kaminari.paginate_array(@media_comments)
.page(params[:page]).per(10)
end
Expand Down
8 changes: 8 additions & 0 deletions app/models/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,14 @@ def last_sign_in_ip=(_ip)
def current_sign_in_ip=(_ip)
end

def unread_media_comments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@fosterfarrell9 fosterfarrell9 Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

media_latest_comments.select do |mc|
(Reader.find_by(user: self, thread: mc[:thread])
&.updated_at || 1000.years.ago) < mc[:latest_comment].created_at &&
Comment on lines +779 to +780
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mc[:medium].visible_for_user?(self)
end
end

private

def set_defaults
Expand Down
16 changes: 10 additions & 6 deletions app/views/commontator/comments/create.js.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,28 @@
%>

<% if @commontator_new_comment.nil? %>
$("#<%= id %>").hide();
$("#<%= id %>").hide();

$("#<%= id %>-link").fadeIn();
$("#<%= id %>-link").fadeIn();
<% else %>
$("#<%= id %>").html("<%= escape_javascript(
$("#<%= id %>").html("<%= escape_javascript(
render partial: 'form', locals: {
comment: @commontator_new_comment, thread: @commontator_thread
}
) %>");
<% end %>

<% if @update_icon %>
$('#commentsIcon').addClass('new-comment');
$("#commentsIcon").addClass("new-comment");
<% end %>

<% if @no_unread_comments %>
$("#commentsIcon").removeClass("new-comment");
<% end %>
Comment on lines 32 to 38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@fosterfarrell9 fosterfarrell9 Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


var commontatorComment = $("#commontator-comment-<%= @comment.id %>").hide().fadeIn();
$('html, body').animate(
{ scrollTop: commontatorComment.offset().top - window.innerHeight/2 }, 'fast'
$("html, body").animate(
{ scrollTop: commontatorComment.offset().top - window.innerHeight / 2 }, "fast",
);

<%= javascript_proc %>