From c2bf001d3cd40227b6ccc48293e7ce0149d6476f Mon Sep 17 00:00:00 2001 From: Andy Mitchell <326561+Themitchell@users.noreply.github.com> Date: Mon, 16 Feb 2026 08:18:48 +0000 Subject: [PATCH 1/5] Ensure full coverage of auth --- lung_cancer_screening/questions/auth.py | 2 -- .../questions/tests/unit/test_auth.py | 10 ++++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lung_cancer_screening/questions/auth.py b/lung_cancer_screening/questions/auth.py index c9278054..f9e3b1fc 100644 --- a/lung_cancer_screening/questions/auth.py +++ b/lung_cancer_screening/questions/auth.py @@ -92,8 +92,6 @@ def _create_client_assertion(self): headers=headers ) - if isinstance(assertion, bytes): - return assertion.decode('utf-8') return assertion def get_token(self, token_payload): diff --git a/lung_cancer_screening/questions/tests/unit/test_auth.py b/lung_cancer_screening/questions/tests/unit/test_auth.py index 7ce461b3..bfced441 100644 --- a/lung_cancer_screening/questions/tests/unit/test_auth.py +++ b/lung_cancer_screening/questions/tests/unit/test_auth.py @@ -99,6 +99,16 @@ def test_update_user_updates_the_user(self): self.assertEqual(user.nhs_number, '1234567890') + + def test_get_token_raisees_if_private_key_not_parsable(self): + settings.OIDC_RP_CLIENT_PRIVATE_KEY = "invalid" + + with self.assertRaises(ValueError) as context: + self.backend.get_token({'code': 'auth-code-123'}) + + self.assertIn("Failed to load private key", str(context.exception)) + + @patch('lung_cancer_screening.questions.auth.requests.post') def test_get_token_success(self, mock_post): settings.OIDC_RP_CLIENT_PRIVATE_KEY = self.test_private_key_pem From e446c7f188dd360d88ac293a0ed30d23df4fb739 Mon Sep 17 00:00:00 2001 From: Andy Mitchell <326561+Themitchell@users.noreply.github.com> Date: Mon, 16 Feb 2026 08:41:09 +0000 Subject: [PATCH 2/5] Add validation tests for height --- .../tests/unit/models/test_height_response.py | 111 ++++++++++++++++-- 1 file changed, 101 insertions(+), 10 deletions(-) diff --git a/lung_cancer_screening/questions/tests/unit/models/test_height_response.py b/lung_cancer_screening/questions/tests/unit/models/test_height_response.py index b174aad8..4fd44c56 100644 --- a/lung_cancer_screening/questions/tests/unit/models/test_height_response.py +++ b/lung_cancer_screening/questions/tests/unit/models/test_height_response.py @@ -1,4 +1,5 @@ from django.test import TestCase, tag +from django.core.exceptions import ValidationError from ...factories.response_set_factory import ResponseSetFactory from ...factories.height_response_factory import HeightResponseFactory @@ -17,28 +18,118 @@ def test_has_a_valid_factory(self): def test_has_response_set_as_foreign_key(self): - response_set = ResponseSetFactory() - response = HeightResponse.objects.create( - response_set=response_set, + response = HeightResponseFactory.build( + response_set=self.response_set, metric=1700 ) - self.assertEqual(response.response_set, response_set) + self.assertEqual(response.response_set, self.response_set) + def test_has_metric_as_int(self): - response_set = ResponseSetFactory() - response = HeightResponse.objects.create( - response_set=response_set, + response = HeightResponseFactory.build( + response_set=self.response_set, metric=1700 ) self.assertIsInstance(response.metric, int) def test_has_imperial_as_int(self): - response_set = ResponseSetFactory() - response = HeightResponse.objects.create( - response_set=response_set, + response = HeightResponseFactory.build( + response_set=self.response_set, imperial=68 ) self.assertIsInstance(response.imperial, int) + + + def test_is_invalid_if_neither_metric_nor_imperial_are_set(self): + response = HeightResponseFactory.build( + response_set=self.response_set, + metric=None, + imperial=None + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Either metric or imperial height must be provided.", + context.exception.messages, + ) + + + def test_is_invalid_if_both_metric_and_imperial_are_set(self): + response = HeightResponseFactory.build( + response_set=self.response_set, + metric=1700, + imperial=68 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Cannot provide both metric and imperial height.", + context.exception.messages, + ) + + + def test_is_invalid_if_metric_is_less_than_minimum_height(self): + response = HeightResponseFactory.build( + response_set=self.response_set, + metric=HeightResponse.MIN_HEIGHT_METRIC - 1 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Height must be between 139.7cm and 243.8 cm", + context.exception.messages, + ) + + + def test_is_invalid_if_metric_is_greater_than_maximum_height(self): + response = HeightResponseFactory.build( + response_set=self.response_set, + metric=HeightResponse.MAX_HEIGHT_METRIC + 1 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Height must be between 139.7cm and 243.8 cm", + context.exception.messages, + ) + + + def test_is_invalid_if_imperial_is_less_than_minimum_height(self): + response = HeightResponseFactory.build( + response_set=self.response_set, + imperial=HeightResponse.MIN_HEIGHT_IMPERIAL - 1 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Height must be between 4 feet 7 inches and 8 feet", + context.exception.messages, + ) + + + def test_is_invalid_if_imperial_is_greater_than_maximum_height(self): + response = HeightResponseFactory.build( + response_set=self.response_set, + imperial=HeightResponse.MAX_HEIGHT_IMPERIAL + 1 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Height must be between 4 feet 7 inches and 8 feet", + context.exception.messages, + ) From e427236492e9682291dd8ecf6b6bf6f70fd17ef5 Mon Sep 17 00:00:00 2001 From: Andy Mitchell <326561+Themitchell@users.noreply.github.com> Date: Mon, 16 Feb 2026 08:46:22 +0000 Subject: [PATCH 3/5] Add validation tests for weight --- .../tests/unit/models/test_weight_response.py | 106 ++++++++++++++++-- 1 file changed, 99 insertions(+), 7 deletions(-) diff --git a/lung_cancer_screening/questions/tests/unit/models/test_weight_response.py b/lung_cancer_screening/questions/tests/unit/models/test_weight_response.py index 5a0a96d4..a10bbfd0 100644 --- a/lung_cancer_screening/questions/tests/unit/models/test_weight_response.py +++ b/lung_cancer_screening/questions/tests/unit/models/test_weight_response.py @@ -1,4 +1,5 @@ from django.test import TestCase, tag +from django.core.exceptions import ValidationError from ...factories.response_set_factory import ResponseSetFactory from ...factories.weight_response_factory import WeightResponseFactory @@ -18,28 +19,119 @@ def test_has_a_valid_factory(self): def test_has_response_set_as_foreign_key(self): - response_set = ResponseSetFactory() response = WeightResponse.objects.create( - response_set=response_set, + response_set=self.response_set, metric=680 ) - self.assertEqual(response.response_set, response_set) + self.assertEqual(response.response_set, self.response_set) + def test_has_metric_as_int(self): - response_set = ResponseSetFactory() response = WeightResponse.objects.create( - response_set=response_set, + response_set=self.response_set, metric=680 ) self.assertIsInstance(response.metric, int) + def test_has_imperial_as_int(self): - response_set = ResponseSetFactory() response = WeightResponse.objects.create( - response_set=response_set, + response_set=self.response_set, imperial=140 ) self.assertIsInstance(response.imperial, int) + + + def test_is_invalid_if_neither_metric_nor_imperial_are_set(self): + response = WeightResponseFactory.build( + response_set=self.response_set, + metric=None, + imperial=None + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Either metric or imperial weight must be provided.", + context.exception.messages, + ) + + + def test_is_invalid_if_both_metric_and_imperial_are_set(self): + response = WeightResponseFactory.build( + response_set=self.response_set, + metric=1700, + imperial=68 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Cannot provide both metric and imperial weight.", + context.exception.messages, + ) + + + def test_is_invalid_if_metric_is_less_than_minimum_weight(self): + response = WeightResponseFactory.build( + response_set=self.response_set, + metric=WeightResponse.MIN_WEIGHT_METRIC - 1 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Weight must be between 25.4kg and 317.5kg", + context.exception.messages, + ) + + + def test_is_invalid_if_metric_is_greater_than_maximum_weight(self): + response = WeightResponseFactory.build( + response_set=self.response_set, + metric=WeightResponse.MAX_WEIGHT_METRIC + 1 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Weight must be between 25.4kg and 317.5kg", + context.exception.messages, + ) + + + def test_is_invalid_if_imperial_is_less_than_minimum_weight(self): + response = WeightResponseFactory.build( + response_set=self.response_set, + imperial=WeightResponse.MIN_WEIGHT_IMPERIAL - 1 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Weight must be between 4 stone and 50 stone", + context.exception.messages, + ) + + + def test_is_invalid_if_imperial_is_greater_than_maximum_weight(self): + response = WeightResponseFactory.build( + response_set=self.response_set, + imperial=WeightResponse.MAX_WEIGHT_IMPERIAL + 1 + ) + + with self.assertRaises(ValidationError) as context: + response.full_clean() + + self.assertIn( + "Weight must be between 4 stone and 50 stone", + context.exception.messages, + ) From 41dd165677595dc43207de49d0cdc4a7a6c449dd Mon Sep 17 00:00:00 2001 From: Andy Mitchell <326561+Themitchell@users.noreply.github.com> Date: Mon, 16 Feb 2026 08:52:07 +0000 Subject: [PATCH 4/5] Add test for tobacco smoking history save early exit If the form is not valid, ensure that no records are modified --- .../forms/test_types_tobacco_smoking_form.py | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lung_cancer_screening/questions/tests/unit/forms/test_types_tobacco_smoking_form.py b/lung_cancer_screening/questions/tests/unit/forms/test_types_tobacco_smoking_form.py index 55f406fb..1ba3e318 100644 --- a/lung_cancer_screening/questions/tests/unit/forms/test_types_tobacco_smoking_form.py +++ b/lung_cancer_screening/questions/tests/unit/forms/test_types_tobacco_smoking_form.py @@ -51,6 +51,37 @@ def test_is_invalid_when_no_option_is_selected(self): ["Select the type of tobacco you smoke or have smoked"], ) + def test_does_not_save_smoking_history_if_invalid(self): + form = TypesTobaccoSmokingForm( + response_set=self.response_set, + data={ + "value": ["invalid"] + } + ) + + form.save() + + self.assertEqual(self.response_set.tobacco_smoking_history.count(), 0) + + + def test_does_not_delete_smoking_history_if_invalid(self): + history = TobaccoSmokingHistoryFactory( + response_set=self.response_set, + type=TobaccoSmokingHistoryTypes.CIGARETTES + ) + + form = TypesTobaccoSmokingForm( + response_set=self.response_set, + data={ + "value": ["invalid"] + } + ) + + form.save() + + self.assertEqual(self.response_set.tobacco_smoking_history.first(), history) + + def test_saves_an_tobacco_smoking_type_for_each_value_selected(self): form = TypesTobaccoSmokingForm( response_set=self.response_set, From 851149f9b2ab825e42cc28b922e0114688b0e638 Mon Sep 17 00:00:00 2001 From: Andy Mitchell <326561+Themitchell@users.noreply.github.com> Date: Tue, 17 Feb 2026 13:46:36 +0000 Subject: [PATCH 5/5] Use ALLOW_LOGOUT_GET_METHOD instead of overwriting logout view --- lung_cancer_screening/core/jinja2/layout.jinja | 2 +- lung_cancer_screening/questions/urls.py | 2 -- lung_cancer_screening/questions/views/logout.py | 6 ------ lung_cancer_screening/settings.py | 3 +-- 4 files changed, 2 insertions(+), 11 deletions(-) delete mode 100644 lung_cancer_screening/questions/views/logout.py diff --git a/lung_cancer_screening/core/jinja2/layout.jinja b/lung_cancer_screening/core/jinja2/layout.jinja index f26fbdc8..6eea4667 100644 --- a/lung_cancer_screening/core/jinja2/layout.jinja +++ b/lung_cancer_screening/core/jinja2/layout.jinja @@ -25,7 +25,7 @@ }, { "text": "Log out", - "href": url("questions:logout") + "href": url("oidc_logout") } ] if request.user.is_authenticated else [] } diff --git a/lung_cancer_screening/questions/urls.py b/lung_cancer_screening/questions/urls.py index ec2643c0..4f112173 100644 --- a/lung_cancer_screening/questions/urls.py +++ b/lung_cancer_screening/questions/urls.py @@ -45,7 +45,6 @@ from .views.start import StartView from .views.weight import WeightView from .views.confirmation import ConfirmationView -from .views.logout import LogoutView urlpatterns = [ path('', RedirectView.as_view(url='/start'), name='root'), @@ -78,5 +77,4 @@ path('start', StartView.as_view(), name='start'), path('weight', WeightView.as_view(), name='weight'), path('confirmation', ConfirmationView.as_view(), name='confirmation'), - path('logout', LogoutView.as_view(), name='logout'), ] diff --git a/lung_cancer_screening/questions/views/logout.py b/lung_cancer_screening/questions/views/logout.py deleted file mode 100644 index b2afdf7b..00000000 --- a/lung_cancer_screening/questions/views/logout.py +++ /dev/null @@ -1,6 +0,0 @@ -from mozilla_django_oidc.views import OIDCLogoutView - - -class LogoutView(OIDCLogoutView): - def get(self, request): - return self.post(request) diff --git a/lung_cancer_screening/settings.py b/lung_cancer_screening/settings.py index b341df26..db5363cb 100644 --- a/lung_cancer_screening/settings.py +++ b/lung_cancer_screening/settings.py @@ -252,9 +252,8 @@ def list_env(key): LOGIN_URL = '/oidc/authenticate/' LOGIN_REDIRECT_URL = '/' LOGOUT_REDIRECT_URL = '/' - - LOGIN_REDIRECT_URL_FAILURE = "/agree-to-share-information" +ALLOW_LOGOUT_GET_METHOD = True # Additional security settings for production if not DEBUG: