From 005799b26fe92c82f229bc1cae84331840ccc29c Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Tue, 27 Aug 2024 16:07:21 +0000 Subject: [PATCH 1/7] feat(model): add sso_domain field to TransitAgency --- .../0024_transitagency_sso_domain_and_more.py | 37 +++++++++++++++++++ benefits/core/models.py | 8 +++- 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 benefits/core/migrations/0024_transitagency_sso_domain_and_more.py diff --git a/benefits/core/migrations/0024_transitagency_sso_domain_and_more.py b/benefits/core/migrations/0024_transitagency_sso_domain_and_more.py new file mode 100644 index 000000000..8d827d55a --- /dev/null +++ b/benefits/core/migrations/0024_transitagency_sso_domain_and_more.py @@ -0,0 +1,37 @@ +# Generated by Django 5.0.7 on 2024-08-27 16:02 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("auth", "0012_alter_user_first_name_max_length"), + ("core", "0023_transitagency_staff_group"), + ] + + operations = [ + migrations.AddField( + model_name="transitagency", + name="sso_domain", + field=models.TextField( + blank=True, + default="", + help_text="The email domain of users to automatically add to this agency's staff group upon login.", + null=True, + ), + ), + migrations.AlterField( + model_name="transitagency", + name="staff_group", + field=models.OneToOneField( + blank=True, + default=None, + help_text="The group of users associated with this TransitAgency.", + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="auth.group", + ), + ), + ] diff --git a/benefits/core/models.py b/benefits/core/models.py index 68ab1927f..ba0b98d93 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -347,7 +347,13 @@ class TransitAgency(models.Model): null=True, blank=True, default=None, - help_text="The group of users who can access this TransitAgency.", + help_text="The group of users associated with this TransitAgency.", + ) + sso_domain = models.TextField( + null=True, + blank=True, + default="", + help_text="The email domain of users to automatically add to this agency's staff group upon login.", ) def __str__(self): From 58f00be2048ed9ee21843bea03bfdda275deb0f0 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Tue, 27 Aug 2024 16:28:46 +0000 Subject: [PATCH 2/7] feat: automatically associate user with TransitAgency using SSO domain this lessens the chance that the user will reach the in_person views without an associated TransitAgency. --- benefits/core/admin.py | 9 +++++++++ tests/pytest/core/test_admin.py | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/benefits/core/admin.py b/benefits/core/admin.py index 2c106b3f6..f7abc1edc 100644 --- a/benefits/core/admin.py +++ b/benefits/core/admin.py @@ -124,6 +124,7 @@ def pre_login_user(user, request): logger.debug(f"Running pre-login callback for user: {user.username}") add_google_sso_userinfo(user, request) add_staff_user_to_group(user, request) + add_transit_agency_staff_user_to_group(user, request) def add_google_sso_userinfo(user, request): @@ -151,3 +152,11 @@ def add_staff_user_to_group(user, request): if user.email in settings.GOOGLE_SSO_STAFF_LIST: staff_group = Group.objects.get(name=settings.STAFF_GROUP_NAME) user.groups.add(staff_group) + + +def add_transit_agency_staff_user_to_group(user, request): + user_sso_domain = user.email.split("@")[1] + if user_sso_domain: + agency = models.TransitAgency.objects.filter(sso_domain=user_sso_domain).first() + if agency is not None and agency.staff_group: + user.groups.add(agency.staff_group) diff --git a/tests/pytest/core/test_admin.py b/tests/pytest/core/test_admin.py index b1657f88a..ace22e490 100644 --- a/tests/pytest/core/test_admin.py +++ b/tests/pytest/core/test_admin.py @@ -1,6 +1,7 @@ import pytest from django.conf import settings from django.contrib.auth.models import User, Group + import benefits.core.admin from benefits.core.admin import GOOGLE_USER_INFO_URL, pre_login_user @@ -94,3 +95,27 @@ def test_pre_login_user_does_not_add_transit_staff_to_group(mocker, settings): staff_group = Group.objects.get(name=settings.STAFF_GROUP_NAME) assert staff_group.user_set.count() == 0 assert agency_user.groups.count() == 0 + + +@pytest.mark.django_db +def test_pre_login_user_add_transit_staff_to_transit_staff_group(mocker, settings, model_TransitAgency): + mocked_request = mocker.Mock() + mocked_request.session.get.return_value = None + + transit_agency_staff_group = Group.objects.create(name="CST Staff") + model_TransitAgency.pk = None + model_TransitAgency.staff_group = transit_agency_staff_group + model_TransitAgency.sso_domain = "cst.org" + model_TransitAgency.save() + + settings.GOOGLE_SSO_STAFF_LIST = ["*"] + settings.GOOGLE_SSO_ALLOWABLE_DOMAINS = ["cst.org"] + + # simulate what `django_google_sso` does for us (sets is_staff to True) + agency_user = User.objects.create_user(username="agency_user", email="manager@cst.org", is_staff=True) + + pre_login_user(agency_user, mocked_request) + + # assert that a transit agency user gets added to their TransitAgency's staff group based on SSO domain + assert agency_user.groups.count() == 1 + assert agency_user.groups.first() == transit_agency_staff_group From b6d61b8c54fcfc3519086d734feff9574ace9536 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Tue, 27 Aug 2024 19:27:05 +0000 Subject: [PATCH 3/7] feat: separate concepts of association to a TransitAgency and permission specifically, permission to do in-person enrollment. now there are two separate groups representing those concepts instead of one. had to specify a `related_name` to avoid name collision when Django follows relationships "backwards": https://docs.djangoproject.com/en/5.1/ref/models/fields/#django.db.models.ForeignKey.related_name https://docs.djangoproject.com/en/5.1/topics/db/queries/#following-relationships-backward refactored `TransitAgency.for_user` method to loop over user's groups. --- ...nsitagency_sso_domain_customer_service.py} | 14 +++++++++++++ benefits/core/models.py | 20 ++++++++++++++----- 2 files changed, 29 insertions(+), 5 deletions(-) rename benefits/core/migrations/{0024_transitagency_sso_domain_and_more.py => 0024_transitagency_sso_domain_customer_service.py} (66%) diff --git a/benefits/core/migrations/0024_transitagency_sso_domain_and_more.py b/benefits/core/migrations/0024_transitagency_sso_domain_customer_service.py similarity index 66% rename from benefits/core/migrations/0024_transitagency_sso_domain_and_more.py rename to benefits/core/migrations/0024_transitagency_sso_domain_customer_service.py index 8d827d55a..955777ac5 100644 --- a/benefits/core/migrations/0024_transitagency_sso_domain_and_more.py +++ b/benefits/core/migrations/0024_transitagency_sso_domain_customer_service.py @@ -31,6 +31,20 @@ class Migration(migrations.Migration): help_text="The group of users associated with this TransitAgency.", null=True, on_delete=django.db.models.deletion.PROTECT, + related_name="transit_agency", + to="auth.group", + ), + ), + migrations.AddField( + model_name="transitagency", + name="customer_service_group", + field=models.OneToOneField( + blank=True, + default=None, + help_text="The group of users who are allowed to do in-person eligibility verification and enrollment.", + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="+", to="auth.group", ), ), diff --git a/benefits/core/models.py b/benefits/core/models.py index ba0b98d93..480a1c8b9 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -348,6 +348,7 @@ class TransitAgency(models.Model): blank=True, default=None, help_text="The group of users associated with this TransitAgency.", + related_name="transit_agency", ) sso_domain = models.TextField( null=True, @@ -355,6 +356,15 @@ class TransitAgency(models.Model): default="", help_text="The email domain of users to automatically add to this agency's staff group upon login.", ) + customer_service_group = models.OneToOneField( + Group, + on_delete=models.PROTECT, + null=True, + blank=True, + default=None, + help_text="The group of users who are allowed to do in-person eligibility verification and enrollment.", + related_name="+", + ) def __str__(self): return self.long_name @@ -403,9 +413,9 @@ def all_active(): @staticmethod def for_user(user: User): - group = user.groups.first() + for group in user.groups.all(): + if hasattr(group, "transit_agency"): + return group.transit_agency # this is looking at the TransitAgency's staff_group - if group is not None and hasattr(group, "transitagency"): - return group.transitagency - else: - return None + # the loop above returns the first match found. Return None if no match was found. + return None From 00c0de34f840703c7911bd7e715697db85ea3ae6 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Tue, 27 Aug 2024 20:37:50 +0000 Subject: [PATCH 4/7] feat: check customer service group when rendering agency dashboard --- benefits/admin.py | 5 +++++ benefits/templates/admin/agency-dashboard-index.html | 2 ++ 2 files changed, 7 insertions(+) diff --git a/benefits/admin.py b/benefits/admin.py index 36df7e2f1..c6c06ee7e 100644 --- a/benefits/admin.py +++ b/benefits/admin.py @@ -33,4 +33,9 @@ def index(self, request, extra_context=None): else: agency = TransitAgency.for_user(request.user) session.update(request, agency=agency) + + if agency is not None: + has_permission_for_in_person = agency.customer_service_group in request.user.groups.all() + context.update({"has_permission_for_in_person": has_permission_for_in_person}) + return TemplateResponse(request, "admin/agency-dashboard-index.html", context) diff --git a/benefits/templates/admin/agency-dashboard-index.html b/benefits/templates/admin/agency-dashboard-index.html index a762cb766..303ba3ffb 100644 --- a/benefits/templates/admin/agency-dashboard-index.html +++ b/benefits/templates/admin/agency-dashboard-index.html @@ -6,6 +6,7 @@ {% endblock title %} {% block content %} + {% if has_permission_for_in_person %}

{{ agency.long_name }}

@@ -21,5 +22,6 @@

In-person enrollment

+ {% endif %} {% endblock content %} From 326d31cdee7d0359b6521a88d0beeda2c101957f Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Tue, 27 Aug 2024 21:05:36 +0000 Subject: [PATCH 5/7] test: update tests to cover expected context data --- tests/pytest/test_admin.py | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/pytest/test_admin.py b/tests/pytest/test_admin.py index e931b8622..1e2939863 100644 --- a/tests/pytest/test_admin.py +++ b/tests/pytest/test_admin.py @@ -32,12 +32,40 @@ def model_SuperUser(model_User): @pytest.mark.django_db -def test_admin_override_agency_user(app_request, model_AgencyUser): +def test_admin_override_agency_user_customer_service(app_request, model_AgencyUser, model_TransitAgency): app_request.user = model_AgencyUser + + # set up TransitAgency with staff_group and customer_service_group + model_TransitAgency.pk = None + model_TransitAgency.staff_group = Group.objects.get(name="CST") + customer_service_group = Group.objects.create(name="CST Customer Service") + model_TransitAgency.customer_service_group = customer_service_group + model_TransitAgency.save() + + # add the user to the customer service group + model_AgencyUser.groups.add(customer_service_group) + + response = BenefitsAdminSite().index(app_request) + + assert response.status_code == 200 + assert response.template_name == "admin/agency-dashboard-index.html" + assert response.context_data["has_permission_for_in_person"] is True + + +@pytest.mark.django_db +def test_admin_override_agency_user_not_customer_service(app_request, model_AgencyUser, model_TransitAgency): + app_request.user = model_AgencyUser + + # set up TransitAgency with only a staff_group + model_TransitAgency.pk = None + model_TransitAgency.staff_group = Group.objects.get(name="CST") + model_TransitAgency.save() + response = BenefitsAdminSite().index(app_request) assert response.status_code == 200 assert response.template_name == "admin/agency-dashboard-index.html" + assert response.context_data["has_permission_for_in_person"] is False @pytest.mark.django_db From b7ed9f586dccda4e8e5766eb79958bac1ca0f091 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Tue, 27 Aug 2024 21:52:50 +0000 Subject: [PATCH 6/7] fix: show transit agency's name --- .../admin/agency-dashboard-index.html | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/benefits/templates/admin/agency-dashboard-index.html b/benefits/templates/admin/agency-dashboard-index.html index 303ba3ffb..b4ecbc35d 100644 --- a/benefits/templates/admin/agency-dashboard-index.html +++ b/benefits/templates/admin/agency-dashboard-index.html @@ -6,22 +6,22 @@ {% endblock title %} {% block content %} - {% if has_permission_for_in_person %}

