Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RAS-939 Survey Status Change #914

Merged
merged 19 commits into from
Feb 15, 2024
Merged

RAS-939 Survey Status Change #914

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
6ba2b91
changes to enable status change from complete to not started
arroyoAle Feb 2, 2024
d5efbaf
Merge branch 'main' into change-status-to-not-started
arroyoAle Feb 2, 2024
291b7cd
auto patch increment
ras-rm-pr-bot Feb 2, 2024
598fd85
removing line used to fix tests
arroyoAle Feb 2, 2024
edb2a09
Merge remote-tracking branch 'origin/change-status-to-not-started' in…
arroyoAle Feb 2, 2024
7becf4b
adding new test and updating if
arroyoAle Feb 5, 2024
6d7ff73
context work for reporting units page and start of tests
arroyoAle Feb 7, 2024
bd66a6b
adding more tests and some fixes
arroyoAle Feb 8, 2024
48078ba
adding case context and tests
arroyoAle Feb 9, 2024
04ab279
adding additional transition tests
arroyoAle Feb 9, 2024
3b80624
linting
arroyoAle Feb 9, 2024
926e9ab
linting again
arroyoAle Feb 9, 2024
77bf6da
fixing for multiple respondents and ces
arroyoAle Feb 12, 2024
a510d11
Merge branch 'main' into change-status-to-not-started
arroyoAle Feb 13, 2024
4bbf054
auto patch increment
ras-rm-pr-bot Feb 13, 2024
ca013d7
review changes
arroyoAle Feb 13, 2024
dd76891
linting
arroyoAle Feb 14, 2024
804edb9
additional typing
arroyoAle Feb 14, 2024
d854df3
review changes
arroyoAle Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 38 additions & 30 deletions Pipfile.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions _infra/helm/response-operations-ui/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ type: application

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
version: 3.1.52
version: 3.1.53

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application.
appVersion: 3.1.52
appVersion: 3.1.53
4 changes: 2 additions & 2 deletions response_operations_ui/templates/reporting-unit-survey.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ <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 %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pricem14pc pricem14pc Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@arroyoAle arroyoAle Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{% 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 %}
Copy link
Contributor Author

@arroyoAle arroyoAle Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{% set survey_link = '&nbsp; <a href="' + url_for("case_bp.get_response_statuses", ru_ref=ru.sampleUnitRef, survey=survey.shortName, period=ce.exerciseRef) + '">View</a>' %}
{% else %}
{% set survey_link = '' %}
Expand Down
5 changes: 2 additions & 3 deletions response_operations_ui/templates/response-status.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
{% from "components/radios/_macro.njk" import onsRadios %}

{% set page_title = 'Change response status for ' ~ ru_name ~ ' ('~ru_ref~') for survey ' ~ survey_id ~ ' ' ~ survey_name ~ ' period ' ~ ce_period %}
{% set isReportingUnitPermission = hasPermission('reportingunits.edit') %}
{%- block main %}

