From b2f215e01089e9b5f5c0b4785829070ba1536e1f Mon Sep 17 00:00:00 2001 From: Jamie Falcus <50366804+jamiefalcus@users.noreply.github.com> Date: Tue, 17 Feb 2026 16:04:26 +0000 Subject: [PATCH] PPHA-535: Smoking change page updates --- features/smoking_change.feature | 14 ++--- features/smoking_current.feature | 2 +- features/steps/preflight_steps.py | 4 ++ .../questions/forms/smoking_change_form.py | 17 +++++-- .../models/tobacco_smoking_history.py | 1 - .../tobacco_smoking_history_factory.py | 16 ++++++ .../unit/forms/test_smoking_change_form.py | 51 +++++++------------ .../tests/unit/views/test_smoking_change.py | 24 +++++++++ .../mixins/ensure_smoking_history_for_type.py | 10 +++- .../questions/views/smoking_change.py | 19 +++++-- .../questions/views/smoking_current.py | 2 +- 11 files changed, 108 insertions(+), 52 deletions(-) diff --git a/features/smoking_change.feature b/features/smoking_change.feature index 47033597..49417f04 100644 --- a/features/smoking_change.feature +++ b/features/smoking_change.feature @@ -3,18 +3,18 @@ Feature: Smoking change page Scenario: The page is accessible Given I am logged in And I have answered questions showing I am eligible - And I have answered questions showing I have smoked "Cigarettes" + And I have answered questions showing I have smoked 10 "Cigarettes" "daily" When I go to "/cigarettes-smoking-change" Then there are no accessibility violations Scenario: Form errors Given I am logged in And I have answered questions showing I am eligible - And I have answered questions showing I have smoked "Cigarettes" + And I have answered questions showing I have smoked 10 "Cigarettes" "daily" When I go to "/cigarettes-smoking-change" And I submit the form Then I see a form error "Select if the number of cigarettes you smoke has changed over time" - When I check "Yes, I used to smoke more" + When I check "Yes, I used to smoke more than 10 cigarettes a day" And I check "No, it has not changed" And I submit the form Then I see a form error "Select if the number of cigarettes you smoke has changed over time, or select 'no, it has not changed'" @@ -23,7 +23,7 @@ Feature: Smoking change page Scenario: Navigating backwards and forwards Given I am logged in And I have answered questions showing I am eligible - And I have answered questions showing I have smoked "Cigarettes" + And I have answered questions showing I have smoked 10 "Cigarettes" "daily" When I go to "/cigarettes-smoking-change" Then I see a back link to "/cigarettes-smoked-amount" When I check "No, it has not changed" @@ -32,14 +32,14 @@ Feature: Smoking change page When I click "Back" Then I am on "/cigarettes-smoking-change" When I uncheck "No, it has not changed" - And I check "Yes, I used to smoke more" + And I check "Yes, I used to smoke more than 10 cigarettes a day" And I submit the form Then I am on "/check-your-answers" Scenario: Checking responses and changing them Given I am logged in And I have answered questions showing I am eligible - And I have answered questions showing I have smoked "Cigarettes" + And I have answered questions showing I have smoked 10 "Cigarettes" "daily" When I go to "/cigarettes-smoking-change" And I check "No, it has not changed" And I submit the form @@ -49,7 +49,7 @@ Feature: Smoking change page When I click the link to change "Has the number of cigarettes you normally smoke changed over time?" under "Smoking history" Then I am on "/cigarettes-smoking-change?change=True" And I see "No, it has not changed" selected - When I check "Yes, I used to smoke more" + When I check "Yes, I used to smoke more than 10 cigarettes a day" And I uncheck "No, it has not changed" And I submit the form Then I am on "/check-your-answers" diff --git a/features/smoking_current.feature b/features/smoking_current.feature index e78ef17f..22a7efd1 100644 --- a/features/smoking_current.feature +++ b/features/smoking_current.feature @@ -22,7 +22,7 @@ Feature: Smoking current page And I have answered questions showing I have smoked for "10" years And I have answered questions showing I have smoked "Cigarettes" When I go to "/cigarettes-smoking-current" - Then I see a back link to "/age-when-started-smoking" + Then I see a back link to "/types-tobacco-smoking" When I check "Yes" and submit Then I am on "/cigarettes-smoked-total-years" diff --git a/features/steps/preflight_steps.py b/features/steps/preflight_steps.py index 47b3cc28..4d16a947 100644 --- a/features/steps/preflight_steps.py +++ b/features/steps/preflight_steps.py @@ -99,6 +99,10 @@ def given_i_have_answered_questions_showing_i_have_smoked_tobacco_type_frequency when_i_check_label(context, humanize(frequency)) when_i_submit_the_form(context) +@given('I have answered questions showing I have smoked {amount} "{tobacco_type}" "{frequency}"') +def i_have_answered_questions_showing_i_have_smoked_amount_tobacco_type_frequency(context, amount, tobacco_type, frequency): + given_i_have_answered_questions_showing_i_have_smoked_tobacco_type_frequency(context, tobacco_type, frequency) + given_i_have_answered_questions_showing_i_have_smoked_amount_tobacco_type(context, amount, tobacco_type) @given('I have answered questions showing I have smoked {amount} "{tobacco_type}" as the amount') def given_i_have_answered_questions_showing_i_have_smoked_amount_tobacco_type(context, amount, tobacco_type): diff --git a/lung_cancer_screening/questions/forms/smoking_change_form.py b/lung_cancer_screening/questions/forms/smoking_change_form.py index e2f93bba..fa2554c1 100644 --- a/lung_cancer_screening/questions/forms/smoking_change_form.py +++ b/lung_cancer_screening/questions/forms/smoking_change_form.py @@ -1,16 +1,16 @@ from django import forms -from inflection import camelize from ...nhsuk_forms.choice_field import MultipleChoiceField from ..models.tobacco_smoking_history import TobaccoSmokingHistory class SmokingChangeForm(forms.Form): - def __init__(self, *args, response_set, tobacco_type, **kwargs): + def __init__(self, *args, response_set, tobacco_smoking_history_item, **kwargs): super().__init__(*args, **kwargs) self.response_set = response_set - self.tobacco_type = camelize(tobacco_type) + self.tobacco_smoking_history = tobacco_smoking_history_item + self.tobacco_type = tobacco_smoking_history_item.type self.fields["value"] = MultipleChoiceField( choices=self.choices(), @@ -28,16 +28,23 @@ def __init__(self, *args, response_set, tobacco_type, **kwargs): value_field = self["value"] value_field.add_divider_after( - TobaccoSmokingHistory.Levels.STOPPED, "or" + TobaccoSmokingHistory.Levels.DECREASED, "or" ) def choices(self): return [ - (value, label) + (value, self.generate_label(value, label)) for value, label in TobaccoSmokingHistory.Levels.choices if value != TobaccoSmokingHistory.Levels.NORMAL ] + def generate_label(self, value, label): + if value == TobaccoSmokingHistory.Levels.NO_CHANGE: + return label + return TobaccoSmokingHistory.Levels(value).label + f" than {self.generate_label_suffix()}" + + def generate_label_suffix(self): + return f"{self.tobacco_smoking_history.smoked_amount_response.value} {self.tobacco_smoking_history.human_type().lower()} a {self.tobacco_smoking_history.smoking_frequency_response.get_value_display_as_singleton_text()}" def _required_error_message(self): return "Select if the number of cigarettes you smoke has changed over time" diff --git a/lung_cancer_screening/questions/models/tobacco_smoking_history.py b/lung_cancer_screening/questions/models/tobacco_smoking_history.py index 86eecebc..47ee2d31 100644 --- a/lung_cancer_screening/questions/models/tobacco_smoking_history.py +++ b/lung_cancer_screening/questions/models/tobacco_smoking_history.py @@ -32,7 +32,6 @@ class Levels(models.TextChoices): DECREASED = "decreased", "Yes, I used to smoke fewer" # Only used to populate values in the form - STOPPED = "stopped", "Yes, I stopped smoking for a period of 1 year or longer" NO_CHANGE = NO_CHANGE_VALUE, "No, it has not changed" diff --git a/lung_cancer_screening/questions/tests/factories/tobacco_smoking_history_factory.py b/lung_cancer_screening/questions/tests/factories/tobacco_smoking_history_factory.py index dadd0cd0..26ef8b00 100644 --- a/lung_cancer_screening/questions/tests/factories/tobacco_smoking_history_factory.py +++ b/lung_cancer_screening/questions/tests/factories/tobacco_smoking_history_factory.py @@ -13,3 +13,19 @@ class Meta: response_set = factory.SubFactory(ResponseSetFactory) type = TobaccoSmokingHistoryTypes.CIGARETTES + + class Params: + complete = factory.Trait( + smoked_amount_response=factory.RelatedFactory( + "lung_cancer_screening.questions.tests.factories.smoked_amount_response_factory.SmokedAmountResponseFactory", + factory_related_name="tobacco_smoking_history" + ), + smoking_frequency_response=factory.RelatedFactory( + "lung_cancer_screening.questions.tests.factories.smoking_frequency_response_factory.SmokingFrequencyResponseFactory", + factory_related_name="tobacco_smoking_history" + ), + smoking_current_response=factory.RelatedFactory( + "lung_cancer_screening.questions.tests.factories.smoking_current_response_factory.SmokingCurrentResponseFactory", + factory_related_name="tobacco_smoking_history" + ) + ) diff --git a/lung_cancer_screening/questions/tests/unit/forms/test_smoking_change_form.py b/lung_cancer_screening/questions/tests/unit/forms/test_smoking_change_form.py index f01f1906..9ae8c6c3 100644 --- a/lung_cancer_screening/questions/tests/unit/forms/test_smoking_change_form.py +++ b/lung_cancer_screening/questions/tests/unit/forms/test_smoking_change_form.py @@ -10,12 +10,18 @@ class TestSmokingChangeForm(TestCase): def setUp(self): self.response_set = ResponseSetFactory.create() + self.normal_smoking_history = TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + type=TobaccoSmokingHistoryTypes.CIGARETTES.value, + level=TobaccoSmokingHistory.Levels.NORMAL, + complete=True + ) def test_is_valid_a_valid_value(self): form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, data={"value": [TobaccoSmokingHistory.Levels.INCREASED]} ) self.assertTrue(form.is_valid()) @@ -24,7 +30,7 @@ def test_is_valid_a_valid_value(self): def test_is_invalid_with_an_invalid_value(self): form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, data={"value": ["invalid"]} ) self.assertFalse(form.is_valid()) @@ -37,7 +43,7 @@ def test_is_invalid_with_an_invalid_value(self): def test_is_invalid_when_no_option_is_selected(self): form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, data={"value": []} ) self.assertFalse(form.is_valid()) @@ -50,7 +56,7 @@ def test_is_invalid_when_no_option_is_selected(self): def test_saves_an_tobacco_smoking_history_for_the_type_and_level_selected(self): form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, data={ "value": [ TobaccoSmokingHistory.Levels.INCREASED, @@ -62,13 +68,12 @@ def test_saves_an_tobacco_smoking_history_for_the_type_and_level_selected(self): form.save() items = self.response_set.tobacco_smoking_history - self.assertEqual(items.count(), 2) + self.assertEqual(items.count(), 3) # Normal, Increased and Decreased self.assertTrue(all([ item.type == TobaccoSmokingHistoryTypes.CIGARETTES for item in items.all() ])) - def test_does_not_create_a_new_tobacco_smoking_history_if_the_level_already_exists(self): existing_item = TobaccoSmokingHistoryFactory( response_set=self.response_set, @@ -78,16 +83,16 @@ def test_does_not_create_a_new_tobacco_smoking_history_if_the_level_already_exis form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, data={ "value": [TobaccoSmokingHistory.Levels.INCREASED] } ) form.save() - - self.assertEqual(self.response_set.tobacco_smoking_history.count(), 1) + increased_history_items = self.response_set.tobacco_smoking_history.filter(level=TobaccoSmokingHistory.Levels.INCREASED) + self.assertEqual(increased_history_items.count(), 1) self.assertEqual( - self.response_set.tobacco_smoking_history.first(), + increased_history_items.first(), existing_item ) @@ -101,7 +106,7 @@ def test_deletes_a_tobacco_smoking_history_if_the_level_is_no_longer_selected(se form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, data={ "value": [TobaccoSmokingHistory.Levels.NO_CHANGE] } @@ -114,15 +119,9 @@ def test_deletes_a_tobacco_smoking_history_if_the_level_is_no_longer_selected(se def test_does_not_delete_normal_level_smoking_history(self): - TobaccoSmokingHistoryFactory( - response_set=self.response_set, - type=TobaccoSmokingHistoryTypes.CIGARETTES, - level=TobaccoSmokingHistory.Levels.NORMAL, - ) - form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, data={ "value": [TobaccoSmokingHistory.Levels.NO_CHANGE] } @@ -134,15 +133,9 @@ def test_does_not_delete_normal_level_smoking_history(self): ).count(), 1) def test_prevents_both_no_change_and_other_levels_selected(self): - TobaccoSmokingHistoryFactory( - response_set=self.response_set, - type=TobaccoSmokingHistoryTypes.CIGARETTES, - level=TobaccoSmokingHistory.Levels.NORMAL, - ) - form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, data={ "value": [TobaccoSmokingHistory.Levels.NO_CHANGE, TobaccoSmokingHistory.Levels.INCREASED] } @@ -165,19 +158,13 @@ def test_initializes_the_form_based_on_existing_smoking_histories(self): type=TobaccoSmokingHistoryTypes.CIGARETTES, level=TobaccoSmokingHistory.Levels.DECREASED, ) - TobaccoSmokingHistoryFactory( - response_set=self.response_set, - type=TobaccoSmokingHistoryTypes.CIGARETTES, - level=TobaccoSmokingHistory.Levels.STOPPED, - ) form = SmokingChangeForm( response_set=self.response_set, - tobacco_type=TobaccoSmokingHistoryTypes.CIGARETTES, + tobacco_smoking_history_item=self.normal_smoking_history, ) self.assertEqual(form.fields["value"].initial, [ TobaccoSmokingHistory.Levels.INCREASED, TobaccoSmokingHistory.Levels.DECREASED, - TobaccoSmokingHistory.Levels.STOPPED, ]) diff --git a/lung_cancer_screening/questions/tests/unit/views/test_smoking_change.py b/lung_cancer_screening/questions/tests/unit/views/test_smoking_change.py index 04cf42eb..eed7c9a2 100644 --- a/lung_cancer_screening/questions/tests/unit/views/test_smoking_change.py +++ b/lung_cancer_screening/questions/tests/unit/views/test_smoking_change.py @@ -18,8 +18,10 @@ def setUp(self): self.tobacco_smoking_history = TobaccoSmokingHistoryFactory.create( response_set=self.response_set, type=TobaccoSmokingHistoryTypes.CIGARETTES.value, + complete=True ) + def test_redirects_if_the_user_is_not_logged_in(self): self.client.logout() response = self.client.get( @@ -64,6 +66,27 @@ def test_redirects_when_the_user_is_not_eligible(self): self.assertRedirects(response, reverse("questions:have_you_ever_smoked")) + def test_redirects_to_smoked_amount_when_does_not_have_a_smoking_frequency_response_and_smoked_amount_response(self): + self.tobacco_smoking_history.smoking_frequency_response.delete() + + response = self.client.get( + reverse( + "questions:smoking_change", + kwargs={ + "tobacco_type": TobaccoSmokingHistoryTypes.CIGARETTES.value.lower() + }, + ) + ) + + self.assertRedirects( + response, + reverse( + "questions:smoked_amount", + kwargs={"tobacco_type": TobaccoSmokingHistoryTypes.CIGARETTES.value.lower()} + ), + fetch_redirect_response=False + ) + def test_responds_successfully(self): response = self.client.get( reverse("questions:smoking_change", kwargs={"tobacco_type": "cigarettes"}) @@ -80,6 +103,7 @@ def setUp(self): self.tobacco_smoking_history = TobaccoSmokingHistoryFactory.create( response_set=self.response_set, type=TobaccoSmokingHistoryTypes.CIGARETTES.value, + complete=True ) self.valid_params = { "value": [TobaccoSmokingHistory.Levels.INCREASED], diff --git a/lung_cancer_screening/questions/views/mixins/ensure_smoking_history_for_type.py b/lung_cancer_screening/questions/views/mixins/ensure_smoking_history_for_type.py index 7035f28f..b21d422b 100644 --- a/lung_cancer_screening/questions/views/mixins/ensure_smoking_history_for_type.py +++ b/lung_cancer_screening/questions/views/mixins/ensure_smoking_history_for_type.py @@ -1,12 +1,20 @@ from inflection import camelize from django.http import Http404 +from lung_cancer_screening.questions.models.tobacco_smoking_history import TobaccoSmokingHistory + class EnsureSmokingHistoryForTypeMixin: def dispatch(self, request, *args, **kwargs): url_tobacco_type = camelize(kwargs["tobacco_type"]) - if request.response_set.tobacco_smoking_history.filter(type=url_tobacco_type).exists(): + tobacco_smoking_history_item = request.response_set.tobacco_smoking_history.filter( + type=url_tobacco_type, + level=TobaccoSmokingHistory.Levels.NORMAL + ) + + if tobacco_smoking_history_item.exists(): + request.tobacco_smoking_history_item = tobacco_smoking_history_item.first() return super().dispatch(request, *args, **kwargs) else: raise Http404 diff --git a/lung_cancer_screening/questions/views/smoking_change.py b/lung_cancer_screening/questions/views/smoking_change.py index c08cd7a3..7ad846c6 100644 --- a/lung_cancer_screening/questions/views/smoking_change.py +++ b/lung_cancer_screening/questions/views/smoking_change.py @@ -1,3 +1,4 @@ +from django.shortcuts import redirect from django.urls import reverse from django.contrib.auth.mixins import LoginRequiredMixin from django.views.generic.edit import FormView @@ -7,12 +8,25 @@ from .mixins.ensure_smoking_history_for_type import EnsureSmokingHistoryForTypeMixin from ..forms.smoking_change_form import SmokingChangeForm +class EnsureSmokingFrequencyAndAmountResponseMixin: + def dispatch(self, request, *args, **kwargs): + if ( + hasattr(request.tobacco_smoking_history_item, 'smoking_frequency_response') + and hasattr(request.tobacco_smoking_history_item, 'smoked_amount_response') + ): + return super().dispatch(request, *args, **kwargs) + + return redirect(reverse( + "questions:smoked_amount", + kwargs={"tobacco_type": kwargs["tobacco_type"]}, + )) class SmokingChangeView( LoginRequiredMixin, EnsureResponseSet, EnsureEligibleMixin, EnsureSmokingHistoryForTypeMixin, + EnsureSmokingFrequencyAndAmountResponseMixin, FormView, ): template_name = "question_form.jinja" @@ -21,7 +35,7 @@ class SmokingChangeView( def get_form_kwargs(self): kwargs = super().get_form_kwargs() kwargs["response_set"] = self.request.response_set - kwargs["tobacco_type"] = self.kwargs["tobacco_type"] + kwargs["tobacco_smoking_history_item"] = self.request.tobacco_smoking_history_item return kwargs def get_context_data(self, **kwargs): @@ -41,7 +55,4 @@ def form_valid(self, form): def get_success_url(self): - if self.request.POST.get("change"): - return reverse("questions:responses") - return reverse("questions:responses") diff --git a/lung_cancer_screening/questions/views/smoking_current.py b/lung_cancer_screening/questions/views/smoking_current.py index 23f74898..e27fa087 100644 --- a/lung_cancer_screening/questions/views/smoking_current.py +++ b/lung_cancer_screening/questions/views/smoking_current.py @@ -13,7 +13,7 @@ class SmokingCurrentView(LoginRequiredMixin, EnsureResponseSet, EnsureEligibleMi template_name = "question_form.jinja" form_class = SmokingCurrentForm model = SmokingCurrentResponse - back_link_url = reverse_lazy("questions:age_when_started_smoking") + back_link_url = reverse_lazy("questions:types_tobacco_smoking") def get_success_url(self): return reverse(