From f7a48f38eb60bdde36b7b2500cdb78cbecbc077b Mon Sep 17 00:00:00 2001 From: djperrefort Date: Mon, 12 Aug 2024 10:13:15 -0400 Subject: [PATCH 1/7] Adds tests for Preference class --- keystone_api/apps/notifications/admin.py | 1 + .../notifications/migrations/0001_initial.py | 4 +- keystone_api/apps/notifications/models.py | 20 ++++-- .../notifications/tests/models/__init__.py | 0 .../tests/models/test_Preference.py | 64 +++++++++++++++++++ 5 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 keystone_api/apps/notifications/tests/models/__init__.py create mode 100644 keystone_api/apps/notifications/tests/models/test_Preference.py diff --git a/keystone_api/apps/notifications/admin.py b/keystone_api/apps/notifications/admin.py index 6c2a0981..4809f95a 100644 --- a/keystone_api/apps/notifications/admin.py +++ b/keystone_api/apps/notifications/admin.py @@ -4,6 +4,7 @@ interfaces for managing application database constructs. """ +from django.conf import settings from django.contrib import admin from .models import * diff --git a/keystone_api/apps/notifications/migrations/0001_initial.py b/keystone_api/apps/notifications/migrations/0001_initial.py index 1860af0d..d8111c9e 100644 --- a/keystone_api/apps/notifications/migrations/0001_initial.py +++ b/keystone_api/apps/notifications/migrations/0001_initial.py @@ -31,9 +31,9 @@ class Migration(migrations.Migration): name='Preference', fields=[ ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('alloc_thresholds', models.JSONField(default=apps.notifications.models._default_alloc_thresholds)), + ('alloc_thresholds', models.JSONField(default=apps.notifications.models.default_alloc_thresholds)), ('notify_status_update', models.BooleanField(default=True)), - ('expiry_thresholds', models.JSONField(default=apps.notifications.models._default_expiry_thresholds)), + ('expiry_thresholds', models.JSONField(default=apps.notifications.models.default_expiry_thresholds)), ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), ], ), diff --git a/keystone_api/apps/notifications/models.py b/keystone_api/apps/notifications/models.py index 68b0709f..ac2cc8b6 100644 --- a/keystone_api/apps/notifications/models.py +++ b/keystone_api/apps/notifications/models.py @@ -11,12 +11,24 @@ from django.conf import settings from django.db import models +__all__ = ['Notification', 'Preference'] + + +def default_alloc_thresholds() -> list[int]: # pragma: nocover + """The default allocating usage thresholds at which to issue a user notification. + + Returned values are defined in units of percent usage. + """ -def _default_alloc_thresholds() -> list[int]: return [90] -def _default_expiry_thresholds() -> list[int]: +def default_expiry_thresholds() -> list[int]: # pragma: nocover + """The default expiration thresholds at which to issue a user notification. + + Returned values are defined in units of days until expiration. + """ + return [30, 14, 0] @@ -47,9 +59,9 @@ class NotificationType(models.TextChoices): class Preference(models.Model): """User notification preferences.""" - alloc_thresholds = models.JSONField(default=_default_alloc_thresholds) + alloc_thresholds = models.JSONField(default=default_alloc_thresholds) notify_status_update = models.BooleanField(default=True) - expiry_thresholds = models.JSONField(default=_default_expiry_thresholds) + expiry_thresholds = models.JSONField(default=default_expiry_thresholds) user = models.OneToOneField(settings.AUTH_USER_MODEL, on_delete=models.CASCADE) diff --git a/keystone_api/apps/notifications/tests/models/__init__.py b/keystone_api/apps/notifications/tests/models/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/keystone_api/apps/notifications/tests/models/test_Preference.py b/keystone_api/apps/notifications/tests/models/test_Preference.py new file mode 100644 index 00000000..9540b509 --- /dev/null +++ b/keystone_api/apps/notifications/tests/models/test_Preference.py @@ -0,0 +1,64 @@ +"""Unit tests for the `Preference` class.""" + +from django.contrib.auth import get_user_model +from django.test import TestCase + +from apps.notifications.models import default_alloc_thresholds, default_expiry_thresholds, Preference + +User = get_user_model() + + +class PreferenceModelTest(TestCase): + """Tests for getting user preferences.""" + + def setUp(self) -> None: + """Create a test user.""" + + self.user = User.objects.create_user(username='testuser', password='foobar123!') + + def test_get_user_preference_creates_new_preference(self) -> None: + """Test a new Preference object is created if one does not exist.""" + + self.assertFalse(Preference.objects.filter(user=self.user).exists()) + preference = Preference.get_user_preference(user=self.user) + self.assertTrue(Preference.objects.filter(user=self.user).exists()) + + # Ensure preference is created with appropriate defaults + self.assertEqual(self.user, preference.user) + self.assertListEqual(default_alloc_thresholds(), preference.alloc_thresholds) + self.assertListEqual(default_expiry_thresholds(), preference.expiry_thresholds) + + def test_get_user_preference_returns_existing_preference(self) -> None: + """Test an existing Preference object is returned if it already exists.""" + + existing_preference = Preference.objects.create(user=self.user) + preference = Preference.get_user_preference(user=self.user) + self.assertEqual(existing_preference, preference) + + +class PreferenceSetTest(TestCase): + """Tests for setting user preferences.""" + + def setUp(self) -> None: + """Create a test user.""" + + self.user = User.objects.create_user(username='testuser', password='foobar123!') + + def test_set_user_preference_creates_preference(self) -> None: + """Test that a new Preference object is created with specified values.""" + + self.assertFalse(Preference.objects.filter(user=self.user).exists()) + + Preference.set_user_preference(user=self.user, notify_status_update=False) + preference = Preference.objects.get(user=self.user) + self.assertFalse(preference.notify_status_update) + + def test_set_user_preference_updates_existing_preference(self) -> None: + """Test that an existing Preference object is updated with specified values.""" + + preference = Preference.objects.create(user=self.user, notify_status_update=True) + self.assertTrue(Preference.objects.filter(user=self.user).exists()) + + Preference.set_user_preference(user=self.user, notify_status_update=False) + preference.refresh_from_db() + self.assertFalse(preference.notify_status_update) From 8d95ba0583ec12b69e95af11fa755afb584e6d2f Mon Sep 17 00:00:00 2001 From: djperrefort Date: Mon, 12 Aug 2024 10:26:06 -0400 Subject: [PATCH 2/7] Fixes bug in set_user_preference where existing preferences cannot be updated --- keystone_api/apps/notifications/models.py | 4 ++-- .../apps/notifications/tests/models/test_Preference.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/keystone_api/apps/notifications/models.py b/keystone_api/apps/notifications/models.py index ac2cc8b6..7b0bf4e0 100644 --- a/keystone_api/apps/notifications/models.py +++ b/keystone_api/apps/notifications/models.py @@ -73,7 +73,7 @@ def get_user_preference(cls, user: settings.AUTH_USER_MODEL) -> Preference: return preference @classmethod - def set_user_preference(cls, *args, **kwargs) -> None: + def set_user_preference(cls, user: settings.AUTH_USER_MODEL, **kwargs) -> None: """Set user preferences, creating or updating as necessary.""" - cls.objects.create(*args, **kwargs) + cls.objects.update_or_create(user=user, defaults=kwargs) diff --git a/keystone_api/apps/notifications/tests/models/test_Preference.py b/keystone_api/apps/notifications/tests/models/test_Preference.py index 9540b509..136fd861 100644 --- a/keystone_api/apps/notifications/tests/models/test_Preference.py +++ b/keystone_api/apps/notifications/tests/models/test_Preference.py @@ -19,6 +19,7 @@ def setUp(self) -> None: def test_get_user_preference_creates_new_preference(self) -> None: """Test a new Preference object is created if one does not exist.""" + # Test a record is created self.assertFalse(Preference.objects.filter(user=self.user).exists()) preference = Preference.get_user_preference(user=self.user) self.assertTrue(Preference.objects.filter(user=self.user).exists()) From a30f7c56414a98626610a87197a881b5feb433f4 Mon Sep 17 00:00:00 2001 From: djperrefort Date: Mon, 12 Aug 2024 12:41:55 -0400 Subject: [PATCH 3/7] Adds test for handling of missing template variables --- .../{templates => jinja2}/general.html | 0 keystone_api/apps/notifications/shortcuts.py | 11 ++- .../test_send_notification_template.py | 15 +++- keystone_api/main/settings.py | 10 ++- poetry.lock | 88 ++++++++++++++++++- pyproject.toml | 1 + 6 files changed, 119 insertions(+), 6 deletions(-) rename keystone_api/apps/notifications/{templates => jinja2}/general.html (100%) diff --git a/keystone_api/apps/notifications/templates/general.html b/keystone_api/apps/notifications/jinja2/general.html similarity index 100% rename from keystone_api/apps/notifications/templates/general.html rename to keystone_api/apps/notifications/jinja2/general.html diff --git a/keystone_api/apps/notifications/shortcuts.py b/keystone_api/apps/notifications/shortcuts.py index e734bbb9..9c8c4bb0 100644 --- a/keystone_api/apps/notifications/shortcuts.py +++ b/keystone_api/apps/notifications/shortcuts.py @@ -6,7 +6,9 @@ from django.conf import settings from django.core.mail import send_mail -from django.template.loader import render_to_string +from django.template.backends.django import Template +from django.template.base import Template as T +from django.template.loader import get_template, render_to_string from django.utils.html import strip_tags from apps.notifications.models import Notification @@ -63,12 +65,17 @@ def send_notification_template( template: The name of the template file to render. notification_type: Optionally categorize the notification type. notification_metadata: Metadata to store alongside the notification. + + Raises: + UndefinedError: When template variables are not defined in the notification metadata """ + notification_metadata = notification_metadata or dict() + context = {'user': user} context.update(notification_metadata) - html_content = render_to_string(template, context) + html_content = render_to_string(template, context, using='jinja2') text_content = strip_tags(html_content) send_notification( diff --git a/keystone_api/apps/notifications/tests/shortcuts/test_send_notification_template.py b/keystone_api/apps/notifications/tests/shortcuts/test_send_notification_template.py index 74f369d4..6c42245d 100644 --- a/keystone_api/apps/notifications/tests/shortcuts/test_send_notification_template.py +++ b/keystone_api/apps/notifications/tests/shortcuts/test_send_notification_template.py @@ -1,5 +1,5 @@ """Tests for the `send_notification_template` function.""" - +import jinja2 from django.core import mail from django.template.exceptions import TemplateDoesNotExist from django.test import override_settings, TestCase @@ -27,7 +27,7 @@ def setUp(self) -> None: self.subject = 'Test Subject' self.notification_type = Notification.NotificationType.general_message - self.notification_metadata = {'key': 'value'} + self.notification_metadata = {'message': 'this is a message'} def test_email_content(self) -> None: """Test an email notification is sent with the correct content.""" @@ -73,3 +73,14 @@ def test_missing_template(self) -> None: self.notification_type, self.notification_metadata ) + + def test_incomplete_rendering(self) -> None: + """Test an error is raise when a template isn't completely rendered""" + + with self.assertRaises(jinja2.UndefinedError): + send_notification_template( + self.user, + self.subject, + 'general.html', + self.notification_type + ) diff --git a/keystone_api/main/settings.py b/keystone_api/main/settings.py index d6943ba6..3766d01e 100644 --- a/keystone_api/main/settings.py +++ b/keystone_api/main/settings.py @@ -8,6 +8,7 @@ import environ from django.core.management.utils import get_random_secret_key +from jinja2 import StrictUndefined BASE_DIR = Path(__file__).resolve().parent.parent sys.path.insert(0, str(BASE_DIR)) @@ -95,7 +96,7 @@ ] TEMPLATES = [ - { + { # The default backend rquired by Django builtins (e.g., the admin) 'BACKEND': 'django.template.backends.django.DjangoTemplates', 'APP_DIRS': True, 'OPTIONS': { @@ -107,6 +108,13 @@ ], }, }, + { # Jinja2 backend used when rendering user notifications + "BACKEND": "django.template.backends.jinja2.Jinja2", + 'APP_DIRS': True, + "OPTIONS": { + "undefined": StrictUndefined, + }, + }, ] # Base styling for the Admin UI diff --git a/poetry.lock b/poetry.lock index 73a4ad82..f895ef13 100644 --- a/poetry.lock +++ b/poetry.lock @@ -564,6 +564,23 @@ files = [ {file = "inflection-0.5.1.tar.gz", hash = "sha256:1a29730d366e996aaacffb2f1f1cb9593dc38e2ddd30c91250c6dde09ea9b417"}, ] +[[package]] +name = "jinja2" +version = "3.1.4" +description = "A very fast and expressive template engine." +optional = false +python-versions = ">=3.7" +files = [ + {file = "jinja2-3.1.4-py3-none-any.whl", hash = "sha256:bc5dd2abb727a5319567b7a813e6a2e7318c39f4f487cfe6c89c6f9c7d25197d"}, + {file = "jinja2-3.1.4.tar.gz", hash = "sha256:4a3aee7acbbe7303aede8e9648d13b8bf88a429282aa6122a993f0ac800cb369"}, +] + +[package.dependencies] +MarkupSafe = ">=2.0" + +[package.extras] +i18n = ["Babel (>=2.7)"] + [[package]] name = "jsonschema" version = "4.23.0" @@ -631,6 +648,75 @@ sqs = ["boto3 (>=1.26.143)", "pycurl (>=7.43.0.5)", "urllib3 (>=1.26.16)"] yaml = ["PyYAML (>=3.10)"] zookeeper = ["kazoo (>=2.8.0)"] +[[package]] +name = "markupsafe" +version = "2.1.5" +description = "Safely add untrusted strings to HTML/XML markup." +optional = false +python-versions = ">=3.7" +files = [ + {file = "MarkupSafe-2.1.5-cp310-cp310-macosx_10_9_universal2.whl", hash = "sha256:a17a92de5231666cfbe003f0e4b9b3a7ae3afb1ec2845aadc2bacc93ff85febc"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-macosx_10_9_x86_64.whl", hash = "sha256:72b6be590cc35924b02c78ef34b467da4ba07e4e0f0454a2c5907f473fc50ce5"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:e61659ba32cf2cf1481e575d0462554625196a1f2fc06a1c777d3f48e8865d46"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:2174c595a0d73a3080ca3257b40096db99799265e1c27cc5a610743acd86d62f"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:ae2ad8ae6ebee9d2d94b17fb62763125f3f374c25618198f40cbb8b525411900"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-musllinux_1_1_aarch64.whl", hash = "sha256:075202fa5b72c86ad32dc7d0b56024ebdbcf2048c0ba09f1cde31bfdd57bcfff"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-musllinux_1_1_i686.whl", hash = "sha256:598e3276b64aff0e7b3451b72e94fa3c238d452e7ddcd893c3ab324717456bad"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:fce659a462a1be54d2ffcacea5e3ba2d74daa74f30f5f143fe0c58636e355fdd"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-win32.whl", hash = "sha256:d9fad5155d72433c921b782e58892377c44bd6252b5af2f67f16b194987338a4"}, + {file = "MarkupSafe-2.1.5-cp310-cp310-win_amd64.whl", hash = "sha256:bf50cd79a75d181c9181df03572cdce0fbb75cc353bc350712073108cba98de5"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-macosx_10_9_universal2.whl", hash = "sha256:629ddd2ca402ae6dbedfceeba9c46d5f7b2a61d9749597d4307f943ef198fc1f"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:5b7b716f97b52c5a14bffdf688f971b2d5ef4029127f1ad7a513973cfd818df2"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:6ec585f69cec0aa07d945b20805be741395e28ac1627333b1c5b0105962ffced"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:b91c037585eba9095565a3556f611e3cbfaa42ca1e865f7b8015fe5c7336d5a5"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:7502934a33b54030eaf1194c21c692a534196063db72176b0c4028e140f8f32c"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-musllinux_1_1_aarch64.whl", hash = "sha256:0e397ac966fdf721b2c528cf028494e86172b4feba51d65f81ffd65c63798f3f"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-musllinux_1_1_i686.whl", hash = "sha256:c061bb86a71b42465156a3ee7bd58c8c2ceacdbeb95d05a99893e08b8467359a"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:3a57fdd7ce31c7ff06cdfbf31dafa96cc533c21e443d57f5b1ecc6cdc668ec7f"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-win32.whl", hash = "sha256:397081c1a0bfb5124355710fe79478cdbeb39626492b15d399526ae53422b906"}, + {file = "MarkupSafe-2.1.5-cp311-cp311-win_amd64.whl", hash = "sha256:2b7c57a4dfc4f16f7142221afe5ba4e093e09e728ca65c51f5620c9aaeb9a617"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-macosx_10_9_universal2.whl", hash = "sha256:8dec4936e9c3100156f8a2dc89c4b88d5c435175ff03413b443469c7c8c5f4d1"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:3c6b973f22eb18a789b1460b4b91bf04ae3f0c4234a0a6aa6b0a92f6f7b951d4"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ac07bad82163452a6884fe8fa0963fb98c2346ba78d779ec06bd7a6262132aee"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:f5dfb42c4604dddc8e4305050aa6deb084540643ed5804d7455b5df8fe16f5e5"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:ea3d8a3d18833cf4304cd2fc9cbb1efe188ca9b5efef2bdac7adc20594a0e46b"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-musllinux_1_1_aarch64.whl", hash = "sha256:d050b3361367a06d752db6ead6e7edeb0009be66bc3bae0ee9d97fb326badc2a"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-musllinux_1_1_i686.whl", hash = "sha256:bec0a414d016ac1a18862a519e54b2fd0fc8bbfd6890376898a6c0891dd82e9f"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:58c98fee265677f63a4385256a6d7683ab1832f3ddd1e66fe948d5880c21a169"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-win32.whl", hash = "sha256:8590b4ae07a35970728874632fed7bd57b26b0102df2d2b233b6d9d82f6c62ad"}, + {file = "MarkupSafe-2.1.5-cp312-cp312-win_amd64.whl", hash = "sha256:823b65d8706e32ad2df51ed89496147a42a2a6e01c13cfb6ffb8b1e92bc910bb"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:c8b29db45f8fe46ad280a7294f5c3ec36dbac9491f2d1c17345be8e69cc5928f"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ec6a563cff360b50eed26f13adc43e61bc0c04d94b8be985e6fb24b81f6dcfdf"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:a549b9c31bec33820e885335b451286e2969a2d9e24879f83fe904a5ce59d70a"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:4f11aa001c540f62c6166c7726f71f7573b52c68c31f014c25cc7901deea0b52"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-musllinux_1_1_aarch64.whl", hash = "sha256:7b2e5a267c855eea6b4283940daa6e88a285f5f2a67f2220203786dfa59b37e9"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-musllinux_1_1_i686.whl", hash = "sha256:2d2d793e36e230fd32babe143b04cec8a8b3eb8a3122d2aceb4a371e6b09b8df"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-musllinux_1_1_x86_64.whl", hash = "sha256:ce409136744f6521e39fd8e2a24c53fa18ad67aa5bc7c2cf83645cce5b5c4e50"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-win32.whl", hash = "sha256:4096e9de5c6fdf43fb4f04c26fb114f61ef0bf2e5604b6ee3019d51b69e8c371"}, + {file = "MarkupSafe-2.1.5-cp37-cp37m-win_amd64.whl", hash = "sha256:4275d846e41ecefa46e2015117a9f491e57a71ddd59bbead77e904dc02b1bed2"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-macosx_10_9_universal2.whl", hash = "sha256:656f7526c69fac7f600bd1f400991cc282b417d17539a1b228617081106feb4a"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-macosx_10_9_x86_64.whl", hash = "sha256:97cafb1f3cbcd3fd2b6fbfb99ae11cdb14deea0736fc2b0952ee177f2b813a46"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:1f3fbcb7ef1f16e48246f704ab79d79da8a46891e2da03f8783a5b6fa41a9532"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:fa9db3f79de01457b03d4f01b34cf91bc0048eb2c3846ff26f66687c2f6d16ab"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:ffee1f21e5ef0d712f9033568f8344d5da8cc2869dbd08d87c84656e6a2d2f68"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-musllinux_1_1_aarch64.whl", hash = "sha256:5dedb4db619ba5a2787a94d877bc8ffc0566f92a01c0ef214865e54ecc9ee5e0"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-musllinux_1_1_i686.whl", hash = "sha256:30b600cf0a7ac9234b2638fbc0fb6158ba5bdcdf46aeb631ead21248b9affbc4"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-musllinux_1_1_x86_64.whl", hash = "sha256:8dd717634f5a044f860435c1d8c16a270ddf0ef8588d4887037c5028b859b0c3"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-win32.whl", hash = "sha256:daa4ee5a243f0f20d528d939d06670a298dd39b1ad5f8a72a4275124a7819eff"}, + {file = "MarkupSafe-2.1.5-cp38-cp38-win_amd64.whl", hash = "sha256:619bc166c4f2de5caa5a633b8b7326fbe98e0ccbfacabd87268a2b15ff73a029"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-macosx_10_9_universal2.whl", hash = "sha256:7a68b554d356a91cce1236aa7682dc01df0edba8d043fd1ce607c49dd3c1edcf"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:db0b55e0f3cc0be60c1f19efdde9a637c32740486004f20d1cff53c3c0ece4d2"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:3e53af139f8579a6d5f7b76549125f0d94d7e630761a2111bc431fd820e163b8"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:17b950fccb810b3293638215058e432159d2b71005c74371d784862b7e4683f3"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:4c31f53cdae6ecfa91a77820e8b151dba54ab528ba65dfd235c80b086d68a465"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-musllinux_1_1_aarch64.whl", hash = "sha256:bff1b4290a66b490a2f4719358c0cdcd9bafb6b8f061e45c7a2460866bf50c2e"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-musllinux_1_1_i686.whl", hash = "sha256:bc1667f8b83f48511b94671e0e441401371dfd0f0a795c7daa4a3cd1dde55bea"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:5049256f536511ee3f7e1b3f87d1d1209d327e818e6ae1365e8653d7e3abb6a6"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-win32.whl", hash = "sha256:00e046b6dd71aa03a41079792f8473dc494d564611a8f89bbbd7cb93295ebdcf"}, + {file = "MarkupSafe-2.1.5-cp39-cp39-win_amd64.whl", hash = "sha256:fa173ec60341d6bb97a89f5ea19c85c5643c1e7dedebc22f5181eb73573142c5"}, + {file = "MarkupSafe-2.1.5.tar.gz", hash = "sha256:d283d37a890ba4c1ae73ffadf8046435c76e7bc2247bbb63c00bd1a709c6544b"}, +] + [[package]] name = "packaging" version = "24.1" @@ -1141,4 +1227,4 @@ ldap = ["django-auth-ldap"] [metadata] lock-version = "2.0" python-versions = "^3.11" -content-hash = "21e4a82c8908c7eb4a2848cb254b49ad3891b3b0be86913fe611fda42d071074" +content-hash = "dd346b2dfd656b251f7f9492fad39bd719988e307ad43f7705950ad3b85999d2" diff --git a/pyproject.toml b/pyproject.toml index 4a21c351..bbf344d2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,6 +37,7 @@ whitenoise = "6.7.0" # Optional dependencies installed via extras django-auth-ldap = { version = "4.8.0", optional = true } +jinja2 = "^3.1.4" [tool.poetry.extras] ldap = ["django-auth-ldap"] From 7ad2ef1959561f44f24b5631105b39efab4d8ad3 Mon Sep 17 00:00:00 2001 From: djperrefort Date: Mon, 12 Aug 2024 13:21:51 -0400 Subject: [PATCH 4/7] Cleanup from debugging --- keystone_api/apps/notifications/shortcuts.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/keystone_api/apps/notifications/shortcuts.py b/keystone_api/apps/notifications/shortcuts.py index 9c8c4bb0..fc169aea 100644 --- a/keystone_api/apps/notifications/shortcuts.py +++ b/keystone_api/apps/notifications/shortcuts.py @@ -6,9 +6,7 @@ from django.conf import settings from django.core.mail import send_mail -from django.template.backends.django import Template -from django.template.base import Template as T -from django.template.loader import get_template, render_to_string +from django.template.loader import render_to_string from django.utils.html import strip_tags from apps.notifications.models import Notification From a51730c216be87dbac7da57eae09f0828340d070 Mon Sep 17 00:00:00 2001 From: djperrefort Date: Mon, 12 Aug 2024 15:30:13 -0400 Subject: [PATCH 5/7] Separates template context from metadata --- keystone_api/apps/allocations/tasks.py | 7 ++- .../templates/expiration_email.html | 4 +- keystone_api/apps/notifications/shortcuts.py | 17 +++--- .../test_send_notification_template.py | 56 ++++++++++--------- 4 files changed, 44 insertions(+), 40 deletions(-) diff --git a/keystone_api/apps/allocations/tasks.py b/keystone_api/apps/allocations/tasks.py index 06a7a420..b773d239 100644 --- a/keystone_api/apps/allocations/tasks.py +++ b/keystone_api/apps/allocations/tasks.py @@ -167,11 +167,14 @@ def send_expiry_notification_for_request(user: User, request: AllocationRequest) user=user, subject=f'Allocation Expires on {request.expire}', template='expiration_email.html', + context={ + 'user': user, + 'request': request, + 'days_to_expire': days_until_expire + }, notification_type=Notification.NotificationType.request_status, notification_metadata={ 'request_id': request.id, - 'request_title': request.title, - 'request_expire': request.expire.isoformat(), 'days_to_expire': days_until_expire } ) diff --git a/keystone_api/apps/allocations/templates/expiration_email.html b/keystone_api/apps/allocations/templates/expiration_email.html index c2fbb87e..14c2373c 100644 --- a/keystone_api/apps/allocations/templates/expiration_email.html +++ b/keystone_api/apps/allocations/templates/expiration_email.html @@ -4,8 +4,8 @@

Dear {{ user.username }},

{% endif %}

- This notification is to remind you your HPC compute allocation "{{ request_title }}" - is set to expire in {{ days_to_expire }} days on {{ request_expire }}. + This notification is to remind you your HPC compute allocation "{{ request.title }}" + is set to expire in {{ days_to_expire }} days on {{ request.expire.isoformat() }}.

Sincerely,

The keystone HPC utility

diff --git a/keystone_api/apps/notifications/shortcuts.py b/keystone_api/apps/notifications/shortcuts.py index fc169aea..301ab700 100644 --- a/keystone_api/apps/notifications/shortcuts.py +++ b/keystone_api/apps/notifications/shortcuts.py @@ -18,8 +18,8 @@ def send_notification( subject: str, plain_text: str, html_text: str, - notification_type: Notification.NotificationType = None, - notification_metadata: dict = None + notification_type: Notification.NotificationType, + notification_metadata: dict | None = None ) -> None: """Send a notification email to a specified user with both plain text and HTML content. @@ -52,8 +52,9 @@ def send_notification_template( user: User, subject: str, template: str, - notification_type: Notification.NotificationType = None, - notification_metadata: dict = None + context: dict, + notification_type: Notification.NotificationType, + notification_metadata: dict | None = None ) -> None: """Render an email template and send it to a specified user. @@ -61,6 +62,7 @@ def send_notification_template( user: The user object to whom the email will be sent. subject: The subject line of the email. template: The name of the template file to render. + context: Variable definitions used to populate the template. notification_type: Optionally categorize the notification type. notification_metadata: Metadata to store alongside the notification. @@ -68,11 +70,6 @@ def send_notification_template( UndefinedError: When template variables are not defined in the notification metadata """ - notification_metadata = notification_metadata or dict() - - context = {'user': user} - context.update(notification_metadata) - html_content = render_to_string(template, context, using='jinja2') text_content = strip_tags(html_content) @@ -100,5 +97,5 @@ def send_general_notification(user: User, subject: str, message: str) -> None: subject=subject, template='general.html', notification_type=Notification.NotificationType.general_message, - notification_metadata={'message': message} + context={'user': user, 'message': message} ) diff --git a/keystone_api/apps/notifications/tests/shortcuts/test_send_notification_template.py b/keystone_api/apps/notifications/tests/shortcuts/test_send_notification_template.py index 6c42245d..3ff7f9a4 100644 --- a/keystone_api/apps/notifications/tests/shortcuts/test_send_notification_template.py +++ b/keystone_api/apps/notifications/tests/shortcuts/test_send_notification_template.py @@ -1,4 +1,5 @@ -"""Tests for the `send_notification_template` function.""" +"""Unit tests for the `send_notification_template` function.""" + import jinja2 from django.core import mail from django.template.exceptions import TemplateDoesNotExist @@ -25,42 +26,44 @@ def setUp(self) -> None: password='foobar123' ) - self.subject = 'Test Subject' - self.notification_type = Notification.NotificationType.general_message - self.notification_metadata = {'message': 'this is a message'} - def test_email_content(self) -> None: """Test an email notification is sent with the correct content.""" + subject = 'Test subject' + send_notification_template( self.user, - self.subject, - 'general.html', - self.notification_type, - self.notification_metadata + subject, + template='general.html', + context={"user": self.user, "message": "this is a message"}, + notification_type=Notification.NotificationType.general_message ) self.assertEqual(len(mail.outbox), 1) email = mail.outbox[0] - self.assertEqual(email.subject, self.subject) - self.assertEqual(email.from_email, settings.EMAIL_FROM_ADDRESS) - self.assertEqual(email.to, [self.user.email]) + self.assertEqual(subject, email.subject) + self.assertEqual(settings.EMAIL_FROM_ADDRESS, email.from_email) + self.assertEqual([self.user.email], email.to) def test_database_is_updated(self) -> None: """Test a record of the email is stored in the database.""" + notification_type = Notification.NotificationType.general_message + notification_metadata = {'key': 'value'} + send_notification_template( self.user, - self.subject, - 'general.html', - self.notification_type, - self.notification_metadata + "Test subject", + template='general.html', + context={"user": self.user, "message": "this is a message"}, + notification_type=notification_type, + notification_metadata=notification_metadata ) notification = Notification.objects.get(user=self.user) - self.assertEqual(notification.notification_type, self.notification_type) - self.assertEqual(notification.metadata, self.notification_metadata) + self.assertEqual(notification_type, notification.notification_type) + self.assertEqual(notification_metadata, notification.metadata) def test_missing_template(self) -> None: """Test an error is raised when a template is not found.""" @@ -68,19 +71,20 @@ def test_missing_template(self) -> None: with self.assertRaises(TemplateDoesNotExist): send_notification_template( self.user, - self.subject, - 'this_template_does_not_exist', - self.notification_type, - self.notification_metadata + "Test subject", + template='this_template_does_not_exist', + context=dict(), + notification_type=Notification.NotificationType.general_message ) def test_incomplete_rendering(self) -> None: - """Test an error is raise when a template isn't completely rendered""" + """Test an error is raise when a template isn't completely rendered.""" with self.assertRaises(jinja2.UndefinedError): send_notification_template( self.user, - self.subject, - 'general.html', - self.notification_type + "Test subject", + template='general.html', + context=dict(), + notification_type=Notification.NotificationType.general_message ) From 1f14144ff6e80c7bb95c9f5b10a3a7b1d00bba07 Mon Sep 17 00:00:00 2001 From: djperrefort Date: Mon, 12 Aug 2024 15:32:19 -0400 Subject: [PATCH 6/7] Updates email notifications for allocations app --- .../expiration_email.html | 0 keystone_api/apps/allocations/tasks.py | 34 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) rename keystone_api/apps/allocations/{templates => jinja2}/expiration_email.html (100%) diff --git a/keystone_api/apps/allocations/templates/expiration_email.html b/keystone_api/apps/allocations/jinja2/expiration_email.html similarity index 100% rename from keystone_api/apps/allocations/templates/expiration_email.html rename to keystone_api/apps/allocations/jinja2/expiration_email.html diff --git a/keystone_api/apps/allocations/tasks.py b/keystone_api/apps/allocations/tasks.py index b773d239..b83c2f89 100644 --- a/keystone_api/apps/allocations/tasks.py +++ b/keystone_api/apps/allocations/tasks.py @@ -160,24 +160,24 @@ def send_expiry_notification_for_request(user: User, request: AllocationRequest) if notification_sent: log.debug(f'Existing notification found.') + return - else: - log.debug(f'Sending new notification for request #{request.id} to user {user.username}.') - send_notification_template( - user=user, - subject=f'Allocation Expires on {request.expire}', - template='expiration_email.html', - context={ - 'user': user, - 'request': request, - 'days_to_expire': days_until_expire - }, - notification_type=Notification.NotificationType.request_status, - notification_metadata={ - 'request_id': request.id, - 'days_to_expire': days_until_expire - } - ) + log.debug(f'Sending new notification for request #{request.id} to user {user.username}.') + send_notification_template( + user=user, + subject=f'Allocation Expires on {request.expire}', + template='expiration_email.html', + context={ + 'user': user, + 'request': request, + 'days_to_expire': days_until_expire + }, + notification_type=Notification.NotificationType.request_status, + notification_metadata={ + 'request_id': request.id, + 'days_to_expire': days_until_expire + } + ) @shared_task() From 8cfe9b3802ee805cd116615980fb94886164316d Mon Sep 17 00:00:00 2001 From: djperrefort Date: Mon, 12 Aug 2024 15:58:57 -0400 Subject: [PATCH 7/7] Update assert formatting --- .../tests/shortcuts/test_send_notification.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/keystone_api/apps/notifications/tests/shortcuts/test_send_notification.py b/keystone_api/apps/notifications/tests/shortcuts/test_send_notification.py index 4bf45707..9104edae 100644 --- a/keystone_api/apps/notifications/tests/shortcuts/test_send_notification.py +++ b/keystone_api/apps/notifications/tests/shortcuts/test_send_notification.py @@ -44,16 +44,16 @@ def test_email_content(self) -> None: self.assertEqual(len(mail.outbox), 1) email = mail.outbox[0] - self.assertEqual(email.subject, self.subject) - self.assertEqual(email.body, self.plain_text) - self.assertEqual(email.from_email, settings.EMAIL_FROM_ADDRESS) - self.assertEqual(email.to, [self.user.email]) - self.assertEqual(email.alternatives, [(self.html_text, 'text/html')]) + self.assertEqual(self.subject, email.subject) + self.assertEqual(self.plain_text, email.body) + self.assertEqual(settings.EMAIL_FROM_ADDRESS, email.from_email) + self.assertEqual([self.user.email], email.to) + self.assertEqual([(self.html_text, 'text/html')], email.alternatives) def test_database_is_updated(self) -> None: """Test a record of the email is stored in the database""" notification = Notification.objects.get(user=self.user) - self.assertEqual(notification.message, self.plain_text) - self.assertEqual(notification.notification_type, self.notification_type) - self.assertEqual(notification.metadata, self.notification_metadata) + self.assertEqual(self.plain_text, notification.message) + self.assertEqual(self.notification_type, notification.notification_type) + self.assertEqual(self.notification_metadata, notification.metadata)