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

Resolve topic should mark topic as unread only for participants #19709

Closed
alya opened this issue Sep 10, 2021 · 24 comments · Fixed by #20977
Closed

Resolve topic should mark topic as unread only for participants #19709

alya opened this issue Sep 10, 2021 · 24 comments · Fixed by #20977
Assignees
Labels

Comments

@alya
Copy link
Contributor

alya commented Sep 10, 2021

At present, resolving a topic marks it as unread. This can be annoying to users, as it unnecessarily bumps the topic, when in fact the intent is to clearly mark it as being "done". However, participants in a topic may want to be alerted when a topic is resolved, e.g. to make sure that they agree with the decision to resolve it.

To balance between these needs, resolving or unresolving a topic should create new unread messages only for users who have participated in the topic. For all others, the resolve/unresolve messages should automatically be marked as read.

When Follow topic is implemented in #6027, this behavior should be altered so that resolving/unresolving topics creates unreads for anyone following the topic.

This change is important for resolve topic workflows, so we should consider it a release goal.

CZO discussion thread

@alya alya added help wanted priority: high release goal We'd like to resolve this for the current major release area: message-editing labels Sep 10, 2021
@zulipbot
Copy link
Member

Hello @zulip/server-message-view members, this issue was labeled with the "area: message-editing" label, so you may want to check it out!

@mitanshu001
Copy link
Collaborator

@zulipbot claim

@zulipbot
Copy link
Member

Hello @mitanshu001, it looks like you've currently claimed 1 issue in this repository. We encourage new contributors to focus their efforts on at most 1 issue at a time, so please complete your work on your other claimed issues before trying to claim this issue again.

We look forward to your valuable contributions!

@zulipbot
Copy link
Member

ERROR: You have not claimed this issue to work on yet.

@mitanshu001
Copy link
Collaborator

@zulipbot claim.
@alya are the required changes are of Python(backend)? I am new to the community. This will be my 2nd issue. Please let me know if this is not a good issue for beginners

@alya
Copy link
Contributor Author

alya commented Sep 13, 2021

@timabbott will have more insight into whether this is a good beginner issue.

@timabbott
Copy link
Sponsor Member

This is probably accessible to someone good at using git grep and similar to trace call flows in a Python codebase. Structurally, you want to pass the mark_as_read parameter through to to do_send_messages from the maybe_send_resolve_topic_notifications call to internal_send_stream_message.

Except that's probably awkward, since the participants are rare (So in a 10K user stream, you'll be passing 10K-7 users around). So probably we want a new unread_for_participants_only parameter that touches that same code.

I expect it won't merge quickly, as we'll need some iteration (e.g. we may refactor it 2-3 times after getting something basic working that has tests), but it's an OK thing to work on for someone experienced in development in general but not the Zulip codebase specifically.

@mitanshu001
Copy link
Collaborator

Thanks, Tim for the explanation
Unassignning myself as I am not getting much time currently, so other people can pick it up. In case no one starts working on it in 4-5 days will pick it up.
@zulipbot abandon

@collinwhitlow
Copy link
Collaborator

@zulipbot claim

@zulipbot
Copy link
Member

Welcome to Zulip, @collinwhitlow! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

@zulipbot
Copy link
Member

ERROR: You have not claimed this issue to work on yet.

@collinwhitlow
Copy link
Collaborator

@zulipbot claim

@zulipbot
Copy link
Member

Hello @collinwhitlow, it looks like we've already sent you a collaboration invite at https://github.com/zulip/zulip/invitations, but you haven't accepted it yet!

Please accept the invite and try to claim this issue again afterwards. We look forward to your contributions!

@collinwhitlow
Copy link
Collaborator

@alya my invitation expired before I had a chance to get started on it. Do you have any idea how I can go about getting a new one so I can successfully claim this issue?

@alya
Copy link
Contributor Author

alya commented Nov 23, 2021

We may need to manually cancel and resend it? I'm not sure. CC @timabbott

@timabbott
Copy link
Sponsor Member

Sent a new one.

@collinwhitlow
Copy link
Collaborator

