From 1f6f8738609d18d040ea3f25377c647975f1c08a Mon Sep 17 00:00:00 2001 From: Piotr Banaszkiewicz Date: Sun, 19 Nov 2023 22:04:11 +0100 Subject: [PATCH] [#2540] Fix bug by removing a feature Bug was first reported by @elichad in https://github.com/carpentries/amy/pull/2551#pullrequestreview-1737378977 The bug was that page didn't render because of some weird Python inspection stuff. As it turned out, that inspection was happening in Django template system when trying to display base class for Python date. It was not possible to get rid of that error in a sane matter, so I decided to remove the feature linking models with their documentation. --- amy/emails/templatetags/emails.py | 20 ---------- amy/emails/tests/test_template_tags.py | 39 +------------------ amy/emails/tests/test_views.py | 4 +- amy/emails/views.py | 8 +++- .../emails/email_template_detail.html | 3 +- amy/templates/emails/email_template_edit.html | 3 +- 6 files changed, 11 insertions(+), 66 deletions(-) diff --git a/amy/emails/templatetags/emails.py b/amy/emails/templatetags/emails.py index 64edf9168..e571b3d48 100644 --- a/amy/emails/templatetags/emails.py +++ b/amy/emails/templatetags/emails.py @@ -1,30 +1,10 @@ from django import template -from django.db.models import Model from emails.models import ScheduledEmailStatus, ScheduledEmailStatusActions register = template.Library() -@register.filter -def model_documentation_link(model: Model) -> str: - # This is a limited mapping of model to its documentation link. It should - # be expanded as needed. - # If our model documentation grows, we should consider including link to model - # documentation inside every model. Then this mapping would become obsolete. - mapping = { - "InstructorRecruitmentSignup": ( - "https://carpentries.github.io/amy/design/database_models/" - "#instructorrecruitmentsignup" - ), - "Event": "https://carpentries.github.io/amy/amy_database_structure/#events", - "Award": "https://carpentries.github.io/amy/design/database_models/#award", - "Person": "https://carpentries.github.io/amy/amy_database_structure/#persons", - } - model_name = model.__class__.__name__ - return mapping.get(model_name, "") - - @register.simple_tag def allowed_actions_for_status(status: ScheduledEmailStatus) -> list[str]: return [ diff --git a/amy/emails/tests/test_template_tags.py b/amy/emails/tests/test_template_tags.py index 49f19f064..01589c831 100644 --- a/amy/emails/tests/test_template_tags.py +++ b/amy/emails/tests/test_template_tags.py @@ -1,44 +1,7 @@ -from unittest.mock import MagicMock - from django.test import TestCase from emails.models import ScheduledEmailStatus -from emails.templatetags.emails import ( - allowed_actions_for_status, - model_documentation_link, -) - - -class TestModelDocumentationLink(TestCase): - def test_model_documentation_link__valid_model(self) -> None: - # Arrange - # Real models are replaced with their metaclass BaseModel, so the tests don't - # work with real models and instead mocks are used. - model = MagicMock() - model.__class__.__name__ = "Person" - - # Act - link = model_documentation_link(model) - - # Assert - self.assertEqual( - link, - "https://carpentries.github.io/amy/amy_database_structure/#persons", - ) - - def test_model_documentation_link__invalid_model(self) -> None: - # Arrange - # Real models are replaced with their metaclass BaseModel, so the tests don't - # work with real models and instead mocks are used. - model = MagicMock() - # Badge is not in the mapping yet - model.__class__.__name__ = "Badge" - - # Act - link = model_documentation_link(model) - - # Assert - self.assertEqual(link, "") +from emails.templatetags.emails import allowed_actions_for_status class TestAllowedActionsForStatusTag(TestCase): diff --git a/amy/emails/tests/test_views.py b/amy/emails/tests/test_views.py index f235fbe12..4dd956589 100644 --- a/amy/emails/tests/test_views.py +++ b/amy/emails/tests/test_views.py @@ -85,7 +85,7 @@ def test_view(self) -> None: self.assertEqual( rv.context["body_context_annotations"], { - "person": Person, + "person": repr(Person), }, ) @@ -147,7 +147,7 @@ def test_view_context(self) -> None: self.assertEqual( rv.context["body_context_annotations"], { - "person": Person, + "person": repr(Person), }, ) diff --git a/amy/emails/views.py b/amy/emails/views.py index bac17b153..1bd711f06 100644 --- a/amy/emails/views.py +++ b/amy/emails/views.py @@ -63,7 +63,9 @@ def get_context_data(self, **kwargs): context["body_context_annotations"] = {} if signal: context["body_context_type"] = signal.context_type - context["body_context_annotations"] = signal.context_type.__annotations__ + context["body_context_annotations"] = { + k: repr(v) for k, v in signal.context_type.__annotations__.items() + } return context @@ -96,7 +98,9 @@ def get_context_data(self, **kwargs): context["body_context_annotations"] = {} if signal: context["body_context_type"] = signal.context_type - context["body_context_annotations"] = signal.context_type.__annotations__ + context["body_context_annotations"] = { + k: repr(v) for k, v in signal.context_type.__annotations__.items() + } return context diff --git a/amy/templates/emails/email_template_detail.html b/amy/templates/emails/email_template_detail.html index 0fb18aa28..252bb1348 100644 --- a/amy/templates/emails/email_template_detail.html +++ b/amy/templates/emails/email_template_detail.html @@ -1,5 +1,4 @@ {% extends "base_nav.html" %} -{% load emails %} {% block content %} @@ -102,7 +101,7 @@ diff --git a/amy/templates/emails/email_template_edit.html b/amy/templates/emails/email_template_edit.html index 28782bba8..60e5fee80 100644 --- a/amy/templates/emails/email_template_edit.html +++ b/amy/templates/emails/email_template_edit.html @@ -1,7 +1,6 @@ {% extends "base_nav.html" %} {% load crispy_forms_tags %} -{% load emails %} {% block content %} @@ -14,7 +13,7 @@