From 831200fb60836ca12db9ad3cff4605a62b7c9baa Mon Sep 17 00:00:00 2001 From: Florian Aucomte Date: Thu, 2 May 2024 12:01:04 +0100 Subject: [PATCH 1/4] feat: Clean illogical data (#2290) --- .../migrations/0049_anonymise_orphan_users.py | 29 +++++ .../0050_anonymise_orphan_schools.py | 30 +++++ .../migrations/0051_verify_returning_users.py | 30 +++++ cfl_common/common/models.py | 1 - ...test_migration_anonymise_orphan_schools.py | 30 +++++ .../test_migration_anonymise_orphan_users.py | 30 +++++ ... => test_migration_unique_school_names.py} | 10 +- .../test_migration_verify_returning_users.py | 59 +++++++++ portal/tests/test_api.py | 119 ++++++++++++++---- portal/views/login/student.py | 43 +++++-- 10 files changed, 340 insertions(+), 41 deletions(-) create mode 100644 cfl_common/common/migrations/0049_anonymise_orphan_users.py create mode 100644 cfl_common/common/migrations/0050_anonymise_orphan_schools.py create mode 100644 cfl_common/common/migrations/0051_verify_returning_users.py create mode 100644 cfl_common/common/tests/test_migration_anonymise_orphan_schools.py create mode 100644 cfl_common/common/tests/test_migration_anonymise_orphan_users.py rename cfl_common/common/tests/{test_0048_unique_school_names.py => test_migration_unique_school_names.py} (74%) create mode 100644 cfl_common/common/tests/test_migration_verify_returning_users.py diff --git a/cfl_common/common/migrations/0049_anonymise_orphan_users.py b/cfl_common/common/migrations/0049_anonymise_orphan_users.py new file mode 100644 index 000000000..e2435d0db --- /dev/null +++ b/cfl_common/common/migrations/0049_anonymise_orphan_users.py @@ -0,0 +1,29 @@ +from django.apps.registry import Apps +from django.db import migrations + +from portal.views.api import __anonymise_user + + +def anonymise_orphan_users(apps: Apps, *args): + """ + Users should never exist without a user-type linked to them. Anonymise all + instances of User objects without a Teacher or Student instance. + """ + User = apps.get_model("auth", "User") + + active_orphan_users = User.objects.filter( + new_teacher__isnull=True, new_student__isnull=True, is_active=True + ) + + for active_orphan_user in active_orphan_users: + __anonymise_user(active_orphan_user) + + +class Migration(migrations.Migration): + dependencies = [("common", "0048_unique_school_names")] + + operations = [ + migrations.RunPython( + code=anonymise_orphan_users, reverse_code=migrations.RunPython.noop + ), + ] diff --git a/cfl_common/common/migrations/0050_anonymise_orphan_schools.py b/cfl_common/common/migrations/0050_anonymise_orphan_schools.py new file mode 100644 index 000000000..d677cf5ea --- /dev/null +++ b/cfl_common/common/migrations/0050_anonymise_orphan_schools.py @@ -0,0 +1,30 @@ +from uuid import uuid4 + +from django.apps.registry import Apps +from django.db import migrations + + +def anonymise_orphan_schools(apps: Apps, *args): + """ + Schools without any teachers or students should be anonymised (inactive). + Mark all active orphan schools as inactive. + """ + School = apps.get_model("common", "School") + + active_orphan_schools = School.objects.filter(teacher_school__isnull=True) + + for active_orphan_school in active_orphan_schools: + active_orphan_school.name = uuid4().hex + active_orphan_school.is_active = False + active_orphan_school.save() + + +class Migration(migrations.Migration): + dependencies = [("common", "0049_anonymise_orphan_users")] + + operations = [ + migrations.RunPython( + code=anonymise_orphan_schools, + reverse_code=migrations.RunPython.noop, + ), + ] diff --git a/cfl_common/common/migrations/0051_verify_returning_users.py b/cfl_common/common/migrations/0051_verify_returning_users.py new file mode 100644 index 000000000..4944ae50c --- /dev/null +++ b/cfl_common/common/migrations/0051_verify_returning_users.py @@ -0,0 +1,30 @@ +from django.apps.registry import Apps +from django.db import migrations + + +def verify_returning_users(apps: Apps, *args): + """ + Users cannot be unverified after having logged in at least once. Grab all + instances of unverified UserProfile where the User has logged in and mark it + as verified. + """ + UserProfile = apps.get_model("common", "UserProfile") + + unverified_returning_userprofiles = UserProfile.objects.filter( + user__last_login__isnull=False, is_verified=False + ) + + for unverified_returning_userprofile in unverified_returning_userprofiles: + unverified_returning_userprofile.is_verified = True + unverified_returning_userprofile.save() + + +class Migration(migrations.Migration): + dependencies = [("common", "0050_anonymise_orphan_schools")] + + operations = [ + migrations.RunPython( + code=verify_returning_users, + reverse_code=migrations.RunPython.noop, + ), + ] diff --git a/cfl_common/common/models.py b/cfl_common/common/models.py index 9bd198f90..871fac32b 100644 --- a/cfl_common/common/models.py +++ b/cfl_common/common/models.py @@ -2,7 +2,6 @@ from datetime import timedelta from uuid import uuid4 -import pgeocode from django.contrib.auth.models import User from django.db import models from django.utils import timezone diff --git a/cfl_common/common/tests/test_migration_anonymise_orphan_schools.py b/cfl_common/common/tests/test_migration_anonymise_orphan_schools.py new file mode 100644 index 000000000..f30d354c8 --- /dev/null +++ b/cfl_common/common/tests/test_migration_anonymise_orphan_schools.py @@ -0,0 +1,30 @@ +import pytest +from django_test_migrations.migrator import Migrator + + +@pytest.mark.django_db +def test_migration_anonymise_orphan_schools(migrator: Migrator): + state = migrator.apply_initial_migration( + ("common", "0049_anonymise_orphan_users") + ) + User = state.apps.get_model("auth", "User") + UserProfile = state.apps.get_model("common", "UserProfile") + Teacher = state.apps.get_model("common", "Teacher") + School = state.apps.get_model("common", "School") + + orphan_school = School.objects.create(name="OrphanSchool") + teacher_school = School.objects.create(name="TeacherSchool") + + teacher_user = User.objects.create_user("TeacherUser", password="password") + teacher_userprofile = UserProfile.objects.create(user=teacher_user) + Teacher.objects.create( + user=teacher_userprofile, new_user=teacher_user, school=teacher_school + ) + + migrator.apply_tested_migration(("common", "0050_anonymise_orphan_schools")) + + def assert_school_anonymised(pk: int, anonymised: bool): + assert School.objects.get(pk=pk).is_active != anonymised + + assert_school_anonymised(orphan_school.pk, True) + assert_school_anonymised(teacher_school.pk, False) diff --git a/cfl_common/common/tests/test_migration_anonymise_orphan_users.py b/cfl_common/common/tests/test_migration_anonymise_orphan_users.py new file mode 100644 index 000000000..4c52c4065 --- /dev/null +++ b/cfl_common/common/tests/test_migration_anonymise_orphan_users.py @@ -0,0 +1,30 @@ +import pytest +from django_test_migrations.migrator import Migrator + + +@pytest.mark.django_db +def test_migration_anonymise_orphan_users(migrator: Migrator): + state = migrator.apply_initial_migration( + ("common", "0048_unique_school_names") + ) + User = state.apps.get_model("auth", "User") + UserProfile = state.apps.get_model("common", "UserProfile") + Teacher = state.apps.get_model("common", "Teacher") + Student = state.apps.get_model("common", "Student") + + orphan_user = User.objects.create_user("OrphanUser", password="password") + teacher_user = User.objects.create_user("TeacherUser", password="password") + student_user = User.objects.create_user("StudentUser", password="password") + teacher_userprofile = UserProfile.objects.create(user=teacher_user) + student_userprofile = UserProfile.objects.create(user=student_user) + Teacher.objects.create(user=teacher_userprofile, new_user=teacher_user) + Student.objects.create(user=student_userprofile, new_user=student_user) + + migrator.apply_tested_migration(("common", "0049_anonymise_orphan_users")) + + def assert_user_anonymised(pk: int, anonymised: bool): + assert User.objects.get(pk=pk).is_active != anonymised + + assert_user_anonymised(orphan_user.pk, True) + assert_user_anonymised(teacher_user.pk, False) + assert_user_anonymised(student_user.pk, False) diff --git a/cfl_common/common/tests/test_0048_unique_school_names.py b/cfl_common/common/tests/test_migration_unique_school_names.py similarity index 74% rename from cfl_common/common/tests/test_0048_unique_school_names.py rename to cfl_common/common/tests/test_migration_unique_school_names.py index 1ad02f502..2e5ee144c 100644 --- a/cfl_common/common/tests/test_0048_unique_school_names.py +++ b/cfl_common/common/tests/test_migration_unique_school_names.py @@ -3,8 +3,10 @@ @pytest.mark.django_db -def test_0048_unique_school_names(migrator: Migrator): - state = migrator.apply_initial_migration(("common", "0047_delete_school_postcode")) +def test_migration_unique_school_names(migrator: Migrator): + state = migrator.apply_initial_migration( + ("common", "0047_delete_school_postcode") + ) School = state.apps.get_model("common", "School") school_name = "ExampleSchool" @@ -15,7 +17,9 @@ def test_0048_unique_school_names(migrator: Migrator): School(name=f"{school_name} 1"), ] ) - school_ids = list(School.objects.order_by("-id")[:3].values_list("id", flat=True)) + school_ids = list( + School.objects.order_by("-id")[:3].values_list("id", flat=True) + ) school_ids.reverse() migrator.apply_tested_migration(("common", "0048_unique_school_names")) diff --git a/cfl_common/common/tests/test_migration_verify_returning_users.py b/cfl_common/common/tests/test_migration_verify_returning_users.py new file mode 100644 index 000000000..f10919228 --- /dev/null +++ b/cfl_common/common/tests/test_migration_verify_returning_users.py @@ -0,0 +1,59 @@ +from datetime import datetime, timezone + +import pytest +from django_test_migrations.migrator import Migrator + +from portal.views.api import __anonymise_user + + +@pytest.mark.django_db +def test_migration_verify_returning_users(migrator: Migrator): + state = migrator.apply_initial_migration( + ("common", "0050_anonymise_orphan_schools") + ) + User = state.apps.get_model("auth", "User") + UserProfile = state.apps.get_model("common", "UserProfile") + + returning_user = User.objects.create_user( + "ReturningUser", + password="password", + last_login=datetime.now(tz=timezone.utc), + ) + returning_userprofile = UserProfile.objects.create(user=returning_user) + + non_returning_user = User.objects.create_user( + "NonReturningUser", password="password" + ) + non_returning_userprofile = UserProfile.objects.create( + user=non_returning_user + ) + + anonymised_returning_user = User.objects.create_user( + "AnonReturningUser", + password="password", + last_login=datetime.now(tz=timezone.utc), + ) + anonymised_returning_userprofile = UserProfile.objects.create( + user=anonymised_returning_user + ) + __anonymise_user(anonymised_returning_user) + + anonymised_non_returning_user = User.objects.create_user( + "AnonNonReturningUser", password="password" + ) + anonymised_non_returning_userprofile = UserProfile.objects.create( + user=anonymised_non_returning_user + ) + __anonymise_user(anonymised_non_returning_user) + + migrator.apply_tested_migration(("common", "0051_verify_returning_users")) + + def assert_userprofile_is_verified(pk: int, verified: bool): + assert UserProfile.objects.get(pk=pk).is_verified == verified + + assert_userprofile_is_verified(returning_userprofile.pk, True) + assert_userprofile_is_verified(non_returning_userprofile.pk, False) + assert_userprofile_is_verified(anonymised_returning_userprofile.pk, True) + assert_userprofile_is_verified( + anonymised_non_returning_userprofile.pk, False + ) diff --git a/portal/tests/test_api.py b/portal/tests/test_api.py index 88ebf0af7..902cae354 100644 --- a/portal/tests/test_api.py +++ b/portal/tests/test_api.py @@ -5,7 +5,10 @@ import pytest from common.models import Class, School, Student, Teacher from common.tests.utils.classes import create_class_directly -from common.tests.utils.organisation import create_organisation_directly, join_teacher_to_organisation +from common.tests.utils.organisation import ( + create_organisation_directly, + join_teacher_to_organisation, +) from common.tests.utils.student import create_school_student_directly from common.tests.utils.teacher import signup_teacher_directly from common.tests.utils.user import create_user_directly, get_superuser @@ -19,7 +22,10 @@ class APITests(APITestCase): def test_valid_date_registered(self): - url = reverse("registered-users", kwargs={"year": "2016", "month": "04", "day": "01"}) + url = reverse( + "registered-users", + kwargs={"year": "2016", "month": "04", "day": "01"}, + ) superuser = get_superuser() self.client.force_authenticate(user=superuser) response = self.client.get(url) @@ -27,14 +33,20 @@ def test_valid_date_registered(self): assert_that(isinstance(response.data, int)) def test_invalid_date_registered(self): - url = reverse("registered-users", kwargs={"year": "2016", "month": "05", "day": "35"}) + url = reverse( + "registered-users", + kwargs={"year": "2016", "month": "05", "day": "35"}, + ) superuser = get_superuser() self.client.force_authenticate(user=superuser) response = self.client.get(url) assert_that(response, has_status_code(status.HTTP_404_NOT_FOUND)) def test_valid_date_lastconnectedsince(self): - url = reverse("last-connected-since", kwargs={"year": "2016", "month": "04", "day": "01"}) + url = reverse( + "last-connected-since", + kwargs={"year": "2016", "month": "04", "day": "01"}, + ) superuser = get_superuser() self.client.force_authenticate(user=superuser) response = self.client.get(url) @@ -42,7 +54,10 @@ def test_valid_date_lastconnectedsince(self): assert_that(isinstance(response.data, int)) def test_invalid_date_lastconnectedsince(self): - url = reverse("last-connected-since", kwargs={"year": "2016", "month": "05", "day": "35"}) + url = reverse( + "last-connected-since", + kwargs={"year": "2016", "month": "05", "day": "35"}, + ) superuser = get_superuser() self.client.force_authenticate(user=superuser) response = self.client.get(url) @@ -67,7 +82,9 @@ def test_get_inactive_users_if_admin(self): assert len(response.data) == 1 @patch("portal.views.api.IS_CLOUD_SCHEDULER_FUNCTION", return_value=True) - def test_get_inactive_users_if_appengine(self, mock_is_cloud_scheduler_function): + def test_get_inactive_users_if_appengine( + self, mock_is_cloud_scheduler_function + ): client = APIClient() create_user_directly(active=False) create_user_directly(active=True) @@ -86,7 +103,9 @@ def test_get_inactive_users_if_unauthorised(self): assert response.status_code == status.HTTP_403_FORBIDDEN @patch("portal.views.api.IS_CLOUD_SCHEDULER_FUNCTION", return_value=True) - def test_delete_inactive_users_if_appengine(self, mock_is_cloud_scheduler_function): + def test_delete_inactive_users_if_appengine( + self, mock_is_cloud_scheduler_function + ): client = APIClient() create_user_directly(active=False) create_user_directly(active=False) @@ -94,14 +113,25 @@ def test_delete_inactive_users_if_appengine(self, mock_is_cloud_scheduler_functi response = client.get(url) users = response.data assert len(users) == 2 + + # NOTE: Migration 0049 causes user 34 (created via migration 0001) to + # be marked as inactive. Slightly tweaked this test so it still + # passes but takes into account this new anonymisation. + old_deleted_users = list(User.objects.filter(is_active=False)) + assert len(old_deleted_users) == 1 + response = client.delete(url) assert mock_is_cloud_scheduler_function.called assert response.status_code == status.HTTP_204_NO_CONTENT + for user in users: with pytest.raises(User.DoesNotExist): User.objects.get(username=user["username"]) + deleted_users = list(User.objects.filter(is_active=False)) - assert len(deleted_users) == 2 + new_deleted_users_count = len(deleted_users) - len(old_deleted_users) + assert new_deleted_users_count == 2 + for user in deleted_users: assert user.first_name == "Deleted" assert user.last_name == "User" @@ -112,27 +142,41 @@ def test_delete_inactive_users_if_appengine(self, mock_is_cloud_scheduler_functi assert len(response.data) == 0 @patch("portal.views.api.IS_CLOUD_SCHEDULER_FUNCTION", return_value=True) - def test_orphan_schools_and_classes_are_anonymised(self, mock_is_cloud_scheduler_function): + def test_orphan_schools_and_classes_are_anonymised( + self, mock_is_cloud_scheduler_function + ): client = APIClient() # Create a school with an active teacher school1_teacher1_email, _ = signup_teacher_directly() school1 = create_organisation_directly(school1_teacher1_email) - klass11, _, access_code11 = create_class_directly(school1_teacher1_email) + klass11, _, access_code11 = create_class_directly( + school1_teacher1_email + ) _, _, student11 = create_school_student_directly(access_code11) # Create a school with one active non-admin teacher and one inactive admin teacher school2_teacher1_email, _ = signup_teacher_directly() school2_teacher2_email, _ = signup_teacher_directly() school2 = create_organisation_directly(school2_teacher1_email) - join_teacher_to_organisation(school2_teacher2_email, school2.name, is_admin=True) - klass21, _, access_code21 = create_class_directly(school2_teacher1_email) + join_teacher_to_organisation( + school2_teacher2_email, school2.name, is_admin=True + ) + klass21, _, access_code21 = create_class_directly( + school2_teacher1_email + ) _, _, student21 = create_school_student_directly(access_code21) - klass22, _, access_code22 = create_class_directly(school2_teacher2_email) + klass22, _, access_code22 = create_class_directly( + school2_teacher2_email + ) _, _, student22 = create_school_student_directly(access_code22) - school2_teacher1 = Teacher.objects.get(new_user__email=school2_teacher1_email) + school2_teacher1 = Teacher.objects.get( + new_user__email=school2_teacher1_email + ) school2_teacher1.is_admin = False school2_teacher1.save() - school2_teacher2 = Teacher.objects.get(new_user__email=school2_teacher2_email) + school2_teacher2 = Teacher.objects.get( + new_user__email=school2_teacher2_email + ) school2_teacher2.new_user.is_active = False school2_teacher2.new_user.save() @@ -141,28 +185,40 @@ def test_orphan_schools_and_classes_are_anonymised(self, mock_is_cloud_scheduler school3_teacher2_email, _ = signup_teacher_directly() school3 = create_organisation_directly(school3_teacher1_email) join_teacher_to_organisation(school3_teacher2_email, school3.name) - klass31, _, access_code31 = create_class_directly(school3_teacher1_email) + klass31, _, access_code31 = create_class_directly( + school3_teacher1_email + ) _, _, student31 = create_school_student_directly(access_code31) - klass32, _, access_code32 = create_class_directly(school3_teacher2_email) + klass32, _, access_code32 = create_class_directly( + school3_teacher2_email + ) _, _, student32 = create_school_student_directly(access_code32) - school3_teacher1 = Teacher.objects.get(new_user__email=school3_teacher1_email) + school3_teacher1 = Teacher.objects.get( + new_user__email=school3_teacher1_email + ) school3_teacher1.new_user.is_active = False school3_teacher1.new_user.save() - school3_teacher2 = Teacher.objects.get(new_user__email=school3_teacher2_email) + school3_teacher2 = Teacher.objects.get( + new_user__email=school3_teacher2_email + ) school3_teacher2.new_user.is_active = False school3_teacher2.new_user.save() # Create a school with no active teachers school4_teacher1_email, _ = signup_teacher_directly() school4 = create_organisation_directly(school4_teacher1_email) - school4_teacher1 = Teacher.objects.get(new_user__email=school4_teacher1_email) + school4_teacher1 = Teacher.objects.get( + new_user__email=school4_teacher1_email + ) school4_teacher1.new_user.is_active = False school4_teacher1.new_user.save() # Create a school with no teachers school5_teacher1_email, _ = signup_teacher_directly() school5 = create_organisation_directly(school5_teacher1_email) - school5_teacher1 = Teacher.objects.get(new_user__email=school5_teacher1_email) + school5_teacher1 = Teacher.objects.get( + new_user__email=school5_teacher1_email + ) school5_teacher1.delete() # Call the API @@ -182,7 +238,9 @@ def test_orphan_schools_and_classes_are_anonymised(self, mock_is_cloud_scheduler assert Student.objects.filter(pk=student21.pk).exists() assert not Student.objects.get(pk=student22.pk).new_user.is_active # Also check the first teacher is now an admin - assert Teacher.objects.get(new_user__email=school2_teacher1_email).is_admin + assert Teacher.objects.get( + new_user__email=school2_teacher1_email + ).is_admin # Check the third school is anonymised together with its classes and students assert not School.objects.filter(name=school3.name).exists() @@ -250,14 +308,19 @@ def test_remove_fake_accounts(self): last_name=random_account["last_name"], ) - assert len(User.objects.all()) == len(random_accounts) + initial_users_length + assert ( + len(User.objects.all()) + == len(random_accounts) + initial_users_length + ) client.login(username=admin_username, password=admin_password) response = client.get(reverse("remove_fake_accounts")) assert response.status_code == 204 # check if after deletion all the users are still there - assert len(User.objects.all()) == initial_users_length + 2 # mentioned in the fake_accounts description + assert ( + len(User.objects.all()) == initial_users_length + 2 + ) # mentioned in the fake_accounts description def has_status_code(status_code): @@ -272,7 +335,11 @@ def _matches(self, response): return response.status_code == self.status_code def describe_to(self, description): - description.append_text("has status code ").append_text(self.status_code) + description.append_text("has status code ").append_text( + self.status_code + ) def describe_mismatch(self, response, mismatch_description): - mismatch_description.append_text("had status code ").append_text(response.status_code) + mismatch_description.append_text("had status code ").append_text( + response.status_code + ) diff --git a/portal/views/login/student.py b/portal/views/login/student.py index 02d582af6..92fda8829 100644 --- a/portal/views/login/student.py +++ b/portal/views/login/student.py @@ -51,7 +51,8 @@ def _add_logged_in_as_message(self, request): class_name = self.kwargs["access_code"].upper() messages.info( request, - f"You are logged in to class: " f"{escape(class_name)}", + f"You are logged in to class: " + f"{escape(class_name)}", extra_tags="safe message--student", ) @@ -72,7 +73,9 @@ def _add_login_data(self, form, login_type): klass = classes[0] name = form.cleaned_data.get("username") - students = Student.objects.filter(new_user__first_name__iexact=name, class_field=klass) + students = Student.objects.filter( + new_user__first_name__iexact=name, class_field=klass + ) try: student = students[0] except IndexError: @@ -81,32 +84,44 @@ def _add_login_data(self, form, login_type): raise Exception(msg) # Log the login time, class, and login type - session = UserSession(user=student.new_user, class_field=klass, login_type=login_type) + session = UserSession( + user=student.new_user, class_field=klass, login_type=login_type + ) session.save() + student.user.is_verified = True + student.user.save() + def form_valid(self, form): """Security check complete. Log the user in.""" - # Reset ratelimit cache upon successful login clear_ratelimit_cache_for_user(form.cleaned_data["username"]) - login_type = self.kwargs.get("login_type", "classlink") # default to "classlink" if not specified + login_type = self.kwargs.get( + "login_type", "classlink" + ) # default to "classlink" if not specified self._add_login_data(form, login_type) return super(StudentLoginView, self).form_valid(form) def post(self, request, *args, **kwargs): """ - If the first name and access code found under the url inputted in the form corresponds to that of a blocked - account, this redirects the user to the locked out page. However, if the lockout - time is more than 24 hours before this is executed, the account is unlocked. + If the first name and access code found under the url inputted in the + form corresponds to that of a blocked account, this redirects the user + to the locked out page. However, if the lockout time is more than 24 + hours before this is executed, the account is unlocked. """ username = request.POST.get("username") # get access code from the current url access_code = get_access_code_from_request(request) - if Student.objects.filter(new_user__first_name=username, class_field__access_code=access_code).exists(): - student = Student.objects.get(new_user__first_name=username, class_field__access_code=access_code) + if Student.objects.filter( + new_user__first_name=username, class_field__access_code=access_code + ).exists(): + student = Student.objects.get( + new_user__first_name=username, + class_field__access_code=access_code, + ) if student.blocked_time is not None: if has_user_lockout_expired(student): @@ -129,9 +144,15 @@ def student_direct_login(request, user_id, login_id): if user: # Log the login time and class student = Student.objects.get(new_user=user) - session = UserSession(user=user, class_field=student.class_field, login_type="direct") + session = UserSession( + user=user, class_field=student.class_field, login_type="direct" + ) session.save() login(request, user) + + student.user.is_verified = True + student.user.save() + return HttpResponseRedirect(reverse_lazy("student_details")) return HttpResponseRedirect(reverse_lazy("home")) From dcbaa2f17382a6edd61870a057f7c008bcd0af44 Mon Sep 17 00:00:00 2001 From: github-actions Date: Thu, 2 May 2024 11:03:01 +0000 Subject: [PATCH 2/4] 6.43.0 Automatically generated by python-semantic-release --- portal/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/portal/__init__.py b/portal/__init__.py index 3d0e834b2..8fef5c28a 100644 --- a/portal/__init__.py +++ b/portal/__init__.py @@ -1 +1 @@ -__version__ = "6.42.0" +__version__ = "6.43.0" From c9fddfb3b7037434c8c77090c365393441629737 Mon Sep 17 00:00:00 2001 From: evemartin Date: Thu, 2 May 2024 13:38:06 +0100 Subject: [PATCH 3/4] fix: move admin given/revoked emails to dotdigital (#2292) * move admin given/revoked emails to dotdigital * correct teacher email * fix tests * address PR comment * Merge branch 'master' into move-admin-given-and-revoked-emails * Try codecov quick fix * Ubuntu latest everywhere * Add project to codecov yaml Co-Authored-By: faucomte97 --- .codecov.yml | 3 + .github/workflows/ci.yml | 11 +- .github/workflows/publish-python-package.yml | 2 +- .../workflows/semantic-pull-request-check.yml | 2 +- .github/workflows/snyk.yaml | 2 +- cfl_common/common/email_messages.py | 24 ---- cfl_common/common/helpers/emails.py | 53 ++----- cfl_common/common/mail.py | 132 ++++++++++++------ portal/tests/test_organisation.py | 8 +- portal/views/teacher/dashboard.py | 88 +++++++----- 10 files changed, 178 insertions(+), 147 deletions(-) diff --git a/.codecov.yml b/.codecov.yml index 2e60774f8..5f7650737 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -6,6 +6,9 @@ coverage: patch: default: target: 90% + project: + default: + target: 90% ignore: - "portal/tests/*.py" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 54b108e69..2c145b1c9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,9 +6,11 @@ on: jobs: test: name: Run tests - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest env: LANG: C.UTF-8 + COVERAGE_REPORT: coverage.xml # NOTE: COVERAGE_FILE is reserved - do not use. + OCADO_TECH_ORG_ID: 2088731 steps: - name: Checkout uses: actions/checkout@v3 @@ -48,4 +50,9 @@ jobs: CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: Upload coverage to Codecov - uses: codecov/codecov-action@v3 + if: github.repository_owner_id == env.OCADO_TECH_ORG_ID + uses: codecov/codecov-action@v4 + with: + fail_ci_if_error: true + token: ${{ secrets.CODECOV_TOKEN }} + file: ${{ env.COVERAGE_REPORT }} diff --git a/.github/workflows/publish-python-package.yml b/.github/workflows/publish-python-package.yml index beac2edd0..aac41fa7c 100644 --- a/.github/workflows/publish-python-package.yml +++ b/.github/workflows/publish-python-package.yml @@ -9,7 +9,7 @@ on: jobs: publish-pypi-packages: name: Publish PyPi Packages - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest steps: - name: Checkout uses: actions/checkout@v3 diff --git a/.github/workflows/semantic-pull-request-check.yml b/.github/workflows/semantic-pull-request-check.yml index 7371d424c..8c0d9e4ba 100644 --- a/.github/workflows/semantic-pull-request-check.yml +++ b/.github/workflows/semantic-pull-request-check.yml @@ -10,7 +10,7 @@ on: jobs: main: name: Validate PR title - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest steps: - uses: amannn/action-semantic-pull-request@v5 env: diff --git a/.github/workflows/snyk.yaml b/.github/workflows/snyk.yaml index 0872aa8e4..2aad94ad6 100644 --- a/.github/workflows/snyk.yaml +++ b/.github/workflows/snyk.yaml @@ -6,7 +6,7 @@ on: jobs: security: name: Run Snyk - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest env: LANG: C.UTF-8 steps: diff --git a/cfl_common/common/email_messages.py b/cfl_common/common/email_messages.py index 776d60316..f5bc9a1f8 100644 --- a/cfl_common/common/email_messages.py +++ b/cfl_common/common/email_messages.py @@ -29,30 +29,6 @@ def kickedEmail(request, schoolName): } -def adminGivenEmail(request, schoolName): - - url = request.build_absolute_uri(reverse("dashboard")) - - return { - "subject": f"You have been made a school or club administrator", - "message": ( - f"Administrator control of the school or club '{schoolName}' has been " - f"given to you. Go to {url} to start managing your school or club." - ), - } - - -def adminRevokedEmail(request, schoolName): - return { - "subject": f"You are no longer a school or club administrator", - "message": ( - f"Your administrator control of the school or club '{schoolName}' has been " - f"revoked. If you think this is an error, please contact one of the other " - f"administrators in your school or club." - ), - } - - def studentJoinRequestSentEmail(request, schoolName, accessCode): return { "subject": f"School or club join request sent", diff --git a/cfl_common/common/helpers/emails.py b/cfl_common/common/helpers/emails.py index 8d1b5ef9f..88f0611b4 100644 --- a/cfl_common/common/helpers/emails.py +++ b/cfl_common/common/helpers/emails.py @@ -6,14 +6,11 @@ import jwt from common import app_settings -from common.app_settings import domain -from common.mail import campaign_ids, send_dotdigital_email +from common.mail import campaign_ids, django_send_email, send_dotdigital_email from common.models import Student, Teacher from django.conf import settings from django.contrib.auth.models import User -from django.core.mail import EmailMultiAlternatives from django.http import HttpResponse -from django.template import loader from django.urls import reverse from django.utils import timezone from requests import delete, get, post, put @@ -31,41 +28,6 @@ class DotmailerUserType(Enum): NO_ACCOUNT = auto() -def send_email( - sender, - recipients, - subject, - text_content, - title, - replace_url=None, - plaintext_template="email.txt", - html_template="email.html", -): - # add in template for templates to message - - # setup templates - plaintext = loader.get_template(plaintext_template) - html = loader.get_template(html_template) - plaintext_email_context = {"content": text_content} - html_email_context = {"content": text_content, "title": title, "url_prefix": domain()} - - # render templates - plaintext_body = plaintext.render(plaintext_email_context) - original_html_body = html.render(html_email_context) - html_body = original_html_body - - if replace_url: - verify_url = replace_url["verify_url"] - verify_replace_url = re.sub(f"(.*/verify_email/)(.*)", f"\\1", verify_url) - html_body = re.sub(f"({verify_url})(.*){verify_url}", f"\\1\\2{verify_replace_url}", original_html_body) - - # make message using templates - message = EmailMultiAlternatives(subject, plaintext_body, sender, recipients) - message.attach_alternative(html_body, "text/html") - - message.send() - - def generate_token(user, new_email="", preverified=False): if preverified: user.userprofile.is_verified = preverified @@ -91,6 +53,19 @@ def _newsletter_ticked(data): return "newsletter_ticked" in data and data["newsletter_ticked"] +def send_email( + sender, + recipients, + subject, + text_content, + title, + replace_url=None, + plaintext_template="email.txt", + html_template="email.html", +): + django_send_email(sender, recipients, subject, text_content, title, replace_url, plaintext_template, html_template) + + def send_verification_email(request, user, data, new_email=None, age=None): """ Sends emails relating to email address verification. diff --git a/cfl_common/common/mail.py b/cfl_common/common/mail.py index b786b830a..f4412ae93 100644 --- a/cfl_common/common/mail.py +++ b/cfl_common/common/mail.py @@ -3,8 +3,13 @@ import requests from common import app_settings +from common.app_settings import MODULE_NAME, domain +from django.core.mail import EmailMultiAlternatives +from django.template import loader campaign_ids = { + "admin_given": 1569057, + "admin_revoked": 1569071, "email_change_notification": 1551600, "email_change_verification": 1551594, "reset_password": 1557153, @@ -34,6 +39,41 @@ class EmailAttachment: content: str +def django_send_email( + sender, + recipients, + subject, + text_content, + title, + replace_url=None, + plaintext_template="email.txt", + html_template="email.html", +): + # add in template for templates to message + + # setup templates + plaintext = loader.get_template(plaintext_template) + html = loader.get_template(html_template) + plaintext_email_context = {"content": text_content} + html_email_context = {"content": text_content, "title": title, "url_prefix": domain()} + + # render templates + plaintext_body = plaintext.render(plaintext_email_context) + original_html_body = html.render(html_email_context) + html_body = original_html_body + + if replace_url: + verify_url = replace_url["verify_url"] + verify_replace_url = re.sub(f"(.*/verify_email/)(.*)", f"\\1", verify_url) + html_body = re.sub(f"({verify_url})(.*){verify_url}", f"\\1\\2{verify_replace_url}", original_html_body) + + # make message using templates + message = EmailMultiAlternatives(subject, plaintext_body, sender, recipients) + message.attach_alternative(html_body, "text/html") + + message.send() + + # pylint: disable-next=too-many-arguments def send_dotdigital_email( campaign_id: int, @@ -71,47 +111,51 @@ def send_dotdigital_email( """ # pylint: enable=line-too-long - if auth is None: - auth = app_settings.DOTDIGITAL_AUTH - - body = { - "campaignId": campaign_id, - "toAddresses": to_addresses, - } - if cc_addresses is not None: - body["ccAddresses"] = cc_addresses - if bcc_addresses is not None: - body["bccAddresses"] = bcc_addresses - if from_address is not None: - body["fromAddress"] = from_address - if personalization_values is not None: - body["personalizationValues"] = [ - { - "name": key, - "value": value, - } - for key, value in personalization_values.items() - ] - if metadata is not None: - body["metadata"] = metadata - if attachments is not None: - body["attachments"] = [ - { - "fileName": attachment.file_name, - "mimeType": attachment.mime_type, - "content": attachment.content, - } - for attachment in attachments - ] - - response = requests.post( - url=f"https://{region}-api.dotdigital.com/v2/email/triggered-campaign", - json=body, - headers={ - "accept": "text/plain", - "authorization": auth, - }, - timeout=timeout, - ) - - assert response.ok, "Failed to send email." f" Reason: {response.reason}." f" Text: {response.text}." + # Dotdigital emails don't work locally, so if testing emails locally use Django to send a dummy email instead + if MODULE_NAME == "local": + django_send_email(from_address, to_addresses, "dummy_subject", "dummy_text_content", "dummy_title") + else: + if auth is None: + auth = app_settings.DOTDIGITAL_AUTH + + body = { + "campaignId": campaign_id, + "toAddresses": to_addresses, + } + if cc_addresses is not None: + body["ccAddresses"] = cc_addresses + if bcc_addresses is not None: + body["bccAddresses"] = bcc_addresses + if from_address is not None: + body["fromAddress"] = from_address + if personalization_values is not None: + body["personalizationValues"] = [ + { + "name": key, + "value": value, + } + for key, value in personalization_values.items() + ] + if metadata is not None: + body["metadata"] = metadata + if attachments is not None: + body["attachments"] = [ + { + "fileName": attachment.file_name, + "mimeType": attachment.mime_type, + "content": attachment.content, + } + for attachment in attachments + ] + + response = requests.post( + url=f"https://{region}-api.dotdigital.com/v2/email/triggered-campaign", + json=body, + headers={ + "accept": "text/plain", + "authorization": auth, + }, + timeout=timeout, + ) + + assert response.ok, "Failed to send email." f" Reason: {response.reason}." f" Text: {response.text}." diff --git a/portal/tests/test_organisation.py b/portal/tests/test_organisation.py index c4512640d..bf93df780 100644 --- a/portal/tests/test_organisation.py +++ b/portal/tests/test_organisation.py @@ -4,9 +4,11 @@ from common.models import Teacher from common.tests.utils.classes import create_class_directly -from common.tests.utils.organisation import (create_organisation, - create_organisation_directly, - join_teacher_to_organisation) +from common.tests.utils.organisation import ( + create_organisation, + create_organisation_directly, + join_teacher_to_organisation, +) from common.tests.utils.student import create_school_student_directly from common.tests.utils.teacher import signup_teacher_directly from selenium.webdriver.common.by import By diff --git a/portal/views/teacher/dashboard.py b/portal/views/teacher/dashboard.py index 9dc4d84fa..3ae649e74 100644 --- a/portal/views/teacher/dashboard.py +++ b/portal/views/teacher/dashboard.py @@ -2,12 +2,24 @@ from uuid import uuid4 from common import email_messages -from common.helpers.emails import (INVITE_FROM, NOTIFICATION_EMAIL, - DotmailerUserType, add_to_dotmailer, - generate_token, send_email, update_email) +from common.helpers.emails import ( + INVITE_FROM, + NOTIFICATION_EMAIL, + DotmailerUserType, + add_to_dotmailer, + generate_token, + send_email, + update_email, +) from common.helpers.generators import get_random_username -from common.models import (Class, JoinReleaseStudent, SchoolTeacherInvitation, - Student, Teacher) +from common.mail import campaign_ids, send_dotdigital_email +from common.models import ( + Class, + JoinReleaseStudent, + SchoolTeacherInvitation, + Student, + Teacher, +) from common.permissions import check_teacher_authorised, logged_in_as_teacher from common.utils import using_two_factor from django.contrib import messages as messages @@ -16,7 +28,7 @@ from django.contrib.auth.models import User from django.http import Http404, HttpResponseRedirect from django.shortcuts import get_object_or_404, render -from django.urls import reverse_lazy +from django.urls import reverse, reverse_lazy from django.utils import timezone from django.views.decorators.http import require_POST from game.level_management import levels_shared_with, unshare_level @@ -25,14 +37,20 @@ from portal.forms.invite_teacher import InviteTeacherForm from portal.forms.organisation import OrganisationForm from portal.forms.registration import DeleteAccountForm -from portal.forms.teach import (ClassCreationForm, InvitedTeacherForm, - TeacherAddExternalStudentForm, - TeacherEditAccountForm) +from portal.forms.teach import ( + ClassCreationForm, + InvitedTeacherForm, + TeacherAddExternalStudentForm, + TeacherEditAccountForm, +) from portal.helpers.decorators import ratelimit from portal.helpers.password import check_update_password -from portal.helpers.ratelimit import (RATELIMIT_LOGIN_GROUP, - RATELIMIT_LOGIN_RATE, RATELIMIT_METHOD, - clear_ratelimit_cache_for_user) +from portal.helpers.ratelimit import ( + RATELIMIT_LOGIN_GROUP, + RATELIMIT_LOGIN_RATE, + RATELIMIT_METHOD, + clear_ratelimit_cache_for_user, +) from .teach import create_class @@ -380,19 +398,22 @@ def invite_toggle_admin(request, invite_id): if invite.invited_teacher_is_admin: messages.success(request, "Administrator invite status has been given successfully") - emailMessage = email_messages.adminGivenEmail(request, invite.school) + send_dotdigital_email( + campaign_ids["admin_given"], + [invite.invited_teacher_email], + personalization_values={ + "SCHOOL_CLUB_NAME": invite.school, + "MANAGEMENT_LINK": request.build_absolute_uri(reverse("dashboard")), + }, + ) else: messages.success(request, "Administrator invite status has been revoked successfully") - emailMessage = email_messages.adminRevokedEmail(request, invite.school) - - send_email( - NOTIFICATION_EMAIL, - [invite.invited_teacher_email], - emailMessage["subject"], - emailMessage["message"], - emailMessage["subject"], - ) + send_dotdigital_email( + campaign_ids["admin_revoked"], + [invite.invited_teacher_email], + personalization_values={"SCHOOL_CLUB_NAME": invite.school}, + ) return HttpResponseRedirect(reverse_lazy("dashboard")) @@ -411,7 +432,14 @@ def organisation_toggle_admin(request, pk): if teacher.is_admin: messages.success(request, "Administrator status has been given successfully.") - email_message = email_messages.adminGivenEmail(request, teacher.school.name) + send_dotdigital_email( + campaign_ids["admin_given"], + [teacher.new_user.email], + personalization_values={ + "SCHOOL_CLUB_NAME": teacher.school.name, + "MANAGEMENT_LINK": request.build_absolute_uri(reverse("dashboard")), + }, + ) else: # Remove access to all levels that are from other teachers' students [ @@ -420,15 +448,11 @@ def organisation_toggle_admin(request, pk): if hasattr(level.owner, "student") and not teacher.teaches(level.owner) ] messages.success(request, "Administrator status has been revoked successfully.") - email_message = email_messages.adminRevokedEmail(request, teacher.school.name) - - send_email( - NOTIFICATION_EMAIL, - [teacher.new_user.email], - email_message["subject"], - email_message["message"], - email_message["subject"], - ) + send_dotdigital_email( + campaign_ids["admin_revoked"], + [teacher.new_user.email], + personalization_values={"SCHOOL_CLUB_NAME": teacher.school.name}, + ) return HttpResponseRedirect(reverse_lazy("dashboard")) From a137e03f522347d45c066e6fa4ab77070072a5dc Mon Sep 17 00:00:00 2001 From: github-actions Date: Thu, 2 May 2024 12:40:01 +0000 Subject: [PATCH 4/4] 6.43.1 Automatically generated by python-semantic-release --- portal/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/portal/__init__.py b/portal/__init__.py index 8fef5c28a..cf3c09654 100644 --- a/portal/__init__.py +++ b/portal/__init__.py @@ -1 +1 @@ -__version__ = "6.43.0" +__version__ = "6.43.1"