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

invites: Add option to receive direct notification on accepted invitations. #28819

Merged
merged 1 commit into from
Jul 6, 2024

Conversation

shashank-23002
Copy link
Collaborator

@shashank-23002 shashank-23002 commented Feb 4, 2024

Previously, when a user's invitation to Zulip was accepted, they received a direct message from welcome bot that their invitations has been accepted. However, some users may not want to receive such a message, e.g. because they are already following new user notifications in a stream, or they simply don't need to know when new users join.

Presently , there is an additional personal notification setting using you which you can decide whether you want to receive direct message from welcome bot when an invited user joins.

Fixes: #20398.

Screenshots and screen captures:

Screenshot 2024-02-06 at 3 31 29 PM Screenshot 2024-02-06 at 11 45 42 AM

When the button is checked -

Screenshot 2024-02-06 at 12 38 52 PM

The notification bot message popped up for test2.

When the button is unchecked -

Screenshot 2024-02-06 at 12 39 16 PM

The notification bot message didn't pop up for test1.

Screen.Recording.2024-02-04.at.6.10.59.PM.mov
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

zulipbot commented Feb 4, 2024

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

@shashank-23002 shashank-23002 force-pushed the issue-20398 branch 3 times, most recently from 5c4280a to 8cd21af Compare February 6, 2024 05:56
@alya
Copy link
Contributor

alya commented Feb 6, 2024

As requested in the PR template, please post screenshots of all your changes in the PR description. It is very difficult to review a video.

@shashank-23002
Copy link
Collaborator Author

Yeah sure, I have updated the PR description to add UI changes.
Though , I haven't removed the video so that complete workflow of the changes can be reviewed.

@alya
Copy link
Contributor

alya commented Feb 6, 2024

Thanks! Looking at the screenshots, in the invite modal, let's move the checkbox to be just below the Emails text box. I've updated the issue as well.

@alya
Copy link
Contributor

alya commented Feb 6, 2024

