From cb31b42b021d7d8b84e1187386b9e8105147df70 Mon Sep 17 00:00:00 2001 From: Andy Mitchell <326561+Themitchell@users.noreply.github.com> Date: Thu, 12 Mar 2026 14:39:32 +0000 Subject: [PATCH] PPHA-589: Ensure no change smoking redirects to next type --- features/smoking_history.feature | 24 ++++++++ .../questions/models/response_set.py | 16 +++++- .../models/tobacco_smoking_history.py | 10 +++- .../tests/unit/models/test_response_set.py | 45 +++++++++++++++ .../models/test_tobacco_smoking_history.py | 19 ++++++- .../tests/unit/views/test_smoking_change.py | 55 ++++++++++--------- .../questions/views/smoked_total_years.py | 1 - .../questions/views/smoking_change.py | 44 ++++++++------- 8 files changed, 165 insertions(+), 49 deletions(-) diff --git a/features/smoking_history.feature b/features/smoking_history.feature index 54dd5321..f7168f25 100644 --- a/features/smoking_history.feature +++ b/features/smoking_history.feature @@ -108,6 +108,7 @@ Feature: Smoking history pages And I check "Rolling tobacco" And I check "Pipe" And I check "Cigarillos" + And I check "Small cigars" And I check "Medium cigars" And I submit the form @@ -261,6 +262,29 @@ Feature: Smoking history pages When I fill in "Roughly how many years did you smoke 5 full pipe loads a month?" with "4" And I submit the form + # Small cigars with no change + Then I am on "/small-cigars-smoking-current" + And I see a page title "Do you currently smoke small cigars?" + When I check "Yes" + And I submit the form + + Then I am on "/small-cigars-smoked-total-years" + When I fill in "Roughly how many years have you smoked small cigars?" with "8" + And I submit the form + + Then I am on "/small-cigars-smoking-frequency" + And I see a page title "How often do you smoke small cigars?" + When I check "Monthly" + And I submit the form + + Then I am on "/small-cigars-smoked-amount" + When I fill in "Roughly how many small cigars do you currently smoke in a normal month?" with "9" + And I submit the form + + Then I am on "/small-cigars-smoking-change" + And I see a page title "Has the number of small cigars you normally smoke changed over time?" + When I check "No, it has not changed" + And I submit the form # Medium cigars with only decreased Then I am on "/medium-cigars-smoking-current" diff --git a/lung_cancer_screening/questions/models/response_set.py b/lung_cancer_screening/questions/models/response_set.py index e3b6ee30..74c5f0f6 100644 --- a/lung_cancer_screening/questions/models/response_set.py +++ b/lung_cancer_screening/questions/models/response_set.py @@ -129,8 +129,12 @@ def types_tobacco_smoking_history(self): )) + def user_editable_tobacco_smoking_histories(self): + return self.tobacco_smoking_history.user_editable().in_form_order().all() + + def previous_normal_smoking_history(self, smoking_history_item): - histories = list(self.tobacco_smoking_history.in_form_order().all()) + histories = list(self.user_editable_tobacco_smoking_histories()) current_history_index = histories.index(smoking_history_item) if current_history_index == 0: @@ -146,3 +150,13 @@ def previous_smoking_history(self, smoking_history_item): return self.tobacco_smoking_history.filter( type=self.previous_normal_smoking_history(smoking_history_item).type, ).in_form_order().last() + + + def next_smoking_history(self, smoking_history_item): + histories = list(self.user_editable_tobacco_smoking_histories()) + current_history_index = histories.index(smoking_history_item) + + if current_history_index == len(histories) - 1: + return None + + return histories[current_history_index + 1] diff --git a/lung_cancer_screening/questions/models/tobacco_smoking_history.py b/lung_cancer_screening/questions/models/tobacco_smoking_history.py index 6b7b54b2..723634d2 100644 --- a/lung_cancer_screening/questions/models/tobacco_smoking_history.py +++ b/lung_cancer_screening/questions/models/tobacco_smoking_history.py @@ -28,14 +28,21 @@ def in_form_order(self): *[When(level=level_val, then=Value(i)) for i, level_val in enumerate(levels_order)], default=Value(len(levels_order)), ) - return self.order_by(order_levels, order_types) + return self.order_by(order_types, order_levels) + + + def user_editable(self): + return self.exclude(level=Levels.NO_CHANGE) + def increased(self): return self.filter(level='increased') + def decreased(self): return self.filter(level='decreased') + def normal(self): return self.filter(level='normal') @@ -114,6 +121,7 @@ def clean(self): super().clean() self._validate_no_change_and_other_levels() + def _validate_no_change_and_other_levels(self): others = self.response_set.tobacco_smoking_history.filter(type=self.type).exclude(pk=self.pk) if self.level == NO_CHANGE_VALUE: diff --git a/lung_cancer_screening/questions/tests/unit/models/test_response_set.py b/lung_cancer_screening/questions/tests/unit/models/test_response_set.py index 35e544a7..d5f08475 100644 --- a/lung_cancer_screening/questions/tests/unit/models/test_response_set.py +++ b/lung_cancer_screening/questions/tests/unit/models/test_response_set.py @@ -421,3 +421,48 @@ def test_returns_decreased_when_the_previous_normal_smoking_history_when_both_pr self.response_set.previous_smoking_history(current_smoking_history), decreased_smoking_history ) + + + def test_next_smoking_history_returns_none_when_it_is_the_only_smoking_history_item(self): + smoking_history_item = TobaccoSmokingHistoryFactory.create( + response_set=self.response_set + ) + self.assertIsNone( + self.response_set.next_smoking_history(smoking_history_item) + ) + + + def test_next_smoking_history_returns_the_next_smoking_history_when_it_is_not_the_only_smoking_history_item_respecting_form_order(self): + TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + cigarillos=True, + increased=True, + ) + next_smoking_history = TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + cigarillos=True, + ) + current_smoking_history = TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + cigarettes=True, + ) + self.assertEqual( + self.response_set.next_smoking_history(current_smoking_history), + next_smoking_history + ) + + + def test_next_smoking_history_does_not_include_no_change(self): + current_smoking_history = TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + cigarillos=True, + normal=True, + ) + TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + cigarillos=True, + no_change=True, + ) + self.assertIsNone( + self.response_set.next_smoking_history(current_smoking_history) + ) diff --git a/lung_cancer_screening/questions/tests/unit/models/test_tobacco_smoking_history.py b/lung_cancer_screening/questions/tests/unit/models/test_tobacco_smoking_history.py index 3e1c3039..2a6264c7 100644 --- a/lung_cancer_screening/questions/tests/unit/models/test_tobacco_smoking_history.py +++ b/lung_cancer_screening/questions/tests/unit/models/test_tobacco_smoking_history.py @@ -652,6 +652,11 @@ def test_is_complete_returns_true_for_a_no_change_level_when_no_responses_are_pr def test_in_form_order_returns_the_tobacco_smoking_history_in_form_order(self): + cigarettes_decreased = TobaccoSmokingHistoryFactory( + response_set=self.response_set, + cigarettes=True, + decreased=True, + ) medium_cigars_decreased = TobaccoSmokingHistoryFactory( response_set=self.response_set, medium_cigars=True, @@ -676,6 +681,18 @@ def test_in_form_order_returns_the_tobacco_smoking_history_in_form_order(self): in_form_order = TobaccoSmokingHistory.objects.in_form_order() self.assertQuerySetEqual( in_form_order, - [cigarettes_normal, medium_cigars_normal, medium_cigars_increased, medium_cigars_decreased], + [cigarettes_normal, cigarettes_decreased, medium_cigars_normal, medium_cigars_increased, medium_cigars_decreased], ordered=True, ) + + + def test_user_editable_does_not_include_no_change(self): + TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + cigarillos=True, + no_change=True, + ) + self.assertQuerySetEqual( + TobaccoSmokingHistory.objects.user_editable().all(), + [], + ) 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 887e0066..e3b4d94a 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 @@ -191,6 +191,7 @@ def test_redirects_when_the_user_is_not_eligible(self): self.assertRedirects(response, reverse("questions:have_you_ever_smoked")) + def test_redirects_to_the_next_question_given_no_level(self): response = self.client.post( reverse("questions:smoking_change", kwargs = { @@ -202,6 +203,35 @@ def test_redirects_to_the_next_question_given_no_level(self): self.assertRedirects(response, reverse("questions:responses")) + def test_redirects_to_the_next_smoking_type_given_no_change_level_and_other_smoking_histories_exist(self): + self.tobacco_smoking_history.delete() + current_smoking_history = TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + cigarettes=True, + complete=True, + normal=True, + ) + next_smoking_history = TobaccoSmokingHistoryFactory.create( + response_set=self.response_set, + medium_cigars=True, + complete=True, + normal=True, + ) + + response = self.client.post( + reverse("questions:smoking_change", kwargs = { + "tobacco_type": current_smoking_history.url_type() + }), + {"value": [TobaccoSmokingHistory.Levels.NO_CHANGE]} + ) + + self.assertRedirects(response, + reverse("questions:smoking_current", kwargs={ + "tobacco_type": next_smoking_history.url_type() + }), + fetch_redirect_response=False + ) + def test_redirects_to_the_next_question_given_level_increased(self): response = self.client.post( reverse("questions:smoking_change", kwargs = { @@ -230,31 +260,6 @@ def test_redirects_to_the_next_question_given_level_decreased_only(self): })) - def test_does_not_redirect_to_increased_if_increased_exists_for_another_type_and_is_not_selected(self): - TobaccoSmokingHistoryFactory.create( - response_set=self.response_set, - type=self.tobacco_smoking_history.type, - complete=True, - increased=True, - ) - medium_cigars = TobaccoSmokingHistoryFactory.create( - response_set=self.response_set, - medium_cigars=True, - complete=True - ) - - response = self.client.post( - reverse("questions:smoking_change", kwargs = { - "tobacco_type": medium_cigars.url_type() - }), - {"value": [TobaccoSmokingHistory.Levels.NO_CHANGE]} - ) - - self.assertRedirects(response, reverse("questions:responses"), - fetch_redirect_response=False - ) - - def test_creates_a_smoking_change_response(self): self.client.post( reverse("questions:smoking_change", kwargs = { diff --git a/lung_cancer_screening/questions/views/smoked_total_years.py b/lung_cancer_screening/questions/views/smoked_total_years.py index d6bd8f2c..efe1e805 100644 --- a/lung_cancer_screening/questions/views/smoked_total_years.py +++ b/lung_cancer_screening/questions/views/smoked_total_years.py @@ -105,7 +105,6 @@ def prerequisite_responses(self): "smoked_amount_response" ] - def remaining_unanswered_histories(self): tobacco_type = camelize(underscore(self.kwargs["tobacco_type"])) histories = self.request.response_set.types_tobacco_smoking_history() diff --git a/lung_cancer_screening/questions/views/smoking_change.py b/lung_cancer_screening/questions/views/smoking_change.py index d16095ff..0d22fff5 100644 --- a/lung_cancer_screening/questions/views/smoking_change.py +++ b/lung_cancer_screening/questions/views/smoking_change.py @@ -3,7 +3,6 @@ from django.contrib.auth.mixins import LoginRequiredMixin from django.views.generic.edit import FormView -from lung_cancer_screening.questions.models.tobacco_smoking_history import TobaccoSmokingHistory from .mixins.ensure_response_set import EnsureResponseSet from .mixins.ensure_eligible import EnsureEligibleMixin @@ -57,25 +56,24 @@ def form_valid(self, form): def get_success_url(self): - tobacco_smoking_history = self.tobacco_smoking_history() - if tobacco_smoking_history.increased().exists(): - return reverse( - "questions:smoking_frequency", - kwargs={ - "tobacco_type": self.kwargs["tobacco_type"], - "level": TobaccoSmokingHistory.Levels.INCREASED, - }, - query=self.get_change_query_params() - ) - elif tobacco_smoking_history.decreased().exists(): - return reverse( - "questions:smoking_frequency", - kwargs={ - "tobacco_type": self.kwargs["tobacco_type"], - "level": TobaccoSmokingHistory.Levels.DECREASED, - }, - query=self.get_change_query_params(), - ) + if self.next_smoking_history(): + if self.next_smoking_history().is_normal(): + return reverse( + "questions:smoking_current", + kwargs={ + "tobacco_type": self.next_smoking_history().url_type(), + }, + query=self.get_change_query_params(), + ) + else: + return reverse( + "questions:smoking_frequency", + kwargs={ + "tobacco_type": self.next_smoking_history().url_type(), + "level": self.next_smoking_history().level, + }, + query=self.get_change_query_params(), + ) else: return reverse("questions:responses") @@ -95,3 +93,9 @@ def tobacco_smoking_history(self): return self.request.response_set.tobacco_smoking_history.by_url_type( self.kwargs["tobacco_type"] ) + + + def next_smoking_history(self): + return self.request.response_set.next_smoking_history( + self.tobacco_smoking_history_item() + )