From ba4b656fb581a0c95c06a2a13b82643e2a9f7b21 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Tue, 4 Jun 2024 17:00:48 +0100 Subject: [PATCH] Add form for users to delete their own accounts --- h/accounts/schemas.py | 15 +++ h/form.py | 9 +- h/routes.py | 2 + h/static/styles/components/_btn.scss | 6 + h/static/styles/components/_form.scss | 16 +-- h/templates/accounts/account.html.jinja2 | 12 +- h/templates/accounts/delete.html.jinja2 | 48 ++++++++ h/templates/accounts/deleted.html.jinja2 | 6 + h/templates/accounts/developer.html.jinja2 | 11 +- h/templates/deform/form.jinja2 | 4 + h/templates/layouts/account.html.jinja2 | 30 ++++- h/views/accounts.py | 85 +++++++++++++ tests/unit/h/accounts/schemas_test.py | 51 ++++++++ tests/unit/h/form_test.py | 27 +++++ tests/unit/h/routes_test.py | 2 + tests/unit/h/views/accounts_test.py | 135 +++++++++++++++++++-- 16 files changed, 426 insertions(+), 33 deletions(-) create mode 100644 h/templates/accounts/delete.html.jinja2 create mode 100644 h/templates/accounts/deleted.html.jinja2 diff --git a/h/accounts/schemas.py b/h/accounts/schemas.py index b64b7ebc64f..8d71774bd40 100644 --- a/h/accounts/schemas.py +++ b/h/accounts/schemas.py @@ -245,6 +245,21 @@ def validator(self, node, value): # pragma: no cover raise exc +class DeleteAccountSchema(CSRFSchema): + password = password_node(title=_("Confirm password")) + + def validator(self, node, value): + super().validator(node, value) + + request = node.bindings["request"] + svc = request.find_service(name="user_password") + + if not svc.check_password(request.user, value.get("password")): + exc = colander.Invalid(node) + exc["password"] = _("Wrong password.") + raise exc + + class NotificationsSchema(CSRFSchema): types = (("reply", _("Email me when someone replies to one of my annotations.")),) diff --git a/h/form.py b/h/form.py index a0b4085f9b3..bd15aa9306c 100644 --- a/h/form.py +++ b/h/form.py @@ -87,7 +87,7 @@ def configure_environment(config): # pragma: no cover config.registry[ENVIRONMENT_KEY] = create_environment(base) -def handle_form_submission(request, form, on_success, on_failure): +def handle_form_submission(request, form, on_success, on_failure, flash_success=True): """ Handle the submission of the given form in a standard way. @@ -114,6 +114,11 @@ def handle_form_submission(request, form, on_success, on_failure): not an XHR request. :type on_failure: callable + :param flash_success: + Whether to show a "success" flash message if handling the form succeeds. + Applies to non-XHR form submissions only. + :type flash_success: bool + """ try: appstruct = form.validate(request.POST.items()) @@ -126,7 +131,7 @@ def handle_form_submission(request, form, on_success, on_failure): if result is None: result = httpexceptions.HTTPFound(location=request.url) - if not request.is_xhr: # pragma: no cover + if (not request.is_xhr) and flash_success: request.session.flash(_("Success. We've saved your changes."), "success") return to_xhr_response(request, result, form) diff --git a/h/routes.py b/h/routes.py index a448f02c95e..36b3b13492d 100644 --- a/h/routes.py +++ b/h/routes.py @@ -16,6 +16,8 @@ def includeme(config): # pylint: disable=too-many-statements config.add_route("account_profile", "/account/profile") config.add_route("account_notifications", "/account/settings/notifications") config.add_route("account_developer", "/account/developer") + config.add_route("account_delete", "/account/delete") + config.add_route("account_deleted", "/account/deleted") config.add_route("claim_account_legacy", "/claim_account/{token}") config.add_route("dismiss_sidebar_tutorial", "/app/dismiss_sidebar_tutorial") diff --git a/h/static/styles/components/_btn.scss b/h/static/styles/components/_btn.scss index b97c584bde9..36767a83cdd 100644 --- a/h/static/styles/components/_btn.scss +++ b/h/static/styles/components/_btn.scss @@ -10,6 +10,12 @@ min-height: 30px; + /* Vertically center text within buttons. + This works even if the element is not a button, + e.g. */ + display: flex; + align-items: center; + background-color: color.$grey-6; border: none; border-radius: 2px; diff --git a/h/static/styles/components/_form.scss b/h/static/styles/components/_form.scss index 5b8880d4434..f2271d218a0 100644 --- a/h/static/styles/components/_form.scss +++ b/h/static/styles/components/_form.scss @@ -126,14 +126,6 @@ padding-right: 15px; } -// A form footer with no top border, useful for additional sections at the -// bottom of a footer. -.form-footer--no-border { - @include form-footer; - - margin-top: 15px; -} - .form-help-text { color: color.$grey-5; } @@ -147,3 +139,11 @@ color: color.$brand; margin-right: 3px; } + +.form-footer__left { + float: left; +} + +.form-footer__right { + float: right; +} diff --git a/h/templates/accounts/account.html.jinja2 b/h/templates/accounts/account.html.jinja2 index 826024a764d..6ce1890a7ca 100644 --- a/h/templates/accounts/account.html.jinja2 +++ b/h/templates/accounts/account.html.jinja2 @@ -7,13 +7,9 @@
{{ email_form }} {{ password_form }} - -
{% endblock page_content %} + +{% block form_footer_right %} +{% trans %}Delete your account{% endtrans %} +{% endblock %} diff --git a/h/templates/accounts/delete.html.jinja2 b/h/templates/accounts/delete.html.jinja2 new file mode 100644 index 00000000000..4fc7b826d72 --- /dev/null +++ b/h/templates/accounts/delete.html.jinja2 @@ -0,0 +1,48 @@ +{% extends "h:templates/layouts/account.html.jinja2" %} + +{% set page_route = 'account_delete' %} +{% set page_title = 'Delete your account' %} + +{% block tabs %}{% endblock tabs %} + +{% block page_content %} +

{% trans %}Delete your account{% endtrans %}

+ +
+

{% trans %}Are you sure you want to delete your account?{% endtrans %}

+ +

+ {% set username = request.user.username %} + + {% if count == 0 %} + {% trans %}This will delete user {{ username }}.{% endtrans %} + {% else %} + {% set oldest_str = oldest.strftime("%B %-d %Y") %} + {% set newest_str = newest.strftime("%B %-d %Y") %} + + {% if oldest_str == newest_str %} + {% trans count=count %} + This will delete user {{ username }}, + including 1 annotation + from {{ oldest_str }}. + {% pluralize count %} + This will delete user {{ username }}, + including {{ count }} annotations + from {{ oldest_str }}. + {% endtrans %} + {% else %} + {% trans %} + This will delete user {{ username }}, + including {{ count }} annotations + spanning {{ oldest_str }} + to {{ newest_str }}. + {% endtrans %} + {% endif %} + {% endif %} +

+ +

{% trans %}This cannot be undone!{% endtrans %}

+
+ + {{ form }} +{% endblock page_content %} diff --git a/h/templates/accounts/deleted.html.jinja2 b/h/templates/accounts/deleted.html.jinja2 new file mode 100644 index 00000000000..812879d78ec --- /dev/null +++ b/h/templates/accounts/deleted.html.jinja2 @@ -0,0 +1,6 @@ +{% extends "h:templates/accounts/base.html.jinja2" %} + +{% block page_title %}{% trans %}Account deleted{% endtrans %}{% endblock %} + +{% set form_message %}{% trans %}Your account has been deleted.{% endtrans %}{% endset %} +{% set signup_message = "Don't have a Hypothesis account?" %} diff --git a/h/templates/accounts/developer.html.jinja2 b/h/templates/accounts/developer.html.jinja2 index eca19b242dc..761a3c7edc3 100644 --- a/h/templates/accounts/developer.html.jinja2 +++ b/h/templates/accounts/developer.html.jinja2 @@ -31,9 +31,12 @@ {% endif %} - {% endblock page_content %} + +{% block form_footer_left %} +

+ You can learn more about the Hypothesis API at + h.readthedocs.io/en/latest/api.html +

+{% endblock %} diff --git a/h/templates/deform/form.jinja2 b/h/templates/deform/form.jinja2 index f4c12ed9eb0..5288b2c4f73 100644 --- a/h/templates/deform/form.jinja2 +++ b/h/templates/deform/form.jinja2 @@ -35,6 +35,10 @@ {% endif %} + {% if field.back_link %} + {{ field.back_link.text }} + {% endif %}
{%- for button in field.buttons -%} diff --git a/h/templates/layouts/account.html.jinja2 b/h/templates/layouts/account.html.jinja2 index 8bc6afece54..4685a178937 100644 --- a/h/templates/layouts/account.html.jinja2 +++ b/h/templates/layouts/account.html.jinja2 @@ -17,6 +17,7 @@ {% block content %}
+ {% block tabs %} + {% endblock tabs %} {% include "h:templates/includes/flash-messages.html.jinja2" %} {{ self.page_content() }} -
- {% include "h:templates/includes/back_link.html.jinja2" %} -
+ {% with %} + {% set footer_left %} + {% block form_footer_left %}{% endblock form_footer_left %} + {% include "h:templates/includes/back_link.html.jinja2" %} + {% endset %} + + {% set footer_right %} + {% block form_footer_right %}{% endblock form_footer_right %} + {% endset %} + + {% if footer_left|trim or footer_right|trim %} + + {% endif %} + {% endwith %}
{% endblock content %} diff --git a/h/views/accounts.py b/h/views/accounts.py index b368c3c5463..b4ead806a46 100644 --- a/h/views/accounts.py +++ b/h/views/accounts.py @@ -8,6 +8,7 @@ from pyramid import httpexceptions, security from pyramid.exceptions import BadCSRFToken from pyramid.view import view_config, view_defaults +from sqlalchemy import func, select from h import accounts, form, i18n, models, session from h.accounts import schemas @@ -18,6 +19,7 @@ PasswordResetEvent, ) from h.emails import reset_password +from h.models import Annotation from h.schemas.forms.accounts import ( EditProfileSchema, ForgotPasswordSchema, @@ -608,6 +610,80 @@ def post(self): return {"token": token.value} +@view_defaults( + route_name="account_delete", + renderer="h:templates/accounts/delete.html.jinja2", + is_authenticated=True, +) +class DeleteController: + def __init__(self, request): + self.request = request + + schema = schemas.DeleteAccountSchema().bind(request=self.request) + + self.form = self.request.create_form( + schema, + buttons=(deform.Button(_("Delete your account"), css_class="btn--danger"),), + formid="delete", + back_link={ + "href": self.request.route_url("account"), + "text": _("Back to safety"), + }, + ) + + @view_config(request_method="GET") + def get(self): + return self.template_data() + + @view_config(request_method="POST") + def post(self): + return form.handle_form_submission( + self.request, + self.form, + on_success=self.delete_user, + on_failure=self.template_data, + flash_success=False, + ) + + def delete_user(self, _appstruct): + self.request.find_service(name="user_delete").delete_user( + self.request.user, + requested_by=self.request.user, + tag=self.request.matched_route.name, + ) + + return httpexceptions.HTTPFound( + location=self.request.route_url("account_deleted") + ) + + def template_data(self): + def query(column): + return ( + select(column) + .where(Annotation.deleted.is_(False)) + .where(Annotation.userid == self.request.authenticated_userid) + ) + + count = self.request.db.scalar( + query(func.count(Annotation.id)) # pylint:disable=not-callable + ) + + oldest = self.request.db.scalar( + query(Annotation.created).order_by(Annotation.created) + ) + + newest = self.request.db.scalar( + query(Annotation.created).order_by(Annotation.created.desc()) + ) + + return { + "count": count, + "oldest": oldest, + "newest": newest, + "form": self.form.render(), + } + + # TODO: This can be removed after October 2016, which will be >1 year from the # date that the last account claim emails were sent out. At this point, # if we have not done so already, we should remove all unclaimed @@ -631,3 +707,12 @@ def dismiss_sidebar_tutorial(request): # pragma: no cover request.user.sidebar_tutorial_dismissed = True return ajax_payload(request, {"status": "okay"}) + + +@view_config( + route_name="account_deleted", + request_method="GET", + renderer="h:templates/accounts/deleted.html.jinja2", +) +def account_deleted(_request): + return {} diff --git a/tests/unit/h/accounts/schemas_test.py b/tests/unit/h/accounts/schemas_test.py index 119a24a73dc..5f6ad9aef3b 100644 --- a/tests/unit/h/accounts/schemas_test.py +++ b/tests/unit/h/accounts/schemas_test.py @@ -325,6 +325,57 @@ def test_it_is_invalid_if_current_password_is_wrong( assert "password" in exc.value.asdict() +@pytest.mark.usefixtures("user_password_service") +class TestDeleteAccountSchema: + def test_it(self, schema, user, user_password_service): + user_password_service.check_password.return_value = True + + schema.deserialize({"password": "test_password"}) + + user_password_service.check_password.assert_called_once_with( + user, "test_password" + ) + + @pytest.mark.parametrize( + "password,expected_error_dict", + [ + ("wrong_password", {"password": "Wrong password."}), + ("", {"password": "Required"}), + (None, {"password": "Required"}), + ], + ) + def test_it_when_the_password_is_wrong( + self, schema, user_password_service, password, expected_error_dict + ): + user_password_service.check_password.return_value = False + + with pytest.raises(colander.Invalid) as exc_info: + schema.deserialize({"password": password}) + + assert exc_info.value.asdict() == expected_error_dict + + def test_it_when_the_password_is_missing(self, schema, user_password_service): + user_password_service.check_password.return_value = False + + with pytest.raises(colander.Invalid) as exc_info: + schema.deserialize({}) + + assert exc_info.value.asdict() == {"password": "Required"} + + @pytest.fixture + def user(self, factories): + return factories.User() + + @pytest.fixture + def pyramid_csrf_request(self, pyramid_csrf_request, user): + pyramid_csrf_request.user = user + return pyramid_csrf_request + + @pytest.fixture + def schema(self, pyramid_csrf_request): + return schemas.DeleteAccountSchema().bind(request=pyramid_csrf_request) + + @pytest.fixture def dummy_node(pyramid_request): class DummyNode: diff --git a/tests/unit/h/form_test.py b/tests/unit/h/form_test.py index 1186854179b..164d71778ac 100644 --- a/tests/unit/h/form_test.py +++ b/tests/unit/h/form_test.py @@ -230,6 +230,33 @@ def test_if_validation_succeeds_it_shows_a_flash_message( assert pyramid_request.session.peek_flash("success") + def test_it_doesnt_show_a_flash_message_for_XHR_requests( + self, form_validating_to, pyramid_request + ): + pyramid_request.is_xhr = True + + form.handle_form_submission( + pyramid_request, + form_validating_to("anything"), + mock_callable(), + mock.sentinel.on_failure, + ) + + assert not pyramid_request.session.peek_flash("success") + + def test_it_doesnt_show_a_flash_message_if_asked_not_to( + self, form_validating_to, pyramid_request + ): + form.handle_form_submission( + pyramid_request, + form_validating_to("anything"), + mock_callable(), + mock.sentinel.on_failure, + flash_success=False, + ) + + assert not pyramid_request.session.peek_flash("success") + def test_if_validation_succeeds_it_calls_to_xhr_response( self, form_validating_to, matchers, pyramid_request, to_xhr_response ): diff --git a/tests/unit/h/routes_test.py b/tests/unit/h/routes_test.py index ee5b38a959a..a8fd99f58e8 100644 --- a/tests/unit/h/routes_test.py +++ b/tests/unit/h/routes_test.py @@ -27,6 +27,8 @@ def test_includeme(): call("account_profile", "/account/profile"), call("account_notifications", "/account/settings/notifications"), call("account_developer", "/account/developer"), + call("account_delete", "/account/delete"), + call("account_deleted", "/account/deleted"), call("claim_account_legacy", "/claim_account/{token}"), call("dismiss_sidebar_tutorial", "/app/dismiss_sidebar_tutorial"), call("activity.search", "/search"), diff --git a/tests/unit/h/views/accounts_test.py b/tests/unit/h/views/accounts_test.py index d7d5971631b..64dc7151412 100644 --- a/tests/unit/h/views/accounts_test.py +++ b/tests/unit/h/views/accounts_test.py @@ -1,12 +1,14 @@ +# pylint:disable=too-many-lines +from datetime import datetime, timedelta from unittest import mock import colander +import deform import pytest from h_matchers import Any from pyramid import httpexceptions from h.models import Subscriptions -from h.services.user_password import UserPasswordService from h.views import accounts as views @@ -881,6 +883,125 @@ def authenticated_userid(self, pyramid_config): return userid +class TestDeleteController: + def test_get( + self, authenticated_user, controller, factories, pyramid_request, schemas + ): + oldest_annotation = factories.Annotation( + userid=authenticated_user.userid, created=datetime(1970, 1, 1) + ) + newest_annotation = factories.Annotation( + userid=authenticated_user.userid, created=datetime(1990, 1, 1) + ) + # An annotation by another user. This shouldn't be counted. + factories.Annotation(created=oldest_annotation.created - timedelta(days=1)) + # A deleted annotation. This shouldn't be counted. + factories.Annotation( + userid=authenticated_user.userid, + deleted=True, + created=newest_annotation.created + timedelta(days=1), + ) + + template_vars = controller.get() + + schemas.DeleteAccountSchema.assert_called_once_with() + schemas.DeleteAccountSchema.return_value.bind.assert_called_with( + request=pyramid_request + ) + pyramid_request.create_form.assert_called_once_with( + schemas.DeleteAccountSchema.return_value.bind.return_value, + buttons=Any.iterable.comprised_of(Any.instance_of(deform.Button)), + formid="delete", + back_link={ + "href": "http://example.com/account/settings", + "text": Any.string(), + }, + ) + pyramid_request.create_form.return_value.render.assert_called_once_with() + + assert template_vars == { + "count": 2, + "oldest": oldest_annotation.created, + "newest": newest_annotation.created, + "form": pyramid_request.create_form.return_value.render.return_value, + } + + def test_get_when_user_has_no_annotations(self, controller, pyramid_request): + template_vars = controller.get() + + assert template_vars == { + "count": 0, + "oldest": None, + "newest": None, + "form": pyramid_request.create_form.return_value.render.return_value, + } + + def test_post(self, controller, form, pyramid_request, schemas): + result = controller.post() + + schemas.DeleteAccountSchema.assert_called_once_with() + schemas.DeleteAccountSchema.return_value.bind.assert_called_with( + request=pyramid_request + ) + pyramid_request.create_form.assert_called_once_with( + schemas.DeleteAccountSchema.return_value.bind.return_value, + buttons=Any.iterable.comprised_of(Any.instance_of(deform.Button)), + formid="delete", + back_link={ + "href": "http://example.com/account/settings", + "text": Any.string(), + }, + ) + form.handle_form_submission.assert_called_once_with( + pyramid_request, + pyramid_request.create_form.return_value, + on_success=controller.delete_user, + on_failure=controller.template_data, + flash_success=False, + ) + assert result == form.handle_form_submission.return_value + + def test_delete_user( + self, authenticated_user, controller, pyramid_request, user_delete_service + ): + response = controller.delete_user(mock.sentinel.appstruct) + + user_delete_service.delete_user.assert_called_once_with( + authenticated_user, + requested_by=authenticated_user, + tag=pyramid_request.matched_route.name, + ) + assert response == Any.instance_of(httpexceptions.HTTPFound).with_attrs( + {"location": "http://example.com/account/deleted"} + ) + + @pytest.fixture(autouse=True) + def authenticated_user(self, factories, pyramid_config, pyramid_request): + authenticated_user = factories.User.build() + pyramid_config.testing_securitypolicy(authenticated_user.userid) + pyramid_request.user = authenticated_user + return authenticated_user + + @pytest.fixture + def controller(self, pyramid_request): + return views.DeleteController(pyramid_request) + + @pytest.fixture(autouse=True) + def routes(self, pyramid_config): + pyramid_config.add_route("account", "/account/settings") + pyramid_config.add_route("account_delete", "/account/delete") + pyramid_config.add_route("account_deleted", "/account/deleted") + + @pytest.fixture(autouse=True) + def form(self, patch): + return patch("h.views.accounts.form") + + +def test_account_deleted(pyramid_request): + # pylint:disable=use-implicit-booleaness-not-comparison + assert views.account_deleted(pyramid_request) == {} + + @pytest.fixture def user_model(patch): return patch("h.models.User") @@ -901,13 +1022,11 @@ def mailer(patch): return patch("h.views.accounts.mailer") -@pytest.fixture -def user_password_service(pyramid_config): - service = mock.Mock(spec_set=UserPasswordService()) - pyramid_config.register_service(service, name="user_password") - return service - - @pytest.fixture(autouse=True) def LoginSchema(patch): return patch("h.views.accounts.LoginSchema") + + +@pytest.fixture(autouse=True) +def schemas(patch): + return patch("h.views.accounts.schemas")