(I also filed #28834 while looking at this PR.)

@shashank-23002
Copy link
Collaborator Author

shashank-23002 commented Feb 6, 2024

Sure, I have updated the code as well as created a PR fixing the new issue.

@alya
Copy link
Contributor

alya commented Feb 7, 2024

Thanks! @sahil839 are you up for reviewing this one? I haven't tested.

@alya alya added the maintainer review PR is ready for review by Zulip maintainers. label Feb 7, 2024
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 few comments.

zerver/views/invite.py Outdated Show resolved Hide resolved
zerver/actions/invites.py Outdated Show resolved Hide resolved
web/templates/invite_user_modal.hbs Outdated Show resolved Hide resolved
zerver/models/prereg_users.py Outdated Show resolved Hide resolved
@shashank-23002 shashank-23002 force-pushed the issue-20398 branch 3 times, most recently from 99aeaaf to 383656e Compare February 14, 2024 19:43
@shashank-23002
Copy link
Collaborator Author

@sahil839 I have addressed the previous feedbacks. Hope that works.

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.

@shashank-23002 Posted a few comments related to avoiding passing the paramters where we already have a default for now in tests and migration.

Also, you should add a test for checking that the message is not sent when send_notification is false.

zerver/models/prereg_users.py Outdated Show resolved Hide resolved
zerver/tests/test_invite.py Outdated Show resolved Hide resolved
zerver/tests/test_auth_backends.py Outdated Show resolved Hide resolved
zerver/tests/test_i18n.py Outdated Show resolved Hide resolved
web/templates/invite_user_modal.hbs Outdated Show resolved Hide resolved
@shashank-23002
Copy link
Collaborator Author

Thanks @sahil839 for the review. I think I have cleaned up the PR.
Though I am not so used to writing new tests for this kind of feature , would be great if you could provide me a little reference to test this.

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.

You need to test whether the message was received by the referrer when send_notification is True and not received when send_notification is False for the invite. You can do this by doing an appropriate query like we do here.

You can read the existing tests and then write the test accordingly.

corporate/tests/test_support_views.py Outdated Show resolved Hide resolved
web/templates/invite_user_modal.hbs Outdated Show resolved Hide resolved
zerver/tests/test_invite.py Outdated Show resolved Hide resolved
@shashank-23002
Copy link
Collaborator Author

@timabbott PTAL

@timabbott
Copy link
Sponsor Member

Cool thanks for bumping this, I'll see if I can review today.

@shashank-23002
Copy link
Collaborator Author

@timabbott bumping this thread so that this may not lose track.

@shashank-23002
Copy link
Collaborator Author

@timabbott rebased the PR . Would be great if this could be reviewed fast as frequent conflicts arise and this might lose track now.

@timabbott
Copy link
Sponsor Member

Merged after making the following fixes to formatting and documentation language.

@shashank-23002 thanks for the contribution!

diff --git a/api_docs/changelog.md b/api_docs/changelog.md
index a5e54d4499..44ea8dca60 100644
--- a/api_docs/changelog.md
+++ b/api_docs/changelog.md
@@ -22,10 +22,10 @@ format used by the Zulip server that they are interacting with.
 
 **Feature level 267**
 
-* [`GET /invites`](/api/get-invites),[`POST /invites`](/api/send-invites):
-  Added a new field `notify_referrer_on_join`
-  indicating whether the referrer has opted to receive direct messages
-  from the notification bot when an invited user joins.
+* [`GET /invites`](/api/get-invites),[`POST /invites`](/api/send-invites): Added
+  `notify_referrer_on_join` parameter, indicating whether the referrer has opted
+  to receive a direct message from the notification bot whenever a user joins
+  via this invitation.
 
 **Feature level 266**
 
diff --git a/web/templates/invite_user_modal.hbs b/web/templates/invite_user_modal.hbs
index 68115b62ee..d9ae432f94 100644
--- a/web/templates/invite_user_modal.hbs
+++ b/web/templates/invite_user_modal.hbs
@@ -30,7 +30,7 @@
         <label class="checkbox display-block">
             <input type="checkbox" id="receive-invite-acceptance-notification" checked/>
             <span></span>
-            {{t "Send me a direct message when my invitation is accepted"}}
+            {{t "Send me a direct message when my invitation is accepted" }}
         </label>
     </div>
     <div class="input-group">
diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml
index 3700759116..71b1e99f9b 100644
--- a/zerver/openapi/zulip.yaml
+++ b/zerver/openapi/zulip.yaml
@@ -12515,9 +12515,13 @@ paths:
                   example: false
                 notify_referrer_on_join:
                   description: |
-                    "A boolean indicating whether the referrer would like to receive a direct message from Notification Bot a user account is created using this invitation."
+                    A boolean indicating whether the referrer would like to receive a
+                    direct message from [notification
+                    bot](/help/configure-automated-notices) when a user account is created
+                    using this invitation.
 
-                    **Changes**: New in Zulip 9.0 (feature level 267).Previously, referrers always received such direct messages.
+                    **Changes**: New in Zulip 9.0 (feature level 267). Previously,
+                    referrers always received such direct messages.
                   type: boolean
                   example: false
                   default: true
@@ -20701,11 +20705,12 @@ components:
         notify_referrer_on_join:
           type: boolean
           description: |
-            A boolean specifying whether the referrer has opted to receive direct messages
-            from the notification bot when an invited user joins.
+            A boolean indicating whether the referrer has opted to receive a direct
+            message from [notification bot](/help/configure-automated-notices) when a user
+            account is created using this invitation.
 
-            **Changes**: New in Zulip 9.0 (feature level 267).Previously, referrers always
-            received such direct messages.
+            **Changes**: New in Zulip 9.0 (feature level 267). Previously,
+            referrers always received such direct messages.
         link_url:
           type: string
           description: |

@timabbott timabbott enabled auto-merge (rebase) July 2, 2024 23:21
@alya
Copy link
Contributor

alya commented Jul 4, 2024

@shashank-23002 are you up for updating https://zulip.com/help/invite-new-users#send-email-invitations to document this feature?

Previously, when a referrer's invitation to Zulip was accepted,
they got a notification from notification-bot indicating
their invitation has been accepted.

This commit adds an option for referrer to decide
whether he wants to receive the direct notification
from the notification-bot.

Fixes: zulip#20398
auto-merge was automatically disabled July 4, 2024 15:15

Head branch was pushed to by a user without write access

@shashank-23002
Copy link
Collaborator Author

Fixed failing tests , so a last quick view by @timabbott would do !!

@shashank-23002 are you up for updating https://zulip.com/help/invite-new-users#send-email-invitations to document this feature?

@alya I would love to when I get time , but someone else may take this up for now if they want.

@alya
Copy link
Contributor

alya commented Jul 5, 2024

OK. @laurynmm could you please add this bit of documentation to your list, then, so that it's done for 9.0?

@timabbott timabbott merged commit 4cce94b into zulip:main Jul 6, 2024
14 checks passed
@timabbott
Copy link
Sponsor Member

timabbott commented Jul 6, 2024

Merged, thanks @shashank-23002!

Next time, post the diff for what you changed, it would make review a lot easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option for whether to receive a direct message when invited users join
5 participants