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

Jump to bottom

bugfix: Fix wildcard_mentions not showing in mentions narrow. #1038

Closed
wants to merge 2 commits into from
Closed

bugfix: Fix wildcard_mentions not showing in mentions narrow. #1038

wants to merge 2 commits into from

Conversation

zee-bit
Copy link
Member

@zee-bit zee-bit commented May 25, 2021 •
edited
Loading

The server defines several wildcard_mentions like @all/@everyone/@stream, which are not marked with 'mentions' in the message flags, rather 'wildcard_mentions'. This bugfix PR now checks for this flag in the message response from the server and appropriately categorizes these messages as mentioned and also updates the count in the respective narrows.

Fixes #1037

@zulipbot zulipbot added the size: M [Automatic label added by zulipbot] label May 25, 2021
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from ba4d273 to d09d458 Compare May 25, 2021 21:26
@zee-bit zee-bit changed the title bugfix: helper: Fix wildcard_mentions not showing in mentions. bugfix: Fix wildcard_mentions not showing in mentions narrow. May 25, 2021
@neiljp
Copy link
Collaborator

neiljp commented May 26, 2021

@zee-bit This looks effective after a quick manual test 👍

As per messages on the stream, I think it'd be useful to ensure through a test that the event handling code handles the behavior correctly of adding/removing these message flags. This sort of updates already via scrolling but not automatically, and the event code looks like it won't behave properly in all cases, so this may be updated behavior belonging in a new commit. We likely want it to behave the same as for starred messages, ie. add but not remove, until leaving and returning to the narrow.

A related point to that above is that we could use a MessageFlags type to clarify what values to expect in messages, and a more limited related type ModifiableMessageFlags perhaps for those we could see in events?

Another related point - perhaps for the first commit? - is that we may want to explore expanding other tests to incorporate this message flag. That'll be both checking for tests with mention and expanding to include extra wildcard_mention cases, as well as considering cases where we don't check mention at all but just eg. starred/read, and may want to generalize?

neiljp
neiljp reviewed May 26, 2021
View reviewed changes
zulipterminal/helper.py Outdated Show resolved Hide resolved
neiljp
neiljp reviewed May 26, 2021
View reviewed changes
tests/helper/test_helper.py Outdated Show resolved Hide resolved
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from d09d458 to 7c85a09 Compare May 28, 2021 10:09
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels May 28, 2021
@zee-bit zee-bit added bug Something isn't working PR needs review PR requires feedback to proceed labels May 29, 2021
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from 7c85a09 to b37a885 Compare June 1, 2021 22:00
neiljp
neiljp reviewed Jun 3, 2021
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@zee-bit I started some feedback on this so wanted to get a little more out to you. I've not looked at the last commit.

Having flags internally as a set may make things simpler for us, though I've not examined how much of a refactor it would be.

zulipterminal/api_types.py Outdated Show resolved Hide resolved
zulipterminal/api_types.py Outdated Show resolved Hide resolved
tests/helper/test_helper.py Outdated Show resolved Hide resolved
zulipterminal/model.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/model/test_model.py Show resolved Hide resolved
tests/model/test_model.py Outdated Show resolved Hide resolved
@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 3, 2021
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from b37a885 to 2ca4b32 Compare June 3, 2021 22:19
neiljp
neiljp reviewed Jun 4, 2021
View reviewed changes
zulipterminal/api_types.py Outdated Show resolved Hide resolved
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from 2ca4b32 to 5b525a4 Compare June 4, 2021 13:23
@zee-bit zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 4, 2021
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from 5b525a4 to bc9e4e2 Compare June 4, 2021 13:34
neiljp
neiljp reviewed Jun 9, 2021
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@zee-bit Just a quick bit of feedback tonight while I have time - independently of the bugfix, I'm looking forward to the refactor of mentions fixture as well as the extra api typing 👍

tests/helper/test_helper.py Outdated Show resolved Hide resolved
zulipterminal/api_types.py Show resolved Hide resolved
@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 9, 2021
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from bc9e4e2 to 904c272 Compare June 10, 2021 09:10
@zee-bit zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 10, 2021
@neiljp
Copy link
Collaborator

neiljp commented Jun 19, 2021

@zee-bit The refactoring first is great, and I've cherry-picked that first commit 👍

I don't like that we introduce a broken state after the second commit - and the subsequent commit is a little unclear too in terms of whether it's fixing that broken state or something else. If you can introduce some wildcards without breaking things then you could, then update the params with more values next - but I suspect that's not that useful.

I'd suggest just adding some mixed data to the new fixture and fixing the code to make tests pass - in one commit. Then if there's a separate bugfix to make, add that - along with a new test case?

@neiljp
Copy link
Collaborator

neiljp commented Jun 19, 2021

(Just cherry-picked the first flag types commit too)

@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from 904c272 to 88bb614 Compare June 19, 2021 09:01
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from 88bb614 to 3be2349 Compare June 19, 2021 12:40
@neiljp
Copy link
Collaborator

neiljp commented Jun 19, 2021

@zee-bit Another cherry-pick after cleaning the commit text to simplify the bug text - it should be accurate, but it's easier if you can do this more yourself. I think you referenced various different bugs? I'd recommend checking it out and we can discuss.

I would have merged the rest but we shouldn't depend on just indexed messages for the count to update - if I edit a message which isn't indexed then the mentioned unreads don't change currently or with the remaining commit(s).

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 19, 2021
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch 2 times, most recently from 1da9a3d to dacdeda Compare June 21, 2021 17:05
@zee-bit zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 26, 2021
@zee-bit
Copy link
Member Author

zee-bit commented Jun 26, 2021

We may need to get the remaining commits (or, at least the second one) in before I can push a fix for the un-indexed message bug, cause the new field that I'm working on locally needs to be updated when flags are changed through update_message_event, which we don't do currently (and is added in the second commit of this PR).

zee-bit added 2 commits July 2, 2021 21:22
@zee-bit
bugfix: api_types/model: Update mentioned narrow on message edit event.
7714f44 
This commit updates the index and unread_count if the message was indexed
when an update message event with change in mention flag(s) is detected.
If the message was not in index, we just update the unread_count. In this
case, the count is updated by comparing with initial_data's "unread_msgs"
field which does not stay synced with the server, hence the logic is buggy.

Changes in mentioned narrow after this event are not rendered at the same
time but it updates the model so that a return to the mentioned messages
narrow will do so.

The tests are also tried to be modified as less as possible, and the
present fixtures were mostly used to test for this new condition,
thus preventing the use of too many fixtures.

Tests amended.
@zee-bit
tests: model: Extend other tests to incorporate both mentioned flags.
f75075a 
This commit expands some of the fixtures of present tests with the
mentioned/wildcard_mentioned flags to make the tests more robust
and generalized.
@zee-bit zee-bit force-pushed the bugfix-all-mentions branch from dacdeda to f75075a Compare July 2, 2021 16:20
@zee-bit
Copy link
Member Author

zee-bit commented Jul 5, 2021

The rest of the PR is protracted to #1068, along with the un-indexed message bugfix.

@zee-bit zee-bit closed this Jul 5, 2021
@neiljp neiljp added missing feature: user A missing feature for all users, present in another Zulip client and removed PR needs review PR requires feedback to proceed labels Sep 8, 2021
@neiljp neiljp added this to the Next Release milestone Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

Assignees
No one assigned
Labels
bug Something isn't working missing feature: user A missing feature for all users, present in another Zulip client size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

All/everyone/stream mentions don't show up in mentions narrow
3 participants
@zee-bit @neiljp @zulipbot