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

Notify on alert words (#1301 rebase + tidy) #1499

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

Notify on alert words (#1301 rebase + tidy) #1499

wants to merge 4 commits into from

Conversation

neiljp
Copy link
Collaborator

@neiljp neiljp commented May 14, 2024 •
edited
Loading

What does this PR do, and why?

This takes #1301 and applies some tidying, following through with some of the changes I suggested.

Full notification logic remains incomplete for now, but this takes us closer.

Other than new features in the server, we do not take into account various user settings, which affects for example wildcard mentions.

@mounilKshah FYI Since you were involved in reviewing #1301, and I'm only testing this remotely right now.

External discussion & connections

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

neiljp and others added 4 commits May 14, 2024 11:25
@neiljp
refactor: tests: model: Use standard test formats (notify_user).
5d267e3 
- Inline test ids
- Add test ids
- Use expected_ prefix where applicable
- Name test functions as function_name__aspect_to_test
@neiljp
tests: model: Add tracking of muted streams & topics (notify_user).
df3c359 
Additional notes added clarify existing notification ids.

This should not affect behavior, but is more regarding documentation of
the current behavior - it does not represent the desired complete
behavior.
@Subhasish-Behera @neiljp
model: Add preliminary notification of alert-words.
a7b327c 
In the notify_user function a condition of "has_alert_word" is added
which also checks if stream/topic is muted (unlike mentions and
wild-card_mentions).

Added new test cases, relying on is_topic_muted/is_stream_muted.
(addition of those parameters split out by neiljp into earlier commit)

Original tests also adjusted by neiljp to simplify conditional through
use of test parameters
@neiljp
refactor: model: Combine conditions in notify for stream.
cbec3c1
@neiljp neiljp added missing feature: user A missing feature for all users, present in another Zulip client area: notifications labels May 14, 2024
@zulipbot zulipbot added the size: XL [Automatic label added by zulipbot] label May 14, 2024
@zulipbot
Copy link
Member

Hello @zulip/server-notifications members, this pull request was labeled with the "area: notifications" label, so you may want to check it out!

@neiljp neiljp requested a review from mounilKshah May 29, 2024 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mounilKshah mounilKshah Awaiting requested review from mounilKshah

Assignees
No one assigned
Labels
area: notifications 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
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@neiljp @zulipbot @Subhasish-Behera