bugfix: core: Fix cases where message was not focused on narrowing with anchor. #967

zee-bit · 2021-03-29T07:29:10Z

This bugfix fixes selecting messages if a contextual msg-id (anchor) is provided to the _narrow_to() function for some edge cases. Earlier, if the user was within the same narrow then narrowing to a quoted message from MsgLinkButton did not select that message. Also, sometimes pressing a on a particular message did not focus on it when narrowed to the all_messages narrow.

Tests added.
Fixes #954.

neiljp

@zee-bit Thanks for fixing the issue - all our stream narrowing from links should work after this - though I'm looking forward to seeing tests :)

We don't need to refer to the issue in the comment, and I'd only include a comment if you can't express the logic clearly. Given None has particular meaning here, I'd suggest using it explicitly.

zee-bit · 2021-03-31T19:10:31Z

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

neiljp

@zee-bit Thanks for working on the tests 👍

As per the comment, let's keep the bugfix commit itself as small as possible for clarity. You've extended the test and also altered some other test lines in the same commit, without touching on why. Feel free to add a refactor commit before or after with a rationale for that and we can explore those or improve these tests - these tests are by no means fixed or optimal I'm sure.

Regarding extending the test state space, we definitely could benefit from this in the longer term. There is likely a case where we don't jump to the message remaining due to having pre-existing messages but not the one in question - but this is a more general bug! (can you see it?)

neiljp · 2021-04-19T01:51:49Z

tests/core/test_core.py

-                           ['topic', msg_box.topic_name]]
-        controller.model.narrow = []
+    @pytest.mark.parametrize('initial_narrow, final_narrow, anchor', [
+        ([], [['stream', 'BOO'], ['topic', 'RDS']], None),


Do we lose anything by having the final narrow be the same each time, given we're testing narrowing to a topic? That would make this a lot less verbose, so make it clearer to understand all the cases.

Do we gain from exploring the initial_narrow state space for this change too, except within a topic?
It's clearer if with a bugfix you add/modify the tests minimally such that it breaks, then fix it.

To clarify, my intention behind keeping the final narrow same was that, the index_topic fixture from conftest contains just one topic and one message, so having multiple final narrows would need some serious refactoring.

No, we don't gain anything from exploring initial_narrow state space, it was just to assure that narrowing works for all possible combinations of initial narrow.

I'll have to explore more on how I can improve the tests with minimal change. Also, breaking the tests seems very unlikely since the index contains just one message and it would focus on that even if we provide an anchor or don't.

zee-bit · 2021-04-30T14:51:16Z

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

zee-bit · 2021-05-21T20:33:40Z

There's a prep PR for this that might simplify the multiple-message index generation in conftest here : #1022.

neiljp

@zee-bit This looks close to merging and will fix a pretty important bug 👍

This may also fix #934 ?

tests/core/test_core.py

neiljp · 2021-06-24T01:19:14Z

tests/core/test_core.py

+        if len(initial_narrow) == 2 and initial_narrow[0][0] == "stream":
+            controller.model.stream_id = 205


It would be nice to simplify this and avoid the conditional, but I'm not sure if we can?

We can perhaps have a initial_stream_id in parameterize?
That would be None when narrow is []?

tests/core/test_core.py

zulipterminal/core.py

This commit adds an extra stream message fixture to a new index fixture so that it could be utilized in tests that demands having at least two messages in a particular topic.

This commit modifies the test_narrow_to_topic() function to leverage the index_multiple_topic_msg fixture and test narrowing to a topic that contains multiple messages. We add testing for cases when anchor is provided to the _narrow_to function to enable us to broaden our scope of testing and open our eyes to bugs that may have been unnoticed with the previous test. We also add asserts to check if the final focus after narrowing is on the message we were expecting it to be in to ensure our code is robust. Tests amended.

This is bugfix that allow focusing on messages from the same narrow if a contextual message-id (anchor) is provided to the _narrow_to function. Earlier, if the user was within the same topic narrow then narrowing to a quoted message from MsgLinkButton did not select the message, since the _narrow_to() function didn't took anchor into account in this case. Tests amended. Fixes zulip#954.

This bugfix allows messages to be focused when narrowing to the "all_messages" narrow from other narrows. The primary bug was if the message from which we press 'a' was not present in model.index['all_msg_ids'], then we were not fetching that from the server. The reason we were not fetching those messages was we only checked if the final narrow is not empty, which in this case will never be, and not if it contains the message we are focusing to. This made the focus to move elsewhere in the "all_messages" narrow. Related discussion: #zulip-terminal > Contextual narrowing bug. Tests added.

neiljp · 2021-06-25T02:37:20Z

@zee-bit I adjusted some commit text, added test cases for the the anchor being another value in the narrow, and moved the expected value into the parametrize block in the last commit (like in the earlier one) - merging now 🎉

zulipbot added size: XS [Automatic label added by zulipbot] bug Something isn't working labels Mar 29, 2021

zee-bit mentioned this pull request Mar 29, 2021

Narrowing with a contextual message id should select that message #954

Closed

neiljp reviewed Mar 30, 2021

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Mar 30, 2021

zee-bit force-pushed the narrow-to-contextual-msg-id branch from 314cf7a to 3bd2378 Compare March 31, 2021 19:08

zulipbot added size: L [Automatic label added by zulipbot] and removed size: XS [Automatic label added by zulipbot] labels Mar 31, 2021

zulipbot 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 Mar 31, 2021

zee-bit force-pushed the narrow-to-contextual-msg-id branch from 3bd2378 to f41aa9f Compare March 31, 2021 19:44

zee-bit force-pushed the narrow-to-contextual-msg-id branch from f41aa9f to ab4ee77 Compare April 17, 2021 07:35

zee-bit requested a review from neiljp April 17, 2021 09:31

neiljp reviewed Apr 19, 2021

View reviewed changes

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 Apr 19, 2021

zee-bit force-pushed the narrow-to-contextual-msg-id branch from ab4ee77 to 858a231 Compare April 29, 2021 16:18

zulipbot 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 Apr 30, 2021

zee-bit force-pushed the narrow-to-contextual-msg-id branch from 858a231 to 68fe859 Compare May 2, 2021 12:31

zee-bit changed the title ~~bugfix: core: Fix message not selected within same narrow if anchored.~~ bugfix: core: Fix cases where message was not selected if anchor was provided. May 2, 2021

zee-bit changed the title ~~bugfix: core: Fix cases where message was not selected if anchor was provided.~~ bugfix: core: Fix cases where message was not focused on narrowing with anchor. May 2, 2021

zee-bit force-pushed the narrow-to-contextual-msg-id branch from 68fe859 to ad7f863 Compare May 2, 2021 19:59

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels May 2, 2021

zee-bit mentioned this pull request May 4, 2021

refactor: Simplify msg template generation in conftest using factory fixtures. #1022

Merged

zee-bit force-pushed the narrow-to-contextual-msg-id branch from ad7f863 to 7687d1f Compare May 6, 2021 15:35

zee-bit force-pushed the narrow-to-contextual-msg-id branch from 7687d1f to 9f587b3 Compare May 21, 2021 20:28

zee-bit force-pushed the narrow-to-contextual-msg-id branch from 9f587b3 to 18cd7fb Compare June 2, 2021 21:33

zee-bit force-pushed the narrow-to-contextual-msg-id branch from 18cd7fb to 0579843 Compare June 23, 2021 22:13

zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Jun 23, 2021

neiljp reviewed Jun 24, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed labels Jun 24, 2021

zee-bit force-pushed the narrow-to-contextual-msg-id branch 3 times, most recently from 9b237b3 to c37cb26 Compare June 24, 2021 23:35

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 24, 2021

zee-bit added 4 commits June 24, 2021 18:55

conftest: Add fixture to build index with multiple topic message.

4b4a731

This commit adds an extra stream message fixture to a new index fixture so that it could be utilized in tests that demands having at least two messages in a particular topic.

neiljp force-pushed the narrow-to-contextual-msg-id branch from c37cb26 to 39cafda Compare June 25, 2021 02:26

neiljp removed the PR needs review PR requires feedback to proceed label Jun 25, 2021

neiljp added this to the Next Release milestone Jun 25, 2021

neiljp merged commit f925c82 into zulip:main Jun 25, 2021

neiljp added the missing feature: user A missing feature for all users, present in another Zulip client label Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: core: Fix cases where message was not focused on narrowing with anchor. #967

bugfix: core: Fix cases where message was not focused on narrowing with anchor. #967

zee-bit commented Mar 29, 2021 •

edited

Loading

neiljp left a comment

zee-bit commented Mar 31, 2021

neiljp left a comment

neiljp Apr 19, 2021

zee-bit Apr 20, 2021

zee-bit commented Apr 30, 2021

zee-bit commented May 21, 2021

neiljp left a comment

neiljp Jun 24, 2021

zee-bit Jun 24, 2021

neiljp commented Jun 25, 2021

		if len(initial_narrow) == 2 and initial_narrow[0][0] == "stream":
		controller.model.stream_id = 205

bugfix: core: Fix cases where message was not focused on narrowing with anchor. #967

bugfix: core: Fix cases where message was not focused on narrowing with anchor. #967

Conversation

zee-bit commented Mar 29, 2021 • edited Loading

neiljp left a comment

Choose a reason for hiding this comment

zee-bit commented Mar 31, 2021

neiljp left a comment

Choose a reason for hiding this comment

neiljp Apr 19, 2021

Choose a reason for hiding this comment

zee-bit Apr 20, 2021

Choose a reason for hiding this comment

zee-bit commented Apr 30, 2021

zee-bit commented May 21, 2021

neiljp left a comment

Choose a reason for hiding this comment

neiljp Jun 24, 2021

Choose a reason for hiding this comment

zee-bit Jun 24, 2021

Choose a reason for hiding this comment

neiljp commented Jun 25, 2021

zee-bit commented Mar 29, 2021 •

edited

Loading