{{ agency.long_name }}

-
-

In-person enrollment

-

Verify transit benefit eligibility using agency criteria and complete a rider’s open-loop card enrollment.

- {% url routes.IN_PERSON_ELIGIBILITY as url_eligibility %} -
-
- New enrollment + {% if has_permission_for_in_person %} +
+

In-person enrollment

+

Verify transit benefit eligibility using agency criteria and complete a rider’s open-loop card enrollment.

+ {% url routes.IN_PERSON_ELIGIBILITY as url_eligibility %} +
+
+ New enrollment +
-
+ {% endif %}
- {% endif %} {% endblock content %} From 04d3c194d80ae4be4afffd92fefeef986410e93a Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Wed, 28 Aug 2024 18:52:32 +0000 Subject: [PATCH 7/7] refactor: add user to group instead of group to user no functional change, just improving readability --- benefits/core/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/benefits/core/admin.py b/benefits/core/admin.py index f7abc1edc..a21f8128c 100644 --- a/benefits/core/admin.py +++ b/benefits/core/admin.py @@ -151,7 +151,7 @@ def add_google_sso_userinfo(user, request): def add_staff_user_to_group(user, request): if user.email in settings.GOOGLE_SSO_STAFF_LIST: staff_group = Group.objects.get(name=settings.STAFF_GROUP_NAME) - user.groups.add(staff_group) + staff_group.user_set.add(user) def add_transit_agency_staff_user_to_group(user, request): @@ -159,4 +159,4 @@ def add_transit_agency_staff_user_to_group(user, request): if user_sso_domain: agency = models.TransitAgency.objects.filter(sso_domain=user_sso_domain).first() if agency is not None and agency.staff_group: - user.groups.add(agency.staff_group) + agency.staff_group.user_set.add(user)