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

user_settings: Automate 'Include realm name in missedmessage email subject'. #24075

Conversation

prakhar1144
Copy link
Member

@prakhar1144 prakhar1144 commented Jan 14, 2023

Under Settings > Notifications, currently, there is a checkbox setting for whether to "Include realm name in subject of message notification emails".

This commit replaces the checkbox setting with a dropdown: Automatic [default], Always, Never

The Automatic option includes the realm name if, and only if, there are multiple Zulip realms associated with the user's email.

Tests are added and(or) modified.

Fixes: #19905.


Screenshots and screen captures:

Following four cases are possible:

Always
Never

2 cases in Automatic

User part of a single org
User part of multiple orgs

Settings Changes

Current
Updated
Dropdown

Second Commit

Current Doc
Updated Doc

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.

@zulipbot
Copy link
Member

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

@prakhar1144 prakhar1144 force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch 4 times, most recently from 94d68e8 to b8a32e0 Compare January 15, 2023 07:38
prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Jan 15, 2023
This commit updates the help page 'email-notification.md' to reflect, the
change of 'Include organization name in subject line' settings from checkbox
to dropdown in zulip#24075.
prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Jan 15, 2023
This commit updates the help page 'email-notification.md' to reflect, the
change of 'Include organization name in subject line' settings from checkbox
to dropdown in zulip#24075.
@prakhar1144 prakhar1144 force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch from d3aa872 to 074f2d9 Compare January 15, 2023 13:29
@prakhar1144 prakhar1144 marked this pull request as ready for review January 15, 2023 14:24
@prakhar1144
Copy link
Member Author

@alya Can you please review this PR?

@alya
Copy link
Contributor

alya commented Jan 20, 2023

Thanks! Could you please add a screenshot of the settings changes as well?

@prakhar1144
Copy link
Member Author

@alya Added -- Thanks.

@alya
Copy link
Contributor

alya commented Jan 20, 2023

Sorry, where's the screenshot?

@prakhar1144
Copy link
Member Author

@alya
Ah, extremely sorry. Actually, I somehow forgot to click the update comment button after preview and closed the tab.
Please have a look now.

@alya
Copy link
Contributor

alya commented Jan 23, 2023

Thanks!

@sahil839 Would you be up for reviewing this one?

I think we'll want to migrate users who haven't toggled this setting to "Automatic", as the issue suggests. I don't know if that's included in the PR already.

@prakhar1144
Copy link
Member Author

I think we'll want to migrate users who haven't toggled this setting to "Automatic", as the issue suggests. I don't know if that's included in the PR already.

Yes, It is included here in this file.

Copy link
Collaborator

@sahil839 sahil839 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted a couple of comments. Looks good overall.

Also, we do not set the old values for this setting in RealmUserDefault in the migration. Is there any particular reason for that?

Also I think we can have a better name for the migration file than the current name. (Recently such migrations were divided into multiple files - like one for adding new field, second for setting the new field to values of old fields and third for removing the old field, but I think this makes more sense only when we are renaming the field and having a single file should work fine here.)

zerver/openapi/zulip.yaml Show resolved Hide resolved
static/js/settings_notifications.js Outdated Show resolved Hide resolved
prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Jan 23, 2023
This commit updates the help page 'email-notification.md' to reflect, the
change of 'Include organization name in subject line' settings from checkbox
to dropdown in zulip#24075.
@prakhar1144 prakhar1144 force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch from 074f2d9 to 2c8fc72 Compare January 23, 2023 14:26
prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Jan 23, 2023
This commit updates the help page 'email-notification.md' to reflect, the
change of 'Include organization name in subject line' settings from checkbox
to dropdown in zulip#24075.
@prakhar1144 prakhar1144 force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch from 2c8fc72 to 3ac6524 Compare January 23, 2023 14:37
@prakhar1144
Copy link
Member Author

@sahil839

Also, we do not set the old values for this setting in RealmUserDefault in the migration. Is there any particular reason for that?

I have corrected this. -- Thanks. (I missed that earlier :) )

Also I think we can have a better name for the migration file

I have updated it to 0423_alter_and_migrate_realm_name_in_notifications.py
Does it look better?

@sahil839
Copy link
Collaborator

Looks good. (I have not reviewed the docs changes).

I have updated it to 0423_alter_and_migrate_realm_name_in_notifications.py
Does it look better?

Yes.

@alya
Copy link
Contributor

alya commented Jan 24, 2023

@timabbott This PR has been reviewed by @sahil839 and me, and is ready for you to take a look.

Please don't merge the documentation commit; I will tweak it once the feature has been integrated.

@alya alya added the integration review Added by maintainers when a PR may be ready for integration. label Jan 24, 2023
prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Jan 26, 2023
This commit updates the help page 'email-notification.md' to reflect, the
change of 'Include organization name in subject line' settings from checkbox
to dropdown in zulip#24075.
prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Mar 5, 2023
This commit updates the help page 'email-notification.md' to reflect
the change in zulip#24075 from a checkbox to a dropdown for the
'Include organisation name in subject line' setting.
@prakhar1144 prakhar1144 force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch from 6c0f006 to 9c3c804 Compare March 5, 2023 14:10
@prakhar1144
Copy link
Member Author

prakhar1144 commented Mar 5, 2023

@timabbott This is ready to review.

Update in the recent push:

  • Use of intermediate variable in migration removed.
  • renamed realm_name_in_notifications_policy to realm_name_in_email_notifications_policy (comment)
  • renamed realm_name_allowed_in_missedmessage_emails_subject to include_realm_name_in_missedmessage_email_subject (comment)
  • Update a comment -- function description .
  • Both RemoveField operations at the end in migration. (comment)
  • Update API feature level

