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

Update index's topic_msg_ids accurately with topic updates. #1130

Merged
merged 4 commits into from
Mar 9, 2022

Conversation

prah23
Copy link
Member

@prah23 prah23 commented Aug 23, 2021

What does this PR do?
Prior to this PR, if a message's topic was changed, the
corresponding message_id was not removed from the old topic's set
and added to the new one under the "topic_msg_ids" key of the model's
index. This PR updates the relevant sets to ensure accuracy.

Tested?

  • Manually
  • Existing tests (adapted, if necessary)
  • Passed linting & tests (each commit)

Commit flow

  • api_types: Include the orig_subject field in UpdateMessageEvent.
    This commit introduces another field to the UpdateMessageEvent class
    in api_types.py to accommodate this field in the event handler
    method for update message events.

  • bugfix: model: Remove updated message_ids from their old topic's set.
    Prior to this commit, if a message's topic was changed, the
    corresponding message_id was not removed from the old topic's set
    under the "topic_msg_ids" key of the model's index. This commit removes
    the updated message_id from that set, ensuring the accuracy of the
    set.
    Tests updated.

@zulipbot zulipbot added the size: M [Automatic label added by zulipbot] label Aug 23, 2021
@neiljp
Copy link
Collaborator

neiljp commented Jan 30, 2022

@prah23 This change looks reasonable, though I'll read the tests again before merging, but as per #zulip-terminal>Topic move bug? #T1130 does this fix a particular situation that can also be manually detected/tested?

@neiljp neiljp added the bug Something isn't working label Jan 30, 2022
@prah23 prah23 force-pushed the topic_msg_ids_bugfix branch from 7df3e14 to 827a429 Compare February 22, 2022 21:51
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Feb 22, 2022
@prah23 prah23 force-pushed the topic_msg_ids_bugfix branch from 827a429 to d3e242b Compare February 22, 2022 21:51
prah23 and others added 4 commits March 7, 2022 19:39
This commit includes more fields under the UpdateMessageEvent class
to ensure type-consistent handling for said fields in the update message
events conveying topic updates.
Prior to this commit, if a message's topic was changed, the
corresponding `message_id` was not removed from the old topic's set
under the "topic_msg_ids" key of the model's index. This commit removes
the updated `message_id` from that set, ensuring the accuracy of the
set.

Tests updated.
Prior to this commit, if a message's topic was changed, the
corresponding `message_id` was not added to the new topic's set
under the "topic_msg_ids" key of the model's index. This commit
adds the updated `message_id` to that set, ensuring the
accuracy of the new topic's set.

Tests updated.
@neiljp neiljp force-pushed the topic_msg_ids_bugfix branch from d3e242b to 6749809 Compare March 9, 2022 03:31
@neiljp neiljp merged commit 6749809 into zulip:main Mar 9, 2022
@neiljp
Copy link
Collaborator

neiljp commented Mar 9, 2022

@prah23 Thanks for the update! 🎉 I tacked a refactor on the end and added a comment to a test, and used a more detailed commit from another PR for the first commit since we can use it later and it was otherwise very tiny :)

@neiljp neiljp added this to the Next Release milestone Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants