settings: Fix "message content in message notification emails". #27543

kartikbhtt7 · 2023-11-04T14:27:32Z

In user settings, if "message content in message notification emails" is disabled at an Organisation level, then the "Allow
message content in message notification emails" should be shown both disabled and greyed out. At present a user can still
enable this box, and messages notification emails still do not contain message content

This PR fixes this issue.

Fixes: #27262

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 · 2023-11-04T14:27:39Z

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

alya · 2023-11-08T12:19:17Z

Thanks for working on this, @kartikbhtt7 !

Please fix the failing tests and post a comment requesting a review.

alya · 2023-11-08T12:21:21Z

You will also need to include a screenshot showing the tooltip.

kartikbhtt7 · 2023-11-09T17:11:21Z

@alya
Fixed the failing tests and added the screenshot of tooltip.
I think it is ready for review

alya · 2023-11-10T00:49:31Z

Thanks! The tooltip should be on the disabled setting itself. We don't want a separate i icon.

kartikbhtt7 · 2023-11-18T20:53:25Z

Thanks! The tooltip should be on the disabled setting itself. We don't want a separate i icon.

@alya
Fixed that in recent commit.

alya · 2023-11-20T17:24:38Z

Thanks! The screenshot looks good to me now.

@sahil839 could you review this one when you have time (not an 8.0 goal)?

sahil839

Posted couple of comments.

sahil839 · 2023-12-18T10:02:08Z

web/src/settings.js

+    const settings_is_disabled = {
+        message_content_in_email_notifications:
+            !page_params.realm_message_content_allowed_in_email_notifications,
+    };


I think you can directly pass message_content_in_email_notifications_disabled as an argument below.

fixed in the latest commit

Ok, I was thinking of something else, but then I noticed that we use a loop to render the email notification settings. Hmm, I am not sure this is the right way to do this. We probably need to refactor some code in settings_config.ts that handles all these notification setttings.

We have show_push_notifications_tooltip which gives the status of whether the setting is disabled or not, but that is currently used only for mobile push notification related settings. We can probably rename it to something like disabled_notification_settings and then add message_content_allowed_in_email_notifications in that object.

A temporary workaround could be to disable the settings in JS rather in doing it in handlebar templates, but I guess the above approach seems good for long-term. Tbh I was confused why we use show_push_notifications_tooltip object to check whether the setting is enabled or not. @timabbott your thoughts on this?

I think renaming this object to disabled_notification_settings would probably suffice.

sahil839 · 2023-12-18T10:05:09Z

web/src/tippyjs.ts

+        appendTo: () => document.body,
+        onHidden(instance) {
+            instance.destroy();
+        },


The tooltip is shown even when the setting is not disabled. Please look at existing examples in the codebase where we show tooltip only for disabled cases.

fixed in the latest commit

zulip#27543

zulipbot · 2023-12-27T11:30:58Z

Hello @kartikbhtt7, it seems like you have referenced #27262 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #27262..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

timabbott · 2024-02-13T18:38:45Z

@kartikbhtt7 are you planning to address the feedback on this? I've tagged it as something other developers can pick up since it's been quite a while.

kartikbhtt7 · 2024-02-13T18:54:50Z

sure I'll address the feedbacks.

zulip#27543

kartikbhtt7 · 2024-03-01T19:40:40Z

addressed the feedback

sahil839

A few points -

There are some unrelated changes which should be fixed.
Can you also implement the changes discussed in https://github.com/zulip/zulip/pull/27543/files#r1438156860. Would be good if there is a separate commit for renaming the object in settings_config and then the commit which disables the setting.
Please update the commit message to mention the issue number there is no need to mention the PR number. And it should be written as "Fixes part of Changes to "Allow message content in message notification emails" #27262.", since the issue should still remain open.

kartikbhtt7 · 2024-03-05T16:20:25Z

There are some unrelated changes which should be fixed.

ig these are some changes that got introduced by the ./tools/lint command

will implement rest of the changes as mentioned.

sahil839 · 2024-03-06T16:24:36Z

ig these are some changes that got introduced by the ./tools/lint command

I don't think that would be the case. Will most probably be fixed by rebasing onto main.

zulipbot · 2024-05-31T22:26:49Z

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

zulipbot added size: S area: notifications (messages) area: settings UI bug labels Nov 4, 2023

kartikbhtt7 force-pushed the first_dev_branch branch 2 times, most recently from 0fbbde0 to 610570e Compare November 4, 2023 19:34

kartikbhtt7 force-pushed the first_dev_branch branch from 610570e to 41e42da Compare November 9, 2023 16:40

kartikbhtt7 force-pushed the first_dev_branch branch 2 times, most recently from ed6ab14 to 8b83f4b Compare November 18, 2023 20:50

kartikbhtt7 force-pushed the first_dev_branch branch from 8b83f4b to 4048ea3 Compare November 20, 2023 17:15

alya added the maintainer review PR is ready for review by Zulip maintainers. label Nov 20, 2023

sahil839 reviewed Dec 18, 2023

View reviewed changes

kartikbhtt7 force-pushed the first_dev_branch branch from 4048ea3 to 737cd99 Compare December 27, 2023 11:30

kartikbhtt7 added a commit to kartikbhtt7/zulip that referenced this pull request Dec 27, 2023

settings: Fix "message content in message notification emails".

737cd99

zulip#27543

timabbott added the completion candidate PRs with reviews that may unblock merging label Feb 13, 2024

settings: Fix "message content in message notification emails".

61c5dbe

zulip#27543

kartikbhtt7 force-pushed the first_dev_branch branch from 737cd99 to 61c5dbe Compare March 1, 2024 19:38

zulipbot added size: M and removed size: S labels Mar 1, 2024

sahil839 reviewed Mar 4, 2024

View reviewed changes

zulipbot added the has conflicts label May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

settings: Fix "message content in message notification emails". #27543

settings: Fix "message content in message notification emails". #27543

kartikbhtt7 commented Nov 4, 2023 •

edited

zulipbot commented Nov 4, 2023

alya commented Nov 8, 2023

alya commented Nov 8, 2023

kartikbhtt7 commented Nov 9, 2023 •

edited

alya commented Nov 10, 2023

kartikbhtt7 commented Nov 18, 2023

alya commented Nov 20, 2023

sahil839 left a comment

sahil839 Dec 18, 2023

kartikbhtt7 Dec 27, 2023

sahil839 Dec 29, 2023

sahil839 Dec 29, 2023

timabbott Feb 13, 2024

sahil839 Dec 18, 2023

kartikbhtt7 Dec 27, 2023

zulipbot commented Dec 27, 2023

timabbott commented Feb 13, 2024

kartikbhtt7 commented Feb 13, 2024

kartikbhtt7 commented Mar 1, 2024

sahil839 left a comment

kartikbhtt7 commented Mar 5, 2024

sahil839 commented Mar 6, 2024

zulipbot commented May 31, 2024

settings: Fix "message content in message notification emails". #27543

Are you sure you want to change the base?

settings: Fix "message content in message notification emails". #27543

Conversation

kartikbhtt7 commented Nov 4, 2023 • edited

zulipbot commented Nov 4, 2023

alya commented Nov 8, 2023

alya commented Nov 8, 2023

kartikbhtt7 commented Nov 9, 2023 • edited

alya commented Nov 10, 2023

kartikbhtt7 commented Nov 18, 2023

alya commented Nov 20, 2023

sahil839 left a comment

Choose a reason for hiding this comment

sahil839 Dec 18, 2023

Choose a reason for hiding this comment

kartikbhtt7 Dec 27, 2023

Choose a reason for hiding this comment

sahil839 Dec 29, 2023

Choose a reason for hiding this comment

sahil839 Dec 29, 2023

Choose a reason for hiding this comment

timabbott Feb 13, 2024

Choose a reason for hiding this comment

sahil839 Dec 18, 2023

Choose a reason for hiding this comment

kartikbhtt7 Dec 27, 2023

Choose a reason for hiding this comment

zulipbot commented Dec 27, 2023

timabbott commented Feb 13, 2024

kartikbhtt7 commented Feb 13, 2024

kartikbhtt7 commented Mar 1, 2024

sahil839 left a comment

Choose a reason for hiding this comment

kartikbhtt7 commented Mar 5, 2024

sahil839 commented Mar 6, 2024

zulipbot commented May 31, 2024

kartikbhtt7 commented Nov 4, 2023 •

edited

kartikbhtt7 commented Nov 9, 2023 •

edited