<div class="ons-grid ons-grid--gutterless">
Expand Down Expand Up @@ -92,9 +93,7 @@ <h1 class="ons-u-mt-m ons-u-fs-xl">Change response status</h1>
]
})
}}
{% if is_case_complete %}
<a href="{{ url_for('reporting_unit_bp.view_reporting_unit_survey', ru_ref=ru_ref, survey_id=survey_id) }}" id="close-response">Close</a>
{% else %}
{% if isReportingUnitPermission %}
<form action="{{ url_for('case_bp.update_response_status', ru_ref=ru_ref, survey=survey_short_name, case_group_id=case_group_id, period=ce_period) }}" method="post" class="ons-u-mt-s">
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
{% set radioComponent = {
Expand Down
42 changes: 41 additions & 1 deletion tests/views/test_change_response_status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
from unittest import TestCase

import fakeredis
import jwt
import requests_mock

from config import TestingConfig
from response_operations_ui import create_app
from tests.views.test_sign_in import url_sign_in_data

short_name = "BLOCKS"
survey_id = "cb0711c3-0ac8-41d3-ae0e-567e5ea1ef87"
Expand All @@ -32,8 +34,9 @@
url_get_case_by_case_group_id = f"{TestingConfig.CASE_URL}/cases/casegroupid/{case_group_id}"
url_get_case_events = f"{TestingConfig.CASE_URL}/cases/{case_id}/events"
get_respondent_by_id_url = f"{TestingConfig.PARTY_URL}/party-api/v1/respondents/id/{party_id}"
project_root = os.path.dirname(os.path.dirname(__file__))
url_permission_url = f"{TestingConfig.UAA_SERVICE_URL}/Users/test-id"

project_root = os.path.dirname(os.path.dirname(__file__))

with open(f"{project_root}/test_data/survey/single_survey.json") as fp:
survey = json.load(fp)
Expand All @@ -60,12 +63,19 @@
with open(f"{project_root}/test_data/case/case_groups_list_no_longer_required.json") as fp:
case_groups_no_longer_required = json.load(fp)

user_permission_reporting_unit_edit_json = {
"id": "5902656c-c41c-4b38-a294-0359e6aabe59",
"groups": [{"value": "f385f89e-928f-4a0f-96a0-4c48d9007cc3", "display": "reportingunits.edit", "type": "DIRECT"}],
}


class TestChangeResponseStatus(TestCase):
def setUp(self):
self.app = create_app("TestingConfig")
payload = {"user_id": "test-id", "aud": "response_operations"}
self.client = self.app.test_client()
self.setup_data()
self.access_token = jwt.encode(payload, TestingConfig.UAA_PRIVATE_KEY, algorithm="RS256")
self.app.config["SESSION_REDIS"] = fakeredis.FakeStrictRedis(
host=self.app.config["REDIS_HOST"], port=self.app.config["FAKE_REDIS_PORT"], db=self.app.config["REDIS_DB"]
)
Expand All @@ -83,7 +93,10 @@ def setup_data(self):

@requests_mock.mock()
def test_get_available_status(self, mock_request):
mock_request.post(url_sign_in_data, json={"access_token": self.access_token}, status_code=201)
mock_request.get(url_get_survey_by_short_name, json=survey)
mock_request.get(url_permission_url, json=user_permission_reporting_unit_edit_json, status_code=200)
self.client.post("/sign-in", follow_redirects=True, data={"username": "user", "password": "pass"})
mock_request.get(url_get_collection_exercises_by_survey, json=collection_exercise_list)
mock_request.get(url_get_business_by_ru_ref, json=business_reporting_unit)
mock_request.get(url_get_available_case_group_statuses, json=self.statuses)
Expand Down Expand Up @@ -300,13 +313,17 @@ def test_respondent_name_not_in_metadata_for_completed_case_event(self, mock_req

@requests_mock.mock()
def test_not_started_status_is_present_for_completed_by_phone(self, mock_request):
mock_request.post(url_sign_in_data, json={"access_token": self.access_token}, status_code=201)
mock_request.get(url_get_survey_by_short_name, json=survey)
mock_request.get(url_permission_url, json=user_permission_reporting_unit_edit_json, status_code=200)
self.client.post("/sign-in", follow_redirects=True, data={"username": "user", "password": "pass"})
mock_request.get(url_get_collection_exercises_by_survey, json=collection_exercise_list)
mock_request.get(url_get_business_by_ru_ref, json=business_reporting_unit)
mock_request.get(url_get_available_case_group_statuses, json=self.statuses)
mock_request.get(url_get_case_groups_by_business_party_id, json=case_groups_completed_by_phone)
mock_request.get(url_get_case_events, json=case_events)
mock_request.get(url_get_case_by_case_group_id, json=[case])
mock_request.get(url_permission_url, json=user_permission_reporting_unit_edit_json, status_code=200)

response = self.client.get(f"/case/{ru_ref}/response-status?survey={short_name}&period={period}")

Expand All @@ -317,7 +334,10 @@ def test_not_started_status_is_present_for_completed_by_phone(self, mock_request

@requests_mock.mock()
def test_not_started_status_is_present_for_no_longer_required(self, mock_request):
mock_request.post(url_sign_in_data, json={"access_token": self.access_token}, status_code=201)
mock_request.get(url_get_survey_by_short_name, json=survey)
mock_request.get(url_permission_url, json=user_permission_reporting_unit_edit_json, status_code=200)
self.client.post("/sign-in", follow_redirects=True, data={"username": "user", "password": "pass"})
mock_request.get(url_get_collection_exercises_by_survey, json=collection_exercise_list)
mock_request.get(url_get_business_by_ru_ref, json=business_reporting_unit)
mock_request.get(url_get_available_case_group_statuses, json=self.statuses)
Expand All @@ -331,3 +351,23 @@ 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):
Copy link
Contributor

@LJBabbage LJBabbage Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mock_request.post(url_sign_in_data, json={"access_token": self.access_token}, status_code=201)
mock_request.get(url_get_survey_by_short_name, json=survey)
mock_request.get(url_permission_url, json=user_permission_reporting_unit_edit_json, status_code=200)
self.client.post("/sign-in", follow_redirects=True, data={"username": "user", "password": "pass"})
mock_request.get(url_get_collection_exercises_by_survey, json=collection_exercise_list)
mock_request.get(url_get_business_by_ru_ref, json=business_reporting_unit)
mock_request.get(url_get_available_case_group_statuses, json=self.statuses)
mock_request.get(url_get_case_groups_by_business_party_id, json=case_groups_no_longer_required)
mock_request.get(url_get_case_events, json=case_events)
mock_request.get(url_get_case_by_case_group_id, json=[case])

response = self.client.get(f"/case/{ru_ref}/response-status?survey={short_name}&period={period}")

data = response.data
self.assertEqual(response.status_code, 200)
self.assertIn(b"Completed", data)
self.assertIn(b"Not started", data)
Loading