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

UI option to subscribe existing accounts during bulk invites. #16041

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

Conversation

sumanthvrao
Copy link
Member

@sumanthvrao sumanthvrao commented Aug 5, 2020

Commit structure

  • The first 3 commits are related to backend changes. This buildup to provide the data needed for the actual subscription UI.
  • The next commit bumps up the feature level and adds to the changelog.
  • The last two add and style the UI.

Fixes #15784

Testing Plan:

I tested it manually for the most part. Of course, the backend tests pass as well.

GIFs or Screenshots:

subscribe_to_stream

Feedback/review is much appreciated.

@zulipbot
Copy link
Member

zulipbot commented Aug 5, 2020

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

@sumanthvrao sumanthvrao changed the title WIP: UI option to subscribe existing accounts during bulk invites. UI option to subscribe existing accounts during bulk invites. Aug 6, 2020
@sumanthvrao
Copy link
Member Author

@timabbott This is ready for a review.

We currently don't have tests for invite.js (It seems to be majorly jQuery and DOM interactions). Though, we could probably refactor some parts of invite.js and make it testable.

@timabbott
Copy link
Member

I merged the first two commits; haven't had a chance to read the full branch yet. I don't think we need to add automated tests for this necessarily.

@sumanthvrao sumanthvrao force-pushed the subscribe-existing-accounts-15784 branch from a6fc7db to cc80048 Compare August 7, 2020 00:56
@sumanthvrao
Copy link
Member Author

Rebased 👍

@sumanthvrao sumanthvrao force-pushed the subscribe-existing-accounts-15784 branch from cc80048 to 06a161d Compare August 10, 2020 02:33
@sumanthvrao
Copy link
Member Author

@timabbott FYI, in case you forgot to give this a review :)

… users.

Previously during an error with invite_users_backend codepath, we returned
3 error items - invited email, error message, is_deactivated. However, for
organizations which haven't enabled EMAIL_ADDRESS_VISIBILITY_EVERYONE the
actual user email stored in the frontend is anonymised and different from
the invited email.

To fix this, we return back the anonymous emails for existing accounts along
with the other parameters. This results in 4 return values ->
    - email (invited email)
    - error_msg
    - is_deactivated
    - maybe_anonymous_email

Incase EMAIL_ADDRESS_VISIBILITY_EVERYONE is enabled for the realm,
maybe_anonymous_email would be same as the invited email. This is also
the case for cross_realm_bots. For invalid emails, we just return back
no strings.

Tests amended.
When the email_address_visibility realm setting is switched to
Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE, get_existing_user_errors
returns the maybe_anonymous_email parameter, which in this case
should match with the delivery_email.
This flag helps the frontend to know when to display the option of
subscribing users on invitation. Currently we show the option in two
cases:
   - When no one was invited (Probably all users exist already)
   - When some were invited while some weren't.

We do not show the subcription option when:
   - We fail to send any invites because emails were invalid.
   - Account is too new to send invites.
@sumanthvrao sumanthvrao force-pushed the subscribe-existing-accounts-15784 branch from 06a161d to abe2b89 Compare August 25, 2020 01:21
When inviting existing accounts in the realm, the user is presented
with an option to subscribe existing users to the marked streams.
The error option displays which accounts are being subscribed
to which streams.

The actual stream subscription can result in three cases:
    1. All the users were newly subscribed to the selected streams.
    2. All the users were already part of selected streams.
    3. Some users got newly subscribed, while some others were
       already subscribed.
Appropriate error messages are show.

Fixes zulip#15784.
@sumanthvrao sumanthvrao force-pushed the subscribe-existing-accounts-15784 branch from abe2b89 to 4916ebf Compare August 25, 2020 01:39
@zulipbot
Copy link
Member

Heads up @sumanthvrao, we just merged some commits that conflict with the changes your 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/master branch and resolve your pull request's merge conflicts accordingly.

@timabbott timabbott force-pushed the main branch 2 times, most recently from 4ec3636 to 88b200c Compare August 18, 2023 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide better UI for handling failures on large bulk invitations
3 participants