Thank you, @timabbott
@zulipbot claim

@zulipbot
Copy link
Member

Hello @collinwhitlow, it looks like you've currently claimed 1 issue in this repository. We encourage new contributors to focus their efforts on at most 1 issue at a time, so please complete your work on your other claimed issues before trying to claim this issue again.

We look forward to your valuable contributions!

@collinwhitlow
Copy link
Collaborator

@timabbott Since I am new to contributing to Zulip, do you happen to know which files would be the most beneficial to start with for this issue?? Thank you!

@alya
Copy link
Contributor Author

alya commented Nov 28, 2021

@collinwhitlow please take a look at the (recently updated) Zulip contributor guide to learn more about how to get help. Also, keep in mind the following guideline:

Before you claim an issue, you should be confident that you will be able to tackle it effectively.

I will go ahead and unassign this issue, and you should feel free to re-claim it once you have figured out how to approach it (or pick a different one if you prefer).

@collinwhitlow
Copy link
Collaborator

That's understandable. I spent some time reviewing that link, and I am now more confident I am in a position to succeed. I'll give this another shot.
@zulipbot claim

@timabbott
Copy link
Sponsor Member

Cool, let me know when you have a draft PR ready for review; this is one of the issues I'm tracking as important to include in the upcoming 5.0 release (final likely in January, but we want to merge as many things like this as we can before then).

@zulipbot
Copy link
Member

zulipbot commented Dec 18, 2021

Hello @collinwhitlow, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 14 days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

timabbott added a commit to timabbott/zulip that referenced this issue Jan 29, 2022
Previously, users found it annoying that the automated "Resolve topic"
notifications triggered an unread for everyone in the stream; this
discouraged some users from using the feature on older threads for
fear of being annoying. We change this to a better default, of only
users who participated in the topic (via either messages or reactions)
being eligible for the new message being unread.

We will likely want to create global and stream-level notifications
settings to control this behavior as a follow-up -- some users, like
me, might prefer the simpler "Always unread" behavior in some streams.

Note that the automated notifications that a topic was resolved will
still result in the topic being moved to the top of the left sidebar.
This would be somewhat difficult to change, since the left sidebar
algorithm just looks at the highest message ID in the topic.

Fixes zulip#19709.
@timabbott
Copy link
Sponsor Member

I opened #20977 to cover this.

amanagr pushed a commit to timabbott/zulip that referenced this issue Jan 31, 2022
Previously, users found it annoying that the automated "Resolve topic"
notifications triggered an unread for everyone in the stream; this
discouraged some users from using the feature on older threads for
fear of being annoying. We change this to a better default, of only
users who participated in the topic (via either messages or reactions)
being eligible for the new message being unread.

We will likely want to create global and stream-level notifications
settings to control this behavior as a follow-up -- some users, like
me, might prefer the simpler "Always unread" behavior in some streams.

Note that the automated notifications that a topic was resolved will
still result in the topic being moved to the top of the left sidebar.
This would be somewhat difficult to change, since the left sidebar
algorithm just looks at the highest message ID in the topic.

Fixes zulip#19709.

Tests added by Aman Agrawal (amanagr@zulip.com).
tiger-yash pushed a commit to tiger-yash/zulip that referenced this issue Mar 8, 2022
Previously, users found it annoying that the automated "Resolve topic"
notifications triggered an unread for everyone in the stream; this
discouraged some users from using the feature on older threads for
fear of being annoying. We change this to a better default, of only
users who participated in the topic (via either messages or reactions)
being eligible for the new message being unread.

We will likely want to create global and stream-level notifications
settings to control this behavior as a follow-up -- some users, like
me, might prefer the simpler "Always unread" behavior in some streams.

Note that the automated notifications that a topic was resolved will
still result in the topic being moved to the top of the left sidebar.
This would be somewhat difficult to change, since the left sidebar
algorithm just looks at the highest message ID in the topic.

Fixes zulip#19709.

Tests added by Aman Agrawal (amanagr@zulip.com).
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 a pull request may close this issue.

5 participants