From 084eaf9e192fbf585c75383404a52d789110c544 Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Sat, 27 Oct 2018 09:55:01 -0400 Subject: [PATCH 1/7] Inherit email field in information about you form We were previously not doing this since the field did not have a description on it. However, I think it's better that the fields match, so inherit away. --- atst/forms/new_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atst/forms/new_request.py b/atst/forms/new_request.py index 8c96ab4a..b293e344 100644 --- a/atst/forms/new_request.py +++ b/atst/forms/new_request.py @@ -167,7 +167,7 @@ class InformationAboutYouForm(ValidatedForm): lname_request = inherit_field(USER_FIELDS["last_name"]) - email_request = EmailField("E-mail Address", validators=[InputRequired(), Email()]) + email_request = inherit_field(USER_FIELDS["email"]) phone_number = inherit_field(USER_FIELDS["phone_number"]) From 2e89f38601ed5ebb09e9ec056d481aca7895a827 Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Sat, 27 Oct 2018 10:08:22 -0400 Subject: [PATCH 2/7] Require all fields on user profile form --- atst/forms/edit_user.py | 12 +++++------- tests/forms/test_edit_user.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 7 deletions(-) create mode 100644 tests/forms/test_edit_user.py diff --git a/atst/forms/edit_user.py b/atst/forms/edit_user.py index 651c3c15..6bdcc3a5 100644 --- a/atst/forms/edit_user.py +++ b/atst/forms/edit_user.py @@ -77,10 +77,8 @@ class EditUserForm(ValidatedForm): first_name = inherit_field(USER_FIELDS["first_name"]) last_name = inherit_field(USER_FIELDS["last_name"]) email = inherit_field(USER_FIELDS["email"]) - phone_number = inherit_field(USER_FIELDS["phone_number"], required=False) - service_branch = inherit_field(USER_FIELDS["service_branch"], required=False) - citizenship = inherit_field(USER_FIELDS["citizenship"], required=False) - designation = inherit_field(USER_FIELDS["designation"], required=False) - date_latest_training = inherit_field( - USER_FIELDS["date_latest_training"], required=False - ) + phone_number = inherit_field(USER_FIELDS["phone_number"]) + service_branch = inherit_field(USER_FIELDS["service_branch"]) + citizenship = inherit_field(USER_FIELDS["citizenship"]) + designation = inherit_field(USER_FIELDS["designation"]) + date_latest_training = inherit_field(USER_FIELDS["date_latest_training"]) diff --git a/tests/forms/test_edit_user.py b/tests/forms/test_edit_user.py new file mode 100644 index 00000000..9c244757 --- /dev/null +++ b/tests/forms/test_edit_user.py @@ -0,0 +1,29 @@ +import pytest +from werkzeug.datastructures import ImmutableMultiDict + +from atst.forms.edit_user import EditUserForm + +from tests.factories import UserFactory + + +def test_edit_user_form_requires_all_fields(): + user = UserFactory.create() + user_data = user.to_dictionary() + del user_data["date_latest_training"] + form_data = ImmutableMultiDict(user_data) + form = EditUserForm(form_data) + assert not form.validate() + assert form.errors == { + 'date_latest_training': ['This field is required.'] + } + + +def test_edit_user_form_valid_with_all_fields(): + user = UserFactory.create() + user_data = user.to_dictionary() + user_data["date_latest_training"] = user_data["date_latest_training"].strftime( + "%m/%d/%Y" + ) + form_data = ImmutableMultiDict(user_data) + form = EditUserForm(form_data) + assert form.validate() From 891dcc5b31a352714135448ca95443d47101382a Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Mon, 29 Oct 2018 13:52:47 -0400 Subject: [PATCH 3/7] Keep list of required fields on user model --- atst/forms/edit_user.py | 28 ++++++++++++++++++---------- atst/forms/new_request.py | 7 ------- atst/models/user.py | 12 ++++++++++++ tests/forms/test_edit_user.py | 4 +--- 4 files changed, 31 insertions(+), 20 deletions(-) diff --git a/atst/forms/edit_user.py b/atst/forms/edit_user.py index 6bdcc3a5..3c337e0b 100644 --- a/atst/forms/edit_user.py +++ b/atst/forms/edit_user.py @@ -2,14 +2,16 @@ import pendulum from copy import deepcopy from wtforms.fields.html5 import DateField, EmailField, TelField from wtforms.fields import RadioField, StringField -from wtforms.validators import Email, Required, Optional +from wtforms.validators import Email, DataRequired, Optional from .fields import SelectField from .forms import ValidatedForm from .data import SERVICE_BRANCHES +from atst.models.user import User from .validators import Name, DateRange, PhoneNumber + USER_FIELDS = { "first_name": StringField("First Name", validators=[Name()]), "last_name": StringField("Last Name", validators=[Name()]), @@ -66,19 +68,25 @@ def inherit_field(unbound_field, required=True): kwargs["validators"] = [] if required: - kwargs["validators"].append(Required()) + kwargs["validators"].append(DataRequired()) else: kwargs["validators"].append(Optional()) return unbound_field.field_class(*unbound_field.args, **kwargs) +def inherit_user_field(field_name): + required = field_name in User.REQUIRED_FIELDS + return inherit_field(USER_FIELDS[field_name], required=required) + + class EditUserForm(ValidatedForm): - first_name = inherit_field(USER_FIELDS["first_name"]) - last_name = inherit_field(USER_FIELDS["last_name"]) - email = inherit_field(USER_FIELDS["email"]) - phone_number = inherit_field(USER_FIELDS["phone_number"]) - service_branch = inherit_field(USER_FIELDS["service_branch"]) - citizenship = inherit_field(USER_FIELDS["citizenship"]) - designation = inherit_field(USER_FIELDS["designation"]) - date_latest_training = inherit_field(USER_FIELDS["date_latest_training"]) + + first_name = inherit_user_field("first_name") + last_name = inherit_user_field("last_name") + email = inherit_user_field("email") + phone_number = inherit_user_field("phone_number") + service_branch = inherit_user_field("service_branch") + citizenship = inherit_user_field("citizenship") + designation = inherit_user_field("designation") + date_latest_training = inherit_user_field("date_latest_training") diff --git a/atst/forms/new_request.py b/atst/forms/new_request.py index b293e344..c1b3e14d 100644 --- a/atst/forms/new_request.py +++ b/atst/forms/new_request.py @@ -164,19 +164,12 @@ class DetailsOfUseForm(ValidatedForm): class InformationAboutYouForm(ValidatedForm): fname_request = inherit_field(USER_FIELDS["first_name"]) - lname_request = inherit_field(USER_FIELDS["last_name"]) - email_request = inherit_field(USER_FIELDS["email"]) - phone_number = inherit_field(USER_FIELDS["phone_number"]) - service_branch = inherit_field(USER_FIELDS["service_branch"]) - citizenship = inherit_field(USER_FIELDS["citizenship"]) - designation = inherit_field(USER_FIELDS["designation"]) - date_latest_training = inherit_field(USER_FIELDS["date_latest_training"]) diff --git a/atst/models/user.py b/atst/models/user.py index bb89c51b..6d1c00ea 100644 --- a/atst/models/user.py +++ b/atst/models/user.py @@ -26,6 +26,18 @@ class User(Base, mixins.TimestampsMixin, mixins.AuditableMixin): designation = Column(String) date_latest_training = Column(Date) + REQUIRED_FIELDS = [ + "email", + "dod_id", + "first_name", + "last_name", + "phone_number", + "service_branch", + "citizenship", + "designation", + "date_latest_training", + ] + @property def atat_permissions(self): return self.atat_role.permissions diff --git a/tests/forms/test_edit_user.py b/tests/forms/test_edit_user.py index 9c244757..2522f264 100644 --- a/tests/forms/test_edit_user.py +++ b/tests/forms/test_edit_user.py @@ -13,9 +13,7 @@ def test_edit_user_form_requires_all_fields(): form_data = ImmutableMultiDict(user_data) form = EditUserForm(form_data) assert not form.validate() - assert form.errors == { - 'date_latest_training': ['This field is required.'] - } + assert form.errors == {"date_latest_training": ["This field is required."]} def test_edit_user_form_valid_with_all_fields(): From f8c4386d842983fcaacee43956211253b492ad07 Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Mon, 29 Oct 2018 16:16:47 -0400 Subject: [PATCH 4/7] Add model attribute for profile completeness --- atst/models/user.py | 9 +++++++++ tests/models/test_user.py | 17 +++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 tests/models/test_user.py diff --git a/atst/models/user.py b/atst/models/user.py index 6d1c00ea..924e3425 100644 --- a/atst/models/user.py +++ b/atst/models/user.py @@ -38,6 +38,15 @@ class User(Base, mixins.TimestampsMixin, mixins.AuditableMixin): "date_latest_training", ] + @property + def profile_complete(self): + return all( + [ + getattr(self, field_name) is not None + for field_name in self.REQUIRED_FIELDS + ] + ) + @property def atat_permissions(self): return self.atat_role.permissions diff --git a/tests/models/test_user.py b/tests/models/test_user.py new file mode 100644 index 00000000..6d5e1d13 --- /dev/null +++ b/tests/models/test_user.py @@ -0,0 +1,17 @@ +import pytest + +from atst.models.user import User + +from tests.factories import UserFactory + + +def test_profile_complete_with_all_info(): + user = UserFactory.create() + assert user.profile_complete + + +@pytest.mark.parametrize("missing_field", User.REQUIRED_FIELDS) +def test_profile_complete_with_missing_info(missing_field): + user = UserFactory.create() + setattr(user, missing_field, None) + assert not user.profile_complete From dd5f99faabba6a28a0a308fe8a80a0c893fb1031 Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Tue, 30 Oct 2018 15:31:56 -0400 Subject: [PATCH 5/7] Force user to fill out their user profile --- atst/domain/auth.py | 16 ++++++++++++++ atst/routes/users.py | 11 +++++++--- templates/user/edit.html | 9 +++++++- tests/routes/test_auth.py | 45 ++++++++++++++++++++++++++++++++++++++ tests/routes/test_users.py | 18 +++++++++++++++ 5 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 tests/routes/test_auth.py diff --git a/atst/domain/auth.py b/atst/domain/auth.py index a60a503a..5ac56fc9 100644 --- a/atst/domain/auth.py +++ b/atst/domain/auth.py @@ -21,10 +21,26 @@ def apply_authentication(app): user = get_current_user() if user: g.current_user = user + if should_redirect_to_user_profile(request, user): + return redirect(url_for("users.user", next=request.path)) elif not _unprotected_route(request): return redirect(url_for("atst.root", next=request.path)) +def should_redirect_to_user_profile(request, user): + has_complete_profile = user.profile_complete + is_unprotected_route = _unprotected_route(request) + is_requesting_user_endpoint = request.endpoint in [ + "users.user", + "users.update_user", + ] + + if has_complete_profile or is_unprotected_route or is_requesting_user_endpoint: + return False + + return True + + def get_current_user(): user_id = session.get("user_id") if user_id: diff --git a/atst/routes/users.py b/atst/routes/users.py index e57584a7..f658b823 100644 --- a/atst/routes/users.py +++ b/atst/routes/users.py @@ -1,4 +1,4 @@ -from flask import Blueprint, render_template, g, request as http_request +from flask import Blueprint, render_template, g, request as http_request, redirect from atst.forms.edit_user import EditUserForm from atst.domain.users import Users @@ -10,16 +10,21 @@ bp = Blueprint("users", __name__) def user(): user = g.current_user form = EditUserForm(data=user.to_dictionary()) - return render_template("user/edit.html", form=form, user=user) + return render_template( + "user/edit.html", next=http_request.args.get("next"), form=form, user=user + ) @bp.route("/user", methods=["POST"]) def update_user(): user = g.current_user form = EditUserForm(http_request.form) - rerender_args = {"form": form, "user": user} + next_url = http_request.args.get("next") + rerender_args = {"form": form, "user": user, "next": next_url} if form.validate(): Users.update(user, form.data) rerender_args["updated"] = True + if next_url: + return redirect(next_url) return render_template("user/edit.html", **rerender_args) diff --git a/templates/user/edit.html b/templates/user/edit.html index 6578c969..bd247a2f 100644 --- a/templates/user/edit.html +++ b/templates/user/edit.html @@ -4,6 +4,13 @@ {% block content %}
+ {% if next is not none %} + {{ Alert('You must complete your profile', + message='

Before continuing, you must complete your profile

', + level='info' + ) }} + {% endif %} + {% if form.errors %} {{ Alert('There were some errors', message="

Please see below.

", @@ -25,7 +32,7 @@
- {% set form_action = url_for('users.update_user') %} + {% set form_action = url_for('users.update_user', next=next) %} {% include "fragments/edit_user_form.html" %} diff --git a/tests/routes/test_auth.py b/tests/routes/test_auth.py new file mode 100644 index 00000000..aef34fbd --- /dev/null +++ b/tests/routes/test_auth.py @@ -0,0 +1,45 @@ +from flask import url_for +from urllib.parse import quote + +from tests.factories import UserFactory + + +def test_request_page_with_complete_profile(client, user_session): + user = UserFactory.create() + user_session(user) + response = client.get("/requests", follow_redirects=False) + assert response.status_code == 200 + + +def test_redirect_when_profile_missing_fields(client, user_session): + user = UserFactory.create(date_latest_training=None) + user_session(user) + requested_url = "/requests" + response = client.get(requested_url, follow_redirects=False) + assert response.status_code == 302 + assert "/user?next={}".format(quote(requested_url, safe="")) in response.location + + +def test_unprotected_route_with_incomplete_profile(client, user_session): + user = UserFactory.create(date_latest_training=None) + user_session(user) + response = client.get("/about", follow_redirects=False) + assert response.status_code == 200 + + +def test_completing_user_profile(client, user_session): + user = UserFactory.create(phone_number=None) + user_session(user) + response = client.get("/requests", follow_redirects=True) + assert b"You must complete your profile" in response.data + + updated_data = {**user.to_dictionary(), "phone_number": "5558675309"} + updated_data["date_latest_training"] = updated_data[ + "date_latest_training" + ].strftime("%m/%d/%Y") + response = client.post(url_for("users.update_user"), data=updated_data) + assert response.status_code == 200 + + response = client.get("/requests", follow_redirects=False) + assert response.status_code == 200 + assert b"You must complete your profile" not in response.data diff --git a/tests/routes/test_users.py b/tests/routes/test_users.py index 74fbdc44..50a723a1 100644 --- a/tests/routes/test_users.py +++ b/tests/routes/test_users.py @@ -23,3 +23,21 @@ def test_user_can_update_profile(user_session, client): updated_user = Users.get_by_dod_id(user.dod_id) assert updated_user.first_name == "chad" assert updated_user.last_name == "vader" + + +def test_user_is_redirected_when_updating_profile(user_session, client): + user = UserFactory.create() + user_session(user) + next_url = "/requests" + + user_data = user.to_dictionary() + user_data["date_latest_training"] = user_data["date_latest_training"].strftime( + "%m/%d/%Y" + ) + response = client.post( + url_for("users.update_user", next=next_url), + data=user_data, + follow_redirects=False, + ) + assert response.status_code == 302 + assert response.location.endswith(next_url) From 90c3c3006414626ea5a93678b5d398f99d8b4630 Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Tue, 30 Oct 2018 15:44:21 -0400 Subject: [PATCH 6/7] Add logout route to "unprotected" routes The logout route doesn't strictly require the user to be logged in and was causing errors with the new profile completeness enforcement. --- atst/domain/auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atst/domain/auth.py b/atst/domain/auth.py index 5ac56fc9..10db8273 100644 --- a/atst/domain/auth.py +++ b/atst/domain/auth.py @@ -7,6 +7,7 @@ UNPROTECTED_ROUTES = [ "atst.root", "dev.login_dev", "atst.login_redirect", + "atst.logout", "atst.unauthorized", "atst.helpdocs", "static", From 4e08f6f3fa0401bc212661d17ecf8b6859becb7d Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Wed, 31 Oct 2018 14:19:27 -0400 Subject: [PATCH 7/7] Formatting fix --- atst/models/user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atst/models/user.py b/atst/models/user.py index d7bdcc28..14d8480f 100644 --- a/atst/models/user.py +++ b/atst/models/user.py @@ -25,7 +25,7 @@ class User(Base, mixins.TimestampsMixin, mixins.AuditableMixin): citizenship = Column(String) designation = Column(String) date_latest_training = Column(Date) - + provisional = Column(Boolean) REQUIRED_FIELDS = [