invites: Do not show streams if user cannot subscribe others. #25739

sahil839 · 2023-05-24T14:48:59Z

We do not show the streams list in invite modal if the user does not have permission to subscribe others to stream.

Screenshots and screen captures:

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.

sahil839 · 2023-05-24T16:10:08Z

@alya can you please review?

alya · 2023-05-24T17:44:39Z

Hm, I feel like the banner will be a bit confusing if you don't know about the feature to configure streams for new users in the first place. Maybe we should do something like this?

The users you invite will be automatically added to default streams for this organization, as you do not have permission to configure which streams new users join.

alya · 2023-05-24T17:45:12Z

I'd also just put it at the top where our banners generally go -- I don't think it needs to be in the same place as the stream config.

sahil839 · 2023-05-25T02:50:08Z

Hm, I feel like the banner will be a bit confusing if you don't know about the feature to configure streams for new users in the first place. Maybe we should do something like this?

The users you invite will be automatically added to default streams for this organization, as you do not have permission to configure which streams new users join.

Will update the banner.

Just one thing to clear, we do not add users to default streams in this case due to recent API changes. Since user is not allowed to add users to the streams, we just do not allow passing anything inside stream_ids and thus we consider that as stream_ids was empty we do not add users to any streams.

Do we want to change this to set stream_ids to default streams if user is not allowed to subscribe others?

alya · 2023-05-25T05:48:52Z

Do we want to change this to set stream_ids to default streams if user is not allowed to subscribe others?

That would make the most sense to me.

sahil839

Updated the PR to have the new message and the message to be on top. Also added commit to subscribe the user to default streams even when the user who invited does not have permission to subscribe others.

Also, added commits to use subscribe_to_default_streams parameter such that user is subscribed to default streams at the time of creation of account and not at the time of inviting them. Have posted some questions related to API for Tim.

sahil839 · 2023-06-08T16:52:36Z

zerver/views/invite.py

@@ -54,6 +53,7 @@ def invite_users_backend(
        default=PreregistrationUser.INVITE_AS["MEMBER"],
    ),
    stream_ids: List[int] = REQ(json_validator=check_list(check_int)),
+    subscribe_to_default_streams: bool = REQ(json_validator=check_bool, default=True),


I am not sure what would be the best default value here. Since the default value for stream_ids is [], I have kept the default value for subscribe_to_default_streams to be True.

Hmm, on the other hand False might be nicer in the sense that with True default, if you pass non-empty stream_ids, you have to also pass subscribe_to_default_streams or you'll run into the error below due to if len(stream_ids) and subscribe_to_default_streams:, right?

While with the default being False, you can just pass stream_ids without worrying about the other parameter's default getting in the way. Perhaps that's more logical? Not sure... 🤔

Thinking about it more, I kinda agree that False can be a better default. Updated the PR for now. Can change it again if feedback from others suggests otherwise.

Do we want also want to change the DB level default for this then?

I think True might be fine as a database-level default, but I think we need a RunPython migration step to make sure that it's set correctly for preexisting invitation objects -- presumably any that had stream_ids set should have a value of True.

While you're it it, we might want to do a multi-step migration where we first add a null=True field, then backfill it, and then make it non-null; these can be big tables.

sahil839 · 2023-06-08T17:28:41Z

zerver/views/invite.py

@@ -74,6 +74,9 @@ def invite_users_backend(

    invitee_emails = get_invitee_emails_set(invitee_emails_raw)

+    if len(stream_ids) and subscribe_to_default_streams:
+        raise JsonableError(_("Invalid parameters"))


We currently do not allow user to pass non empty list in stream_ids and subscribe_to_default_streams as True together. I cannot think of a concise error message, so would love to have some suggestions in here.

"Invalid parameters: stream_ids and subscribe_to_default_streams are mutually exclusive". Something like that perhaps?

Yes, this is better than "Invalid parameters". Updated the PR.

This works for me; I think @laurynmm might be working on a very similar string this week related to the stream/channel migration, so ideally we'd make a shared exception class that just takes a list of parameters and uses common code? Not sure.

sahil839 · 2023-06-08T17:29:40Z

zerver/models.py

@@ -2367,6 +2367,7 @@ class PreregistrationUser(models.Model):
    full_name_validated = models.BooleanField(default=False)
    referred_by = models.ForeignKey(UserProfile, null=True, on_delete=CASCADE)
    streams = models.ManyToManyField("Stream")
+    subscribe_to_default_streams = models.BooleanField(default=True)


Do we want this to be named something like subscribe_to_default_streams_only?

subscribe_to_exactly_default_streams would be most clear; can you do an "api design" thread about it?

zulipbot · 2023-06-09T12:35:14Z

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

shameondev · 2023-06-09T12:36:36Z

Hey, @sahil839, are there any remaining product questions here or is it just the technical details that need to be discussed?

sahil839 · 2023-06-09T14:36:02Z

I don't think so. I have already updated the PR as per Alya's last feedback on the banner and the behavior to subscribe to default streams at the time of actual account creation and not at the time of invitation was also discussed with both Tim and Alya.

I will add the "QA needed" label though, so that this will be tested someone else too.

sahil839 · 2023-06-12T14:14:38Z

@mateuszmandera Can you do an initial review of this, especially the backend part?

mateuszmandera · 2023-06-12T15:39:22Z

zerver/views/invite.py

+
+        # We would subscribe the invited user to default streams even when the user
+        # inviting them does not have permission to subscribe others.
+        streams = get_default_streams_for_realm(user_profile.realm_id)


Why is this the desired behavior specifically only in the case of not user_profile.can_subscribe_other_users() and stream is empty, but not in the case of user_profile.can_subscribe_other_users() and streams is empty? The "consistent" way would be to do streams = get_default_streams_for_realm(user_profile.realm_id) whenever streams was not provided.

I added this as discussed in this comment. We do not want to subscribe users to streams when stream_ids is passed as empty explicitly. But in case of not user_profile.can_subscribe_other_users we do not allow stream_ids to be passed as non-empty list and in such case we do not show any UI for selecting streams to users and thus want the invited users to be subscribed to default streams.

I guess @mateuszmandera recent work in #29556 also provides a very clear user case for having it be empty.

And as we also add subscribe_to_default_streams parameter, we can probably allow users without permission to subscribe others, to set the parameter as false and not subscribe the new user to default streams. Though as of this PR, we do not allow to user to deselect the subscribe_to_default_streams parameter if the user is not allowed to subscribe others to streams.

mateuszmandera

Looks reasonable to me - didn't look carefully at tests yet though. Posted a couple more comments for now.

mateuszmandera · 2023-06-12T21:47:27Z

zerver/views/invite.py

@@ -54,6 +53,7 @@ def invite_users_backend(
        default=PreregistrationUser.INVITE_AS["MEMBER"],
    ),
    stream_ids: List[int] = REQ(json_validator=check_list(check_int)),
+    subscribe_to_default_streams: bool = REQ(json_validator=check_bool, default=True),


Hmm, on the other hand False might be nicer in the sense that with True default, if you pass non-empty stream_ids, you have to also pass subscribe_to_default_streams or you'll run into the error below due to if len(stream_ids) and subscribe_to_default_streams:, right?

While with the default being False, you can just pass stream_ids without worrying about the other parameter's default getting in the way. Perhaps that's more logical? Not sure... 🤔

mateuszmandera · 2023-06-12T21:48:40Z

zerver/views/invite.py

@@ -74,6 +74,9 @@ def invite_users_backend(

    invitee_emails = get_invitee_emails_set(invitee_emails_raw)

+    if len(stream_ids) and subscribe_to_default_streams:
+        raise JsonableError(_("Invalid parameters"))


"Invalid parameters: stream_ids and subscribe_to_default_streams are mutually exclusive". Something like that perhaps?

zerver/views/registration.py

sahil839 · 2023-06-14T03:47:40Z

Updated the PR. Posted inline comments as well to respond the feedback and mentioning the changes done in brief below.

Updated the default in API to be False as suggested above. Not sure what should be the DB level default.
Updated the error message for passing both stream_ids and subscribe_to_default_streams parameter.
Changed the default value of subscribe_to_default_streams to None in accounts_home in views/registration.py which was previously set to True since the database level default is already True and setting this to None prevents an extra call to .save().

timabbott · 2024-05-14T00:11:21Z

Logic looks good, let's make the migration safe and then I think we should be able to merge this.

sahil839 · 2024-05-14T15:56:04Z

Updated the migration to use batching.

…thers. This commit changes the code to subscribe the invited user to default streams even if the user who invited the new user was not allowed to subscribe others to streams.

We do not show the streams list in invite modal if the user does not have permission to subscribe others to stream and show a notice mentioning it.

alya · 2024-05-14T20:17:17Z

Could you please post an up-to-date screenshot, including the other banners we show at the top now, to make it easy to review how looks all together?

timabbott · 2024-05-14T20:35:52Z

I pushed to update the API feature level. I struggled to figure out why the migration query is what it is in this PR -- The core logic change is this:

-    user_was_invited = prereg_user is not None and (
-        prereg_user.referred_by is not None or prereg_user.multiuse_invite is not None
-    )
-
     if add_initial_stream_subscriptions:
         # If the Preregistration object didn't explicitly list some streams (it
         # happens when user directly signs up without any invitation), we add the
         # default streams for the realm. Note that we are fine with "slim" Stream
         # objects for calling bulk_add_subscriptions and add_new_user_history,
         # which we verify in StreamSetupTest tests that check query counts.
-        if len(streams) == 0 and not user_was_invited:
-            streams = get_slim_realm_default_streams(realm.id)
+        if prereg_user is None or prereg_user.include_realm_default_subscriptions:
+            default_streams = get_slim_realm_default_streams(realm.id)
+            streams = list(set(streams) | set(default_streams))

I feel like there are 3 case:

User was not invited, but has a prereg_user from the signup process; this will have create_preregistration_user create one with the Django default of include_realm_default_subscription=True. But existing objects will need to be migrated, and these have referred_by=None, multiuse_invite=None, streams=None, so that's what your migration targets.
User was not invited, and is being created by the API, LDAP auth, or some other process that does not generate a preregistration user. In that case, they should get the default streams; that doesn't involve the table though, and the logic change is correct.
User was invited, in either form. In that case, the previous logic used that list of streams unconditionally, given the not user_was_invited check above. So the migration should set False, as it does.

OK so I think this is correct ... but let me make that clear in the code comments.

This commit adds include_realm_default_subscriptions parameter to the invite endpoints and the corresponding field in PreregistrationUser and MultiuseInvite objects. This field will be used to subscribe the new users to the default streams at the time of account creation and not to the streams that were default when sending the invite.

This commit adds code to use include_default_realm_subscriptions when user selects "Subscribe to default streams" checkbox in the invite modal instead of passing the stream ids of default streams.

timabbott · 2024-05-14T20:57:06Z

OK, added a very detailed docstring for the migration as part of convincing myself that its logic is correct. I renumbered the API documentation and marked this to merge once CI passes, thanks for all the work on this @sahil839!

I also did some copyediting on the docs; one detail worth highlighting is that we should never just say "the user" in API docs; "the current user" or "the target user" is much clearer.

alya · 2024-05-14T22:40:48Z

Could you please post an up-to-date screenshot, including the other banners we show at the top now, to make it easy to review how looks all together?

I guess I can review post-merge.

sahil839 · 2024-05-15T03:54:25Z

@alya we do not show the new banner along with existing banners because the existing banners are only shown to admins and admins always have permission to subscribe others. Sharing the updated versions of banner in #30097 which makes the new banner spacing similar to existing banners.

alexmv · 2024-05-21T15:40:47Z

zerver/actions/create_user.py

        # If the Preregistration object didn't explicitly list some streams (it
        # happens when user directly signs up without any invitation), we add the
        # default streams for the realm. Note that we are fine with "slim" Stream
        # objects for calling bulk_add_subscriptions and add_new_user_history,
        # which we verify in StreamSetupTest tests that check query counts.


This comment is now a lie. @sahil839, can you update it?

Yes. Opened #30165.

zulipbot added the size: S label May 24, 2023

sahil839 force-pushed the subscribe-others-invite-2 branch from fcf571f to 830c42c Compare June 8, 2023 11:23

zulipbot added size: XL and removed size: S labels Jun 8, 2023

sahil839 force-pushed the subscribe-others-invite-2 branch 3 times, most recently from 7530e85 to 921ab8a Compare June 8, 2023 17:33

sahil839 commented Jun 8, 2023

View reviewed changes

shameondev added area: invitations maintainer review PR is ready for review by Zulip maintainers. labels Jun 9, 2023

sahil839 added the QA needed Manual testing requested. label Jun 9, 2023

mateuszmandera reviewed Jun 12, 2023

View reviewed changes

sahil839 force-pushed the subscribe-others-invite-2 branch 2 times, most recently from d80230d to c72d3be Compare June 13, 2023 10:11

zulipbot added the has conflicts label Jun 13, 2023

sahil839 force-pushed the subscribe-others-invite-2 branch from c72d3be to c6ddce9 Compare June 14, 2023 02:37

zulipbot removed the has conflicts label Jun 14, 2023

sahil839 force-pushed the subscribe-others-invite-2 branch from c6ddce9 to 4f97bb5 Compare June 14, 2023 02:46

zulipbot added the has conflicts label Jun 23, 2023

zulipbot added has conflicts and removed has conflicts labels May 1, 2024

sahil839 force-pushed the subscribe-others-invite-2 branch from 132baea to b881cad Compare May 9, 2024 07:58

zulipbot added has conflicts and removed has conflicts labels May 9, 2024

sahil839 force-pushed the subscribe-others-invite-2 branch from b881cad to bc5db7a Compare May 13, 2024 16:29

zulipbot removed the has conflicts label May 13, 2024

sahil839 force-pushed the subscribe-others-invite-2 branch 3 times, most recently from 8aa7f26 to f5e077c Compare May 14, 2024 07:05

zulipbot added the has conflicts label May 14, 2024

sahil839 added 2 commits May 14, 2024 13:10

invites: Subscribe to default streams when not allowed to subscribe o…

5a60b5e

…thers. This commit changes the code to subscribe the invited user to default streams even if the user who invited the new user was not allowed to subscribe others to streams.

invites: Do not show streams if user cannot subscribe others.

01cb418

We do not show the streams list in invite modal if the user does not have permission to subscribe others to stream and show a notice mentioning it.

timabbott force-pushed the subscribe-others-invite-2 branch from f5e077c to f3f0576 Compare May 14, 2024 20:22

sahil839 added 2 commits May 14, 2024 13:55

invite: Use include_default_realm_subscriptions parameter in webapp.

32f614c

This commit adds code to use include_default_realm_subscriptions when user selects "Subscribe to default streams" checkbox in the invite modal instead of passing the stream ids of default streams.

timabbott force-pushed the subscribe-others-invite-2 branch from f3f0576 to 32f614c Compare May 14, 2024 20:55

timabbott enabled auto-merge (rebase) May 14, 2024 20:56

zulipbot removed the has conflicts label May 14, 2024

timabbott merged commit c41a352 into zulip:main May 14, 2024
14 checks passed

alya mentioned this pull request May 15, 2024

invite: Fix invite modal bug for users who can't subscribe others. #30097

Merged

12 tasks

alexmv reviewed May 21, 2024

View reviewed changes

sahil839 mentioned this pull request May 22, 2024

create_user: Fix comment in set_up_streams_for_new_human_user. #30165

Merged

12 tasks

invites: Do not show streams if user cannot subscribe others. #25739

invites: Do not show streams if user cannot subscribe others. #25739

Conversation

sahil839 commented May 24, 2023

sahil839 commented May 24, 2023

alya commented May 24, 2023

alya commented May 24, 2023

sahil839 commented May 25, 2023

alya commented May 25, 2023

sahil839 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Jun 9, 2023

shameondev commented Jun 9, 2023

sahil839 commented Jun 9, 2023

sahil839 commented Jun 12, 2023

mateuszmandera Jun 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszmandera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil839 commented Jun 14, 2023

timabbott commented May 14, 2024

sahil839 commented May 14, 2024

alya commented May 14, 2024

timabbott commented May 14, 2024

timabbott commented May 14, 2024

alya commented May 14, 2024

sahil839 commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszmandera Jun 12, 2023 •

edited