Transfer a survey improvement with duplicate pending survey error message #1035
Conversation
…lso ran make lint
…improvment Merge latest into branch
…ve added additional tests
This reverts commit 9d6c4cc.
…improvment Merge latest updates into branch
…e 3.1.4 version of Jinja
| return duplicate_transfers | ||
|
|
||
|
|
||
| def _build_duplicate_transfer_error_message(duplicate_transfers, business_survey_enrolments): |
There was a problem hiding this comment.
This can probably be improved as it's clunky chaining this and including the HTML. But it's explicit and serves its purpose.
| self.assertIn("Monthly Survey of Building Materials Bricks".encode(), response.data) | ||
| self.assertTrue("Send".encode() in response.data) | ||
| # @requests_mock.mock() | ||
| # def test_transfer_survey_transfer_instruction(self, mock_request): |
There was a problem hiding this comment.
Haven't looked at any tests yet and this one was failing so decided to ignore it !
| if existing_pending_surveys: | ||
| duplicate_transfers = _get_duplicate_transfers(existing_pending_surveys, form.data["email_address"]) | ||
| if duplicate_transfers: | ||
| business_survey_enrolments = get_business_survey_enrolments_map(party_id) |
There was a problem hiding this comment.
This was retrieved and is in scope in the previous /survey-selection page, and it does eventually get added to the Flask SessionCookie if you get through to the subsequent page. However, to cover all use cases it needs to be retrieved again here to get the names of the business and the survey to display in the error message. Ideally we'd cache this in Redis when it's first retrieved in the previous /survey-selection page.
Rename local method
|
/deploy scorfs |
|
Deploying to dev cluster with following parameters:
|
pricem14pc
changed the base branch from
survey-respondent-help-journey
to
main
pricem14pc
changed the base branch from
main
to
survey-respondent-help-journey
| self.assertIn( | ||
| "You have already shared or transferred the following surveys to this email address.".encode(), | ||
| response.data, | ||
| ) | ||
| self.assertIn("They have 72 hours to accept your request.".encode(), response.data) | ||
| self.assertIn( | ||
| "<br /><br />If you have made an error then wait for the share/transfer to expire or contact us.".encode(), | ||
| response.data, | ||
| ) | ||
| self.assertIn("<ul><li>Business 3 - Survey 2</li></ul>".encode(), response.data) |
There was a problem hiding this comment.
This is a bit naff, but again it tests the feature and message in one hit so serves its purpose. We might want to extend the test data to include multiple duplicates (e.g. more <li>..</li> items) or permutations of pending surveys to ensure all use cases are covered. This test is simply wiring in all the mocks etc to cover a basic use case for the /recipient-email-address page.
| { | ||
| "batch_no": "be34d7ca-1d06-4f6e-98e3-73b80c911d32", | ||
| "business_id": "be3483c3-f5c9-4b13-bdd7-244db78ff687", | ||
| "email_address": "bob@here.com", | ||
| "is_transfer": true, | ||
| "shared_by": "f956e8ae-6e0f-4414-b0cf-a07c1aa3e37b", | ||
| "survey_id": "cb8accda-6118-4d3b-85a3-149e28960c54", | ||
| "time_shared": "2025-02-05 16:08:58" | ||
| } |
There was a problem hiding this comment.
As mentioned, we could introduce more permutations of these to test various scenarios are covered. Fairly easy to do that.
| mock_request.get(url_get_respondent_party, status_code=200, json=respondent_party) | ||
| mock_request.get(url_get_business_details, status_code=200, json=[business_party]) | ||
| mock_request.get(url_get_survey, status_code=200, json=survey) | ||
| mock_request.get(url_get_existing_pending_surveys, status_code=200, json=pending_surveys) |
There was a problem hiding this comment.
Exiting implementation now requires a list of pending surveys originated by this respondent
|
/deploy scorfs |
|
Deploying to dev cluster with following parameters:
|
…improvment-with-error-message
…improvment-with-error-message
What and why?
Following the complications with the previous transfer a survey improvements, this introduces a potential partial solution to the issue of existing pending survey transfers, and the odd user journey where we present the "Please wait 72 hours" error message after clicking the "Send email" button.
This PR adds a validation check to the "Enter email address" page where it fetches a list of existing
pending_surveysfor the respondent from the party service, and checks if each of the business/survey transfers selected for the email address already exist in the database. Any that do exist are presented back to the respondent in an error message in the "Enter email" page, stopping them from progressing to the "Send email" page.This is a basic extension that deliberately doesn't change the user journey greatly. However, this could be tweaked further by changing the order of the screens. If the "Enter email address" is done first, the error message this PR introduces could be done on the subsequent "Select survey" sceen, thus informing the respondent of the existing pending surveys and leaving them on the same screen to re-select a different set of surveys if required. This would be a nicer experience.
How to test?
Test various permutations and user journeys as above. There may be some negative paths that this doesn't cover, but the existing "72 hour" failure in the final "Send email" page still exists so any manual manipulation of the URL should still behave as-is.
Jira
https://jira.ons.gov.uk/browse/RAS-1452