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

streams: Make default notifications settings customizable per-stream. #23943

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

Conversation

Ujjawal3
Copy link
Collaborator

@Ujjawal3 Ujjawal3 commented Jan 2, 2023

This PR introduces a new boolean field push_notifications_enabled in Steam model. If this field is set to true push notifications will be enabled by default for new subscribers of steam. Default value of this field is false.

Fixes: #23873

Screenshots and screen captures:
ScreenShot showing Stream Creation form for owner

Screenshot from 2023-01-23 22-06-42

Tooltip shown when push notifications are not configured on server.
Screenshot from 2023-03-02 21-16-15

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.

@Ujjawal3 Ujjawal3 marked this pull request as draft January 2, 2023 09:02
@Ujjawal3 Ujjawal3 force-pushed the Issue-23873 branch 2 times, most recently from af78140 to 011002f Compare January 2, 2023 10:08
@Ujjawal3 Ujjawal3 marked this pull request as ready for review January 2, 2023 10:42
@Ujjawal3 Ujjawal3 marked this pull request as draft January 2, 2023 10:42
@Ujjawal3 Ujjawal3 force-pushed the Issue-23873 branch 2 times, most recently from 2e09c83 to 38eebc4 Compare January 4, 2023 08:35
@Ujjawal3 Ujjawal3 force-pushed the Issue-23873 branch 8 times, most recently from 89fe500 to e40cc8e Compare January 5, 2023 20:07
@Ujjawal3 Ujjawal3 changed the title Add new feature settings:Make default notifications settings customizable per-stream Jan 5, 2023
@Ujjawal3 Ujjawal3 marked this pull request as ready for review January 5, 2023 20:15
@Ujjawal3 Ujjawal3 force-pushed the Issue-23873 branch 8 times, most recently from b8c3071 to d7f520d Compare January 8, 2023 08:06
@Ujjawal3 Ujjawal3 force-pushed the Issue-23873 branch 2 times, most recently from 1a8258c to bc98fa4 Compare April 15, 2023 12:18
@Ujjawal3 Ujjawal3 changed the title settings:Make default notifications settings customizable per-stream streams: Make default notifications settings customizable per-stream. Apr 20, 2023
@Ujjawal3 Ujjawal3 force-pushed the Issue-23873 branch 3 times, most recently from 75b6327 to 8fc92a7 Compare May 8, 2023 11:33
Added a new option for administrators in stream creation form
which allows to choose the default value of push notifications
when a stream in subscribed.

If set to true push notificatios will be enabled on subscribing
a stream. If set to false push notifications will depend on
user-level settings.

Fixes zulip#23873.
@zulipbot
Copy link
Member

Heads up @Ujjawal3, 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.

@timabbott timabbott added the completion candidate PRs with reviews that may unblock merging label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion candidate PRs with reviews that may unblock merging has conflicts integration review Added by maintainers when a PR may be ready for integration. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make default notifications settings customizable per-stream
5 participants