narrow: Mark messages as read when applying -is:dm filter. #25351
Conversation
|
Hello @zulip/server-search members, this pull request was labeled with the "area: search" label, so you may want to check it out! |
|
@syed-rafat thanks for working on this! Can you extend the tests to get back to 100% test coverage on |
|
Sure! I am on it.
Duly noted, I will extend the test to get 100% coverage. |
syed-rafat
force-pushed
the
mark-read-for-private-narrow
branch
from
39959ac to
b5ff6e9
Compare
syed-rafat
force-pushed
the
mark-read-for-private-narrow
branch
from
6077cb2 to
8c0b7aa
Compare
|
@timabbott It is up for review. |
syed-rafat
force-pushed
the
mark-read-for-private-narrow
branch
2 times, most recently
from
4b3b54e to
80beb9e
Compare
|
Works for me in manual testing! @laurynmm Would you be up for doing a round of review on this one? |
There was a problem hiding this comment.
@syed-rafat Thanks for working on this!
I made one main comment in the review, but it's just the beginning of looking at what it will take to add these filters to can_mark_messages_read.
The TODO comment above is_common_narrow probably applies here in that it would need to be rewritten to not use the functionality of can_mark_messages_read as a separate prep refactor to these changes. Otherwise, this small change has a much bigger impact and creates some visible bugs.
Let me know if you have any questions about the above and what the next steps would be.
web/src/filter.js
Outdated
|
|
||
| // Excluding direct messages from stream and topic does not | ||
| // accomplish anything, but we are still letting user mark | ||
| // mark messages as read to be consistent with our design. |
There was a problem hiding this comment.
There's no need to two duplicate comments here.
There was a problem hiding this comment.
Sorry about that, it slipped during last merge while rebasing.
core_filters This a pre commit to prepare for changes in zulip#25351 In web/src/filter, this initial logic is used by both can_mark_messages_read and is_common_narrow. Changes related to this pr zulip#25351 breaks is_common_narrow. As per TODO comment of is_common_narrow, we moved those filter logics to a core_filters method which is extended in both of them.
syed-rafat
force-pushed
the
mark-read-for-private-narrow
branch
from
80beb9e to
29aa356
Compare
|
@syed-rafat - You're going to want to reorder your commits so that the refactoring pre-commit is first and the other changes are second. Also, it looks like you forgot that you renamed Finally, you'll want to get all the tests and linters passing. I imagine that missed function rename might be what the linter caught here. |
This new method has all the filters that work with both can_mark_messages_read and is_common_narrow. This a pre commit to prepare for changes necessary in zulip#25351. is_common_narrow breaks without this refactor.
It will mark messages as read when applying -is:dm filter alone or coupled with stream and topic filters. But it will not mark messages as read when there is also search term present. Fixes zulip#25113.
syed-rafat
force-pushed
the
mark-read-for-private-narrow
branch
from
29aa356 to
97fd8d2
Compare
|
Hello @syed-rafat, it seems like you have referenced #25113 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage. Please run An example of a correctly-formatted commit: To learn how to write a great commit message, please refer to our guide. |
|
Heads up @syed-rafat, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
timabbott
force-pushed
the
main
branch
2 times, most recently
from
4ec3636 to
88b200c
Compare
|
@syed-rafat do you plan to address the feedback above, or should someone else take over from here? |
It will mark messages as read when applying -is:dm filter alone or coupled with stream and topic filters. But it will not mark messages as read when there is also search term present.
fixes #25113.