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

notifications: Add initial implementation for reaction notifications. #27910

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

notifications: Add initial implementation for reaction notifications. #27910

wants to merge 2 commits into from

Conversation

akarsh-jain-790
Copy link
Collaborator

@akarsh-jain-790 akarsh-jain-790 commented Nov 27, 2023 •
edited
Loading

Overview:

This PR introduces new feature – now you can get notified when someone reacts to your messages! 🎉
Here's a quick rundown of what's changed:

1. DM Checkbox:

  • Added a checkbox for you to decide if you want to know when someone reacts to your Direct Messages (DMs).

2. Stream Messages Dropdown:

  • Created a dropdown for messages in streams.
  • Choose how you want to be notified about reactions to your messages:
    - Never
    - In topics you follow
    - In unmuted topics
    - Always

'Always' is the default for stream messages

Fixes: #27327
CZO thread

Screenshots and screen captures:

SETTINGS / NOTIFICATIONS Screenshot-2024-02-26-at-3 25 42PM Screenshot-2024-02-26-at-3 25 28PM
Strings for Notifications: Screenshot 2024-02-28 at 2 13 38 PM Screenshot 2024-02-28 at 2 14 22 PM Screenshot 2024-02-28 at 2 14 46 PM Screenshot 2024-02-28 at 2 15 19 PM
Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@akarsh-jain-790 akarsh-jain-790 marked this pull request as ready for review December 30, 2023 19:56
@roanster007
Copy link
Collaborator

roanster007 commented Jan 21, 2024 •
edited
Loading

@akarsh-jain-790 you sure this is the right implementation...? cos I feel the server should send the reaction_notification event only to the message sender, but in your case, I find you have coupled received_reaction with add_reaction, which means every event of add_reaction, will also pass through the received_reaction.

@maltokyo
Copy link

looking forward to this one!

@akarsh-jain-790
Copy link
Collaborator Author

@akarsh-jain-790 you sure this is the right implementation...? cos I feel the server should send the reaction_notification event only to the message sender, but in your case, I find you have coupled received_reaction with add_reaction, which means every event that passes through add_reaction, will also pass through the received_reaction.

@roanster007 The received_reaction function logic ensures notifications are generated for reactions on a user's own messages and respects user settings for various notification scenarios. The coupling of add_reaction and received_reaction is intentional for consistent event triggering, ensuring message senders stay informed.

@akarsh-jain-790 akarsh-jain-790 force-pushed the issue--27896 branch 2 times, most recently from cb667a2 to a714533 Compare February 4, 2024 06:12
@akarsh-jain-790 akarsh-jain-790 force-pushed the issue--27896 branch 2 times, most recently from 8a29218 to c741c14 Compare February 11, 2024 09:45
@akarsh-jain-790 akarsh-jain-790 force-pushed the issue--27896 branch 9 times, most recently from b07ba96 to 83a8247 Compare February 28, 2024 08:15
@prakhar1144
Copy link
Member

@akarsh-jain-790 Can you rebase this PR? I think, this would be quick to review & tag for integration review.

@akarsh-jain-790 akarsh-jain-790 force-pushed the issue--27896 branch 3 times, most recently from e2855d5 to 7cacc23 Compare April 10, 2024 13:01
@akarsh-jain-790
Copy link
Collaborator Author

akarsh-jain-790 commented Apr 10, 2024 •
edited
Loading

@akarsh-jain-790 Can you rebase this PR? I think, this would be quick to review & tag for integration review.

@prakhar1144 I have rebase the PR, you can review now.

@akarsh-jain-790 akarsh-jain-790 force-pushed the issue--27896 branch 3 times, most recently from 4de8841 to e22fb92 Compare April 27, 2024 09:30
@alya alya mentioned this pull request Aug 27, 2024
Open
@akarsh-jain-790
settings: Implement user settings for reaction notifications.
51e33c3 
This commit introduces two new user settings:
`enable_dm_reaction_notifications` and `streams_reaction_notification`.
The `enable_dm_reaction_notifications` setting allows users to opt-in to
receive notifications for reactions in direct messages, while the
`streams_reaction_notification` setting enables users to choose whether
they want notifications for reactions in stream messages. These settings
provide users with flexible control over their notification preferences,
enhancing their overall messaging experience.
@akarsh-jain-790
notifications: Implement functionality for reaction notifications.
137d3d9 
This commit implements the functionality for reaction notifications
based on the user settings  enable_dm_reaction_notifications and
streams_reaction_notification.
@akarsh-jain-790
Copy link
Collaborator Author

@prakhar1144 I have rebase the PR and is ready for review.

@zulipbot
Copy link
Member

zulipbot commented Sep 9, 2024

Heads up @akarsh-jain-790, 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 upstream/main branch and resolve your pull request's merge conflicts accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prakhar1144 prakhar1144 Awaiting requested review from prakhar1144

Assignees
No one assigned
Labels
has conflicts maintainer review PR is ready for review by Zulip maintainers. size: XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Notify message sender about emoji reactions
6 participants
@akarsh-jain-790 @roanster007 @maltokyo @alya @prakhar1144 @zulipbot