Comments which needs your attention (potential followup PR)

prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Mar 9, 2023
This commit updates the help page 'email-notification.md' to reflect
the change in zulip#24075 from a checkbox to a dropdown for the
'Include organisation name in subject line' setting.
@prakhar1144 prakhar1144 force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch from 9c3c804 to b334cef Compare March 9, 2023 10:26
prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Mar 9, 2023
This commit updates the help page 'email-notification.md' to reflect
the change in zulip#24075 from a checkbox to a dropdown for the
'Include organisation name in subject line' setting.
@prakhar1144 prakhar1144 force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch from b334cef to 559d300 Compare March 9, 2023 10:58
prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Mar 9, 2023
This commit updates the help page 'email-notification.md' to reflect
the change in zulip#24075 from a checkbox to a dropdown for the
'Include organisation name in subject line' setting.
@prakhar1144 prakhar1144 force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch from 559d300 to 86b41a6 Compare March 9, 2023 21:21
@prakhar1144
Copy link
Member Author

@timabbott This PR is ready to review after the minor changes.

Updates in the recent push


Also, earlier Alya said:

Please don't merge the documentation commit; I will tweak it once the feature has been integrated.

So, the first two commit would be good to merge (if it LGTM).

This commit adds 'zerver/lib/email_notifications.py'
to the FILES_WITH_LEGACY_SUBJECT set.

Because the file can have 'subject' in the email sense,
it should be exempted from the 'avoid subject as a var' lint rule.
Currently, there is a checkbox setting for whether to
"Include realm name in subject of message notification emails".

This commit replaces the checkbox setting with a dropdown
having values: Automatic [default], Always, Never.

The Automatic option includes the realm name if, and only if,
there are multiple Zulip realms associated with the user's email.

Tests are added and(or) modified.

Fixes: zulip#19905.
@timabbott timabbott force-pushed the issue-19905-automate-org-name-in-email-subject-setting branch from 86b41a6 to 5180fe2 Compare March 15, 2023 00:21
@timabbott
Copy link
Member

Dropped the documentation commit and marked this to merge once CI passes. Thanks so much for all the work on this @prakhar1144!

Here's the original draft of that change:

commit 11944208adfa9fcf3b1312d40d68ed369cc9a944
Author: Prakhar Pratyush <prakhar841301@gmail.com>
Date:   Sun Jan 15 15:48:43 2023 +0530

    docs: Update 'Include organization name in subject line' settings' docs.
    
    This commit updates the help page 'email-notification.md' to reflect
    the change in #24075 from a checkbox to a dropdown for the
    'Include organisation name in subject line' setting.

diff --git a/help/email-notifications.md b/help/email-notifications.md
index 400cd61bd3..31b266343a 100644
--- a/help/email-notifications.md
+++ b/help/email-notifications.md
@@ -57,15 +57,25 @@ To configure the delay for message notification emails:
 If you belong to multiple Zulip organizations, it can be helpful to have the
 name of the organization in the subject line of your message notification emails.
 
+By default, Zulip includes the organization name if, and only if, your email is
+associated with multiple Zulip organizations.
+
+You can configure Zulip to always or never include the organization name in the
+subject line.
+
 {start_tabs}
 
 {settings_tab|notifications}
 
-1. Under **Email message notifications**, toggle
+1. Under **Email message notifications**, configure
    **Include organization name in subject of message notification emails**.
 
 {end_tabs}
 
+The default is **Automatic**, which includes the organization name if, and only if, your email is
+associated with multiple Zulip organizations. **Always** and **Never** either include or don't include
+the organization name, regardless of how many organizations you are part of.
+
 
 ### Hide message content
 

@timabbott timabbott enabled auto-merge (rebase) March 15, 2023 00:22
@timabbott timabbott merged commit ae72777 into zulip:main Mar 15, 2023
@prakhar1144
Copy link
Member Author

Thanks.

@alya

Since the PR is merged, following comments need your attention:

ps: The PR description contains an image of updated docs (which I earlier proposed). In case that's helpful in any way.

@alya
Copy link
Contributor

alya commented Mar 15, 2023

Thanks! I don't want to dig through the comment thread -- what happened to the documentation commit (prakhar1144@c193fd4) you had before?

@alya
Copy link
Contributor

alya commented Mar 15, 2023

Assuming it hasn't been merged, you can put it up as a new PR, and I'll use that as a starting point.

prakhar1144 added a commit to prakhar1144/zulip that referenced this pull request Mar 15, 2023
This commit updates the help page 'email-notification.md' to reflect
the change in zulip#24075 from a checkbox to a dropdown for the
'Include organisation name in subject line' setting.
@prakhar1144
Copy link
Member Author

@alya

Here's the PR #24696 (with the description -- what happened to the documentation commit I had before)

@prakhar1144 prakhar1144 deleted the issue-19905-automate-org-name-in-email-subject-setting branch March 15, 2023 14:30
alya pushed a commit to alya/zulip that referenced this pull request Mar 16, 2023
Document new "Automatic" configuration option.

This commit updates the help page 'email-notification.md' to reflect
the change in zulip#24075 from a checkbox to a dropdown for the
'Include organisation name in subject line' setting.
timabbott pushed a commit that referenced this pull request Mar 16, 2023
Document new "Automatic" configuration option.

This commit updates the help page 'email-notification.md' to reflect
the change in #24075 from a checkbox to a dropdown for the
'Include organisation name in subject line' setting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: notifications (messages) area: settings (user) 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.

Automate "Include organization name in subject of message notification emails" setting
5 participants