From 007a51f277f0e00f5e0a89e83cabbdd934cd02fe Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Mon, 24 Apr 2023 20:21:02 +0530 Subject: [PATCH] accounts: Allow user to change email visibility during first login. We now allow users to change email address visibility setting on the "Terms of service" page during first login. This page is not shown for users creating account using normal registration process, but is useful for imported users and users created through API, LDAP, SCIM and management commands. --- templates/zerver/accounts_accept_terms.html | 14 ++++-- web/styles/portico/portico.css | 13 ++++- web/styles/portico/portico_signin.css | 5 ++ zerver/actions/user_settings.py | 2 +- zerver/forms.py | 16 +++++- zerver/tests/test_home.py | 55 +++++++++++++++++++++ zerver/views/home.py | 37 ++++++++++++-- 7 files changed, 132 insertions(+), 10 deletions(-) diff --git a/templates/zerver/accounts_accept_terms.html b/templates/zerver/accounts_accept_terms.html index 24db79e054..93ec655c44 100644 --- a/templates/zerver/accounts_accept_terms.html +++ b/templates/zerver/accounts_accept_terms.html @@ -23,9 +23,10 @@ the registration flow has its own (nearly identical) copy of the fields below in {{ csrf_input }}
-
-

{{ email }}

-
+
{{ email }}
+ {% if first_time_login %} + {% include 'zerver/new_user_email_address_visibility.html' %} + {% endif %}
{% if first_time_terms_of_service_message_template %} @@ -36,6 +37,7 @@ the registration flow has its own (nearly identical) copy of the fields below in {% endif %} + {% if terms_of_service %}
{# This is somewhat subtle. @@ -57,6 +59,7 @@ the registration flow has its own (nearly identical) copy of the fields below in {% endfor %} {% endif %}
+ {% endif %}
@@ -65,4 +68,9 @@ the registration flow has its own (nearly identical) copy of the fields below in
+ +{% if first_time_login %} +{% include 'zerver/change_email_address_visibility_modal.html' %} +{% endif %} + {% endblock %} diff --git a/web/styles/portico/portico.css b/web/styles/portico/portico.css index e5d35c153e..803f5cb986 100644 --- a/web/styles/portico/portico.css +++ b/web/styles/portico/portico.css @@ -934,13 +934,22 @@ input.new-organization-button { } #registration-email { + margin-bottom: 18px; + & label { float: left; - padding-top: 5px; width: 100%; display: block; - margin: 0; + margin-left: 2px; + margin-bottom: 0; text-align: left; + font-weight: 600; + font-size: 1rem; + } + + .controls { + padding-top: 25px; + margin: 2px; } } diff --git a/web/styles/portico/portico_signin.css b/web/styles/portico/portico_signin.css index e41548e908..15c45d3064 100644 --- a/web/styles/portico/portico_signin.css +++ b/web/styles/portico/portico_signin.css @@ -330,6 +330,11 @@ html { margin-bottom: 20px; font-size: 16px; font-weight: 400; + max-width: 400px; + } + + #new-user-email-address-visibility { + max-width: 400px; } .center-block { diff --git a/zerver/actions/user_settings.py b/zerver/actions/user_settings.py index ee1e7dc02a..b4be9b6bd3 100644 --- a/zerver/actions/user_settings.py +++ b/zerver/actions/user_settings.py @@ -250,7 +250,7 @@ def check_change_bot_full_name( @transaction.atomic(durable=True) -def do_change_tos_version(user_profile: UserProfile, tos_version: str) -> None: +def do_change_tos_version(user_profile: UserProfile, tos_version: Optional[str]) -> None: user_profile.tos_version = tos_version user_profile.save(update_fields=["tos_version"]) event_time = timezone_now() diff --git a/zerver/forms.py b/zerver/forms.py index 996711cab9..c9c3ea044a 100644 --- a/zerver/forms.py +++ b/zerver/forms.py @@ -204,7 +204,21 @@ class RegistrationForm(RealmDetailsForm): class ToSForm(forms.Form): - terms = forms.BooleanField(required=True) + terms = forms.BooleanField(required=False) + email_address_visibility = forms.TypedChoiceField( + required=False, + coerce=int, + empty_value=None, + choices=[ + (value, name) + for value, name in UserProfile.EMAIL_ADDRESS_VISIBILITY_ID_TO_NAME_MAP.items() + ], + ) + + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + if settings.TERMS_OF_SERVICE_VERSION is not None: + self.fields["terms"] = forms.BooleanField(required=True) class HomepageForm(forms.Form): diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index f47ede8025..3b0b4577c2 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -558,6 +558,61 @@ class HomeTest(ZulipTestCase): self.assertEqual(result.status_code, 302) self.assertEqual(result["Location"], "/") + user = self.example_user("hamlet") + user.tos_version = "-1" + user.save() + + result = self.client_post("/accounts/accept_terms/") + self.assertEqual(result.status_code, 200) + self.assert_in_response("I agree to the", result) + self.assert_in_response( + "Administrators of this Zulip organization will be able to see this email address.", + result, + ) + + result = self.client_post( + "/accounts/accept_terms/", + { + "terms": True, + "email_address_visibility": UserProfile.EMAIL_ADDRESS_VISIBILITY_MODERATORS, + }, + ) + self.assertEqual(result.status_code, 302) + self.assertEqual(result["Location"], "/") + + user = self.example_user("hamlet") + self.assertEqual( + user.email_address_visibility, UserProfile.EMAIL_ADDRESS_VISIBILITY_MODERATORS + ) + + def test_set_email_address_visibility_without_terms_of_service(self) -> None: + self.login("hamlet") + user = self.example_user("hamlet") + user.tos_version = "-1" + user.save() + + with self.settings(TERMS_OF_SERVICE_VERSION=None): + result = self.client_get("/", dict(stream="Denmark")) + self.assertEqual(result.status_code, 200) + self.assert_in_response( + "Administrators of this Zulip organization will be able to see this email address.", + result, + ) + + result = self.client_post( + "/accounts/accept_terms/", + { + "email_address_visibility": UserProfile.EMAIL_ADDRESS_VISIBILITY_MODERATORS, + }, + ) + self.assertEqual(result.status_code, 302) + self.assertEqual(result["Location"], "/") + + user = self.example_user("hamlet") + self.assertEqual( + user.email_address_visibility, UserProfile.EMAIL_ADDRESS_VISIBILITY_MODERATORS + ) + def test_bad_narrow(self) -> None: self.login("hamlet") with self.assertLogs(level="WARNING") as m: diff --git a/zerver/views/home.py b/zerver/views/home.py index 1a7bf91009..5a131327c5 100644 --- a/zerver/views/home.py +++ b/zerver/views/home.py @@ -7,7 +7,7 @@ from django.http import HttpRequest, HttpResponse from django.shortcuts import redirect, render from django.utils.cache import patch_cache_control -from zerver.actions.user_settings import do_change_tos_version +from zerver.actions.user_settings import do_change_tos_version, do_change_user_setting from zerver.context_processors import get_realm_from_request, get_valid_realm_from_request from zerver.decorator import web_public_view, zulip_login_required from zerver.forms import ToSForm @@ -17,13 +17,16 @@ from zerver.lib.request import RequestNotes from zerver.lib.streams import access_stream_by_name from zerver.lib.subdomains import get_subdomain from zerver.lib.user_counts import realm_user_count -from zerver.models import PreregistrationUser, Realm, Stream, UserProfile +from zerver.models import PreregistrationUser, Realm, RealmUserDefault, Stream, UserProfile def need_accept_tos(user_profile: Optional[UserProfile]) -> bool: if user_profile is None: return False + if user_profile.tos_version == UserProfile.TOS_VERSION_BEFORE_FIRST_LOGIN: + return True + if settings.TERMS_OF_SERVICE_VERSION is None: return False @@ -37,20 +40,48 @@ def accounts_accept_terms(request: HttpRequest) -> HttpResponse: if request.method == "POST": form = ToSForm(request.POST) if form.is_valid(): - assert settings.TERMS_OF_SERVICE_VERSION is not None + assert ( + settings.TERMS_OF_SERVICE_VERSION is not None + or request.user.tos_version == UserProfile.TOS_VERSION_BEFORE_FIRST_LOGIN + ) do_change_tos_version(request.user, settings.TERMS_OF_SERVICE_VERSION) + + email_address_visibility = form.cleaned_data["email_address_visibility"] + if ( + email_address_visibility is not None + and email_address_visibility != request.user.email_address_visibility + ): + do_change_user_setting( + request.user, + "email_address_visibility", + email_address_visibility, + acting_user=request.user, + ) return redirect(home) else: form = ToSForm() + default_email_address_visibility = None + first_time_login = request.user.tos_version == UserProfile.TOS_VERSION_BEFORE_FIRST_LOGIN + if first_time_login: + realm_user_default = RealmUserDefault.objects.get(realm=request.user.realm) + default_email_address_visibility = realm_user_default.email_address_visibility + context = { "form": form, "email": request.user.delivery_email, # Text displayed when updating TERMS_OF_SERVICE_VERSION. "terms_of_service_message": settings.TERMS_OF_SERVICE_MESSAGE, + "terms_of_service_version": settings.TERMS_OF_SERVICE_VERSION, # HTML template used when agreeing to terms of service the # first time, e.g. after data import. "first_time_terms_of_service_message_template": None, + "first_time_login": first_time_login, + "default_email_address_visibility": default_email_address_visibility, + "email_address_visibility_options_dict": UserProfile.EMAIL_ADDRESS_VISIBILITY_ID_TO_NAME_MAP, + "email_address_visibility_admins_only": UserProfile.EMAIL_ADDRESS_VISIBILITY_ADMINS, + "email_address_visibility_moderators": UserProfile.EMAIL_ADDRESS_VISIBILITY_MODERATORS, + "email_address_visibility_nobody": UserProfile.EMAIL_ADDRESS_VISIBILITY_NOBODY, } if (