RAS-939 Survey Status Change #914

arroyoAle · 2024-02-02T13:44:17Z

What and why?

Adds a new case group transition from complete to not started to allow business users a new status change in rOps

How to test?

deploy this and RAS-939 Survey Status Change rm-case-service#289 to your environment
run the acceptance tests to populate your env
log into frontstage and complete the survey
go to http://localhost:8085/reporting-units/49900000001 and navigate to change the status from complete to not started
check that the survey appears in the todo page in frontstage
check that http://localhost:8085/reporting-units/49900000001 and http://localhost:8085/case/49900000001/response-status?survey=QBS&period=1912 look the same and act the same as they do on main including with and without permissions

Jira

https://jira.ons.gov.uk/browse/RAS-939

…to change-status-to-not-started

arroyoAle · 2024-02-02T16:18:27Z

response_operations_ui/templates/reporting-unit-survey.html

                        {% set survey_link = '&nbsp;   <a href="' + url_for("case_bp.get_response_statuses", ru_ref=ru.sampleUnitRef, survey=survey.shortName, period=ce.exerciseRef) + '">Change</a>' %}
-                    {% elif ce.responseStatus in ['Completed', 'Completed by phone', 'No longer required'] and isReportingUnitPermission  %}
+                    {% elif isReportingUnitPermission  %}


I'm unsure about this since I added the permission check in the next page as well so this might be worth just being an else and then view link

I decided to just make this an if else and show the view to all users as that is how it is used across the rest of the site. It just made more sense to me

LJBabbage · 2024-02-05T17:02:57Z

response_operations_ui/templates/reporting-unit-survey.html

@@ -67,12 +67,10 @@ <h3 id="COLLECTION_EXERCISES" class="ons-u-fs-m">Collection exercises</h3>
                        {% set status_class = 'ons-status--error' %}
                    {% endif %}

-                    {% if ce.responseStatus in ['Not started', 'In progress'] and isReportingUnitPermission  %}
+                    {% if ce.responseStatus in ['Not started', 'In progress','Completed', 'Completed by phone', 'No longer required'] and isReportingUnitPermission  %}


This isn't going to work in conjunction with eQ, QuestionnaireState which holds the answers for eQ will still be present if Completed by phone or No longer required has been used and the respondent started the survey. eQ only deletes QuestionnaireState if it's completed by them

It would be nice to take the opportunity to move to this to the backend, removing the logic from the template. It only needs the check and link generation, not everything.

This isn't going to work in conjunction with eQ, QuestionnaireState which holds the answers for eQ will still be present if Completed by phone or No longer required has been used and the respondent started the survey. eQ only deletes QuestionnaireState if it's completed by them

The issue of EQ post submission state has also been raised when changing from COMPLETED to NOT STARTED. Depending on the time-bound post submission state, and the post-submission cookie state the respondent will not necessarily be presented with a new 'blank' questionnaire as the response_id may resolve to an existing state for a period of time after submission.

I haven't changed the functionality of the Completed by Phone and No longer required rOps users were already able to change those to Not started just that before they were clicking a link that said View and were then presented with options to change the status - I've also moved the logic to the backend

LJBabbage · 2024-02-05T17:14:47Z

tests/views/test_change_response_status.py

+        self.assertIn(b"Not started", data)
+
+    @requests_mock.mock()
+    def test_not_started_status_is_not_present_for_not_started_without_permission(self, mock_request):


I find this name difficult to understand, specially with 2 not_started in it

Updated to be more clear

LJBabbage · 2024-02-05T17:19:05Z

tests/views/test_change_response_status.py

@@ -331,3 +356,43 @@ def test_not_started_status_is_present_for_no_longer_required(self, mock_request
        self.assertEqual(response.status_code, 200)
        self.assertIn(b"No longer required", data)
        self.assertIn(b"Not started", data)
+
+    @requests_mock.mock()
+    def test_not_started_status_is_present_for_completed(self, mock_request):


We really should try to move away from this pattern or mass mocking at the highest level as it gets messy. The functionality should in the backend which then means we can unit test the link creation combinations.

I'll leave this in but I have moved the link creation to the backend with additional tests

You can, but now you have done the work to test the dict being passed, you don't need to. The idea is you should only need to test the template logic

SteveScorfield · 2024-02-12T08:40:13Z

/deploy scorfs

ras-rm-pr-bot · 2024-02-12T08:40:42Z

Deploying to dev cluster with following parameters:

namespace: scorfs
tag: pr-914
configBranch: main
paramKey: ``
paramValue: ``

SteveScorfield

I have tested these changes and reviewed the code. All looks good to me.

LJBabbage

Looking better and nice to see some pytest fixtures use. It needs a bit or refinement with function names, structure and reuse.

LJBabbage · 2024-02-12T16:27:49Z

response_operations_ui/contexts/case.py

+from flask import url_for
+
+
+def build_case_context(


The function name and return, don't really align

Updated this

LJBabbage · 2024-02-12T16:28:20Z

response_operations_ui/contexts/case.py

+
+
+def build_case_context(
+    ru_ref,


It's not fundamental, but I have been trying to do typing for anything new

I've added typing where i can

LJBabbage · 2024-02-12T16:30:52Z

response_operations_ui/contexts/case.py

+    return {"change_response_status": change_response_status}
+
+
+def _build_change_response_status(


Not quite sure why this is split as it is single use, bar the dict assignment I can't see what else the parent function is doing

You're right I feel like I was on autopilot when writing this - now fixed

LJBabbage · 2024-02-12T16:36:32Z

response_operations_ui/contexts/case.py

+):
+    if has_reporting_unit_permission:
+        change_response_status = {
+            "form_action": url_for(


Very minor, but I would probably go with url as the key, just because it aligns with the flask function

updated this

LJBabbage · 2024-02-12T16:39:55Z

response_operations_ui/contexts/case.py

+
+def _generate_radios(allowed_transitions_for_case):
+    radios = []
+    for index, (event, status) in enumerate(allowed_transitions_for_case.items()):


list comprehension instead?

Swapped list comprehension in

LJBabbage · 2024-02-13T09:19:34Z

tests/context/conftest.py

+def multiple_collection_exercises_with_details():
+    return [
+        {
+            "id": "57e06b27-a62b-4d8e-8d57-7b6b80067e4f",


Feels to me this chunk can be extracted

LJBabbage · 2024-02-13T09:20:21Z

tests/context/conftest.py

+            },
+        ],
+        "attributes": {
+            "birthdate": "01/09/1993",


This again feels like it can be extracted and used as a fixture import

Made into a fixture import

LJBabbage · 2024-02-13T09:21:11Z

tests/context/conftest.py

+@pytest.fixture
+def multiple_cases():
+    return {
+        "state": "ACTIONABLE",


Again this looks like the same chunk bar an odd change

Updated this

LJBabbage · 2024-02-13T09:22:01Z

tests/context/test_case_context.py

+SURVEY_ID = "02b9c366-7397-42f7-942a-76dc5876d86d"
+
+
+def test_case_context_with_permissions(app, transitions_for_complete_case, expected_case_context_with_all_permissions):


You might be able to use fixture params here

Swapped for fixture params

LJBabbage · 2024-02-13T09:24:15Z

tests/views/test_change_response_status.py

@@ -331,3 +356,43 @@ def test_not_started_status_is_present_for_no_longer_required(self, mock_request
        self.assertEqual(response.status_code, 200)
        self.assertIn(b"No longer required", data)
        self.assertIn(b"Not started", data)
+
+    @requests_mock.mock()
+    def test_not_started_status_is_present_for_completed(self, mock_request):


You can, but now you have done the work to test the dict being passed, you don't need to. The idea is you should only need to test the template logic

# Conflicts: # Pipfile.lock # response_operations_ui/views/reporting_units.py

SteveScorfield · 2024-02-14T10:19:01Z

/deploy scorfs

ras-rm-pr-bot · 2024-02-14T10:19:32Z

Deploying to dev cluster with following parameters:

namespace: scorfs
tag: pr-914
configBranch: main
paramKey: ``
paramValue: ``

SteveScorfield

I have tested this again and all works as expected. I'm also happy with the recommended code changes.

LJBabbage

Code looking good, a few minor things, tests need a bit more work regarding naming and fixture use

LJBabbage · 2024-02-14T10:05:07Z

response_operations_ui/contexts/case.py

+    survey_id: str,
+    has_reporting_unit_permission: bool,
+) -> dict:
+    if has_reporting_unit_permission:


You have kind of doubled up here with the frontend and checked permissions twice ({% if isReportingUnitPermission %} in the template), so else is never reachable. However we shouldn't need to do isReportingUnitPermission in the template, as it's been done in the back end. Depends on what else uses it on how to pass it through, if it's just this then checking change_response_status is not null would work

I've removed this permissions check from the frontend and made it just check for it existing

LJBabbage · 2024-02-14T10:08:58Z

response_operations_ui/contexts/reporting_units.py

+        survey["surveyRef"],
+        survey["shortName"],
+        survey["id"],


Sorry my fault, I wasn't clear. If it's 1 or 2 elements in a list we should pass the value, more than that pass the list. It's a balance act between passing what it needs and how many vars for the function

Ahh yeah that makes more sense - updated

LJBabbage · 2024-02-14T10:09:41Z

response_operations_ui/contexts/case.py

+    return {"change_response_status": change_response_status}
+
+
+def _generate_radios(allowed_transitions_for_case: dict):


Missing the return type

added return type

LJBabbage · 2024-02-14T10:11:19Z

response_operations_ui/contexts/reporting_units.py

+        ),
+        "hyperlink_text": "Change" if ru_permission else "View",
+        "status_class": (
+            CE_STATUS_CLASS[response_status] if response_status in CE_STATUS_CLASS.keys() else "ons-status--error"


A get with default might be better here

Updated to get with a default

LJBabbage · 2024-02-14T10:26:58Z

response_operations_ui/contexts/reporting_units.py

+    for respondent in respondents:
+        row = {}
+        if permissions["reporting_unit_edit"]:
+            if unused_iac:


I see it, the logic is a bit unusual as the frontend is also checking off the iac key being passed to it and diverting it around. We should try not to do that as it kind of invalidates any unit tests in that area as there is the same logic both in the frontend and back.

LJBabbage · 2024-02-14T10:44:26Z

tests/context/conftest.py

+
+
+@pytest.fixture()
+def collection_exercises_with_details():


This has a lot of stuff we don't need. Notice up the top there is a slim line ce_details. The idea being that is the starting point, each fixture below extents depending on what it needs to test. It doesn't need anything that it is not testing, it's not going to the frontend so dates, events, samples etc aren't needed for everything. So if it's build_reporting_units_context your testing, call your fixture appropriately extent the CE base and just make sure the values are populated to test, everything else could be None.

It will reduce the code massively and make it easier to maintain

Updated and slimmed down

LJBabbage · 2024-02-14T10:50:46Z

tests/context/conftest.py

+@pytest.fixture
+def transitions_for_complete_case():
+    return {"COMPLETED_TO_NOTSTARTED": "Not started"}
+
+
+@pytest.fixture
+def transitions_for_completed_by_phone_case():
+    return {"COMPLETED_TO_NOTSTARTED": "Not started"}
+
+
+@pytest.fixture
+def transitions_for_no_longer_required_case():
+    return {"COMPLETED_TO_NOTSTARTED": "Not started"}
+


Not sure why these are all the same

removed the repeated ones

LJBabbage · 2024-02-14T10:51:16Z

tests/context/conftest.py

+@pytest.fixture
+def transitions_for_in_progress_case():
+    return {
+        "COMPLETED_BY_PHONE": "Completed by phone",
+        "NO_LONGER_REQUIRED": "No longer required",
+    }
+
+
+@pytest.fixture
+def transitions_for_not_started_case():
+    return {
+        "COMPLETED_BY_PHONE": "Completed by phone",
+        "NO_LONGER_REQUIRED": "No longer required",
+    }


LJBabbage · 2024-02-14T10:52:20Z

tests/context/test_case_context.py

+from response_operations_ui.contexts.case import build_response_status_context
+
+
+def test_case_context_with_permissions(


A number of test names needs a tweak, they need to reflect what we are testing here its not case but the response status

LJBabbage

Looks good and works as expected, seen a small issue not connected to the card, respondent seems to be missing, but it's the same in latest. Bar that one recommendation for future tests

LJBabbage · 2024-02-15T11:27:48Z

tests/context/test_case_context.py

+from response_operations_ui.contexts.case import build_response_status_context
+
+
+def test_response_status_context_with_permissions(


These are fine, and one for another time, but pytest.mark.parametrize would of worked well here and saved a lot of repetition, https://docs.pytest.org/en/7.1.x/how-to/parametrize.html#parametrize. You would of had to use something like getfixturevalue in the function for the compare

arroyoAle and others added 5 commits February 2, 2024 13:43

changes to enable status change from complete to not started

6ba2b91

Merge branch 'main' into change-status-to-not-started

d5efbaf

auto patch increment

291b7cd

removing line used to fix tests

598fd85

Merge remote-tracking branch 'origin/change-status-to-not-started' in…

edb2a09

…to change-status-to-not-started

arroyoAle commented Feb 2, 2024

View reviewed changes

adding new test and updating if

7becf4b

arroyoAle changed the title ~~changes to enable status change from complete to not started~~ RAS-939 Survey Status Change Feb 5, 2024

arroyoAle mentioned this pull request Feb 5, 2024

RAS-939 Survey Status Change ONSdigital/rm-case-service#289

Merged

arroyoAle marked this pull request as ready for review February 5, 2024 14:03

LJBabbage requested changes Feb 5, 2024

View reviewed changes

arroyoAle added 6 commits February 7, 2024 16:14

context work for reporting units page and start of tests

6d7ff73

adding more tests and some fixes

bd66a6b

adding case context and tests

48078ba

adding additional transition tests

04ab279

linting

3b80624

linting again

926e9ab

fixing for multiple respondents and ces

77bf6da

SteveScorfield reviewed Feb 12, 2024

View reviewed changes

SteveScorfield approved these changes Feb 12, 2024

View reviewed changes

LJBabbage requested changes Feb 13, 2024

View reviewed changes

arroyoAle and others added 5 commits February 13, 2024 09:30

Merge branch 'main' into change-status-to-not-started

a510d11

# Conflicts: # Pipfile.lock # response_operations_ui/views/reporting_units.py

auto patch increment

4bbf054

review changes

ca013d7

linting

dd76891

additional typing

804edb9

SteveScorfield approved these changes Feb 14, 2024

View reviewed changes

LJBabbage requested changes Feb 14, 2024

View reviewed changes

review changes

d854df3

LJBabbage approved these changes Feb 15, 2024

View reviewed changes

arroyoAle merged commit bb502c3 into main Feb 15, 2024
3 checks passed

arroyoAle deleted the change-status-to-not-started branch February 15, 2024 12:35

		return {"change_response_status": change_response_status}


		def _build_change_response_status(

		SURVEY_ID = "02b9c366-7397-42f7-942a-76dc5876d86d"


		def test_case_context_with_permissions(app, transitions_for_complete_case, expected_case_context_with_all_permissions):

		return {"change_response_status": change_response_status}


		def _generate_radios(allowed_transitions_for_case: dict):

		from response_operations_ui.contexts.case import build_response_status_context


		def test_case_context_with_permissions(

		from response_operations_ui.contexts.case import build_response_status_context


		def test_response_status_context_with_permissions(

RAS-939 Survey Status Change #914

RAS-939 Survey Status Change #914

Conversation

arroyoAle commented Feb 2, 2024 • edited Loading

What and why?

How to test?

Jira

arroyoAle Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pricem14pc Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

arroyoAle Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LJBabbage Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveScorfield commented Feb 12, 2024

ras-rm-pr-bot commented Feb 12, 2024

SteveScorfield left a comment

Choose a reason for hiding this comment

LJBabbage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveScorfield commented Feb 14, 2024

ras-rm-pr-bot commented Feb 14, 2024

SteveScorfield left a comment

Choose a reason for hiding this comment

LJBabbage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LJBabbage Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LJBabbage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arroyoAle commented Feb 2, 2024 •

edited

Loading

arroyoAle Feb 2, 2024 •

edited

Loading

pricem14pc Feb 6, 2024 •

edited

Loading

arroyoAle Feb 9, 2024 •

edited

Loading

LJBabbage Feb 5, 2024 •

edited

Loading

LJBabbage Feb 14, 2024 •

edited

Loading