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.

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

message-editing: Update unread count when message is deleted in stream. #8415

Closed
wants to merge 1 commit into from

Conversation

shubham-padia
Copy link
Member

@shubham-padia shubham-padia commented Feb 16, 2018

fixes #8411 first issue
Testing instructions:
Send multiple messages to a stream, while the other user is offline. Note the unread count on the second user's sidebar. Start deleting those messages one by one. Note that the unread count is decreasing with the deletion.

@zulipbot
Copy link
Member

zulipbot commented Feb 16, 2018

Hello @zulip/ members, this pull request was labeled with the **** labels, so you may want to check it out!

@@ -356,6 +356,8 @@ exports.dispatch_normal_event = function dispatch_normal_event(event) {

case 'delete_message':
var msg_id = event.message_id;
var message = message_store.get(msg_id);
unread_ops.mark_message_as_read(message);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Does this do the right thing in the case that the message had already been read? (I can imagine trying to double-mark something as read throwing an exception).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, sorry should have mentioned in the PR itself. mark_messages_as_read passes the message list to unread.get_unread_messages, which returns all the unread messages out of the given list. So double marking something as read would not occur

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Cool. I''ll merge a quick PR adding a comment to that effect, just to make things more readable.

@timabbott
Copy link
Sponsor Member

I posted one comment; it also looks like the node tests are failing.

@timabbott
Copy link
Sponsor Member

@shubham-padia you can use tools/test-js-with-node locally to run the node tests quickly while debugging the issue. I suspect we just need to add a require at the top of the test file.

@shubham-padia
Copy link
Member Author

@timabbott The tests have been fixed

@timabbott
Copy link
Sponsor Member

Nice! Merged after tweaking the commit message to better match our style, thanks @shubham-padia!

You should read git log to get a feel for proper commit message style; e.g. lines should be wrapped, sentences (even "Fixes #8415") should be capitalized, etc.

I left #8411 open because I bet there's still an issue with it appearing in the recent_topics data structure even if you deleted the last message in the topic. I think the update_message code path for changing the topic of a message should have some things you can borrow for fixing that.

@timabbott timabbott closed this Feb 16, 2018
fixes zulip#8411
Unread counts in the left sidebar were not updated immediately on message delete.
@shubham-padia shubham-padia deleted the msg_del branch September 8, 2018 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve live-update when a message is deleted by an administrator
3 participants