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.

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-1200: Transfer a Survey #1028

Merged

Conversation

SteveScorfield
Copy link
Contributor

@SteveScorfield SteveScorfield commented Jan 17, 2025

What and why?

As part of the Frontstage survey respondent help journey improvement, the transfer of surveys needed to be updated and improved to reflect current requirements. This PR achieves this.

Please note that I have done a bit of refactoring to make the code more logical and I have removed what I believe to be code that is no longer required. There are more changes because of this.

How to test?

  1. Deploy this PR to your env
  2. Run the acceptance tests
  3. in rOps create a few different CEs for the same and different businesses
  4. Enrol the user on these CEs
  5. Then do the following:
    • Goto 'My Account'
    • Click 'I am no longer required to complete any surveys' in the 'Help with your account section'
    • Select a single CE and click continue
    • Follow the instructions until you are returned back to the 'Todo' section of the survey list
  6. Repeat step 5 as many times as you wish, with a different combinations of CEs and businesses
  7. Test errors:
    • Don't select a survey to transfer and hit continue
    • Don't add an email address and hit continue
    • Use a email address that has already received a transfer address for the same CE
  8. Ensure that the updates reflect the design pattern

Note: The design system has changed since the original Figma document was made. It was decided that using the same classnames for the padding/margins i.e. 'ons-u-mb-l'. The sizing will no longer match what is on the documents so check for the correct class names and not the sizes.

Jira

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

@SteveScorfield SteveScorfield requested a review from a team as a code owner January 17, 2025 14:22
@SteveScorfield
Copy link
Contributor Author

/deploy scorfs

@ras-rm-pr-bot
Copy link
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-1028

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

Copy link
Contributor

@LJBabbage LJBabbage left a comment

Choose a reason for hiding this comment

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

The happy path works, but a bit of refactoring and simplifying is needed. The unhappy path needs a look, and I found myself stuck a few times on route. This however might be rectified once useful links, menu options go in.
On a higher level, I found the lack of feedback on sharing multiple surveys if one fail poor. i.e if shared 10 and 1 was already done it just tells me no, but this isn't connected to this story.

I haven't currently looked at test coverage, due to the amount of changes requested

frontstage/controllers/party_controller.py Outdated Show resolved Hide resolved
frontstage/controllers/party_controller.py Outdated Show resolved Hide resolved
frontstage/controllers/party_controller.py Outdated Show resolved Hide resolved
frontstage/controllers/party_controller.py Outdated Show resolved Hide resolved
frontstage/templates/surveys/surveys-todo.html Outdated Show resolved Hide resolved
frontstage/views/account/account_transfer_survey.py Outdated Show resolved Hide resolved
frontstage/views/account/account_transfer_survey.py Outdated Show resolved Hide resolved
frontstage/views/account/account_transfer_survey.py Outdated Show resolved Hide resolved
frontstage/views/account/account_transfer_survey.py Outdated Show resolved Hide resolved
frontstage/views/surveys/surveys_list.py Outdated Show resolved Hide resolved
Copy link
Contributor

@arroyoAle arroyoAle left a comment

Choose a reason for hiding this comment

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

A few comments here and there, will just leave as comments as to not block when I'm not here

Makefile Outdated Show resolved Hide resolved
frontstage/controllers/party_controller.py Outdated Show resolved Hide resolved
@pricem14pc
Copy link
Contributor

pricem14pc commented Jan 27, 2025

If I make it through to the my-account/transfer-surveys/send-instruction page but don't click "Send Email", when I go back to the /todo page I still see the flash notification even though no pending_surveys have been created. This may be an existing undesired consequence of holding state in the flask session. Presumably that state would need updating following a successful "Send Email" and only flash the message if that's set. Alternatively the message should be flashed based on the pending_surveys DB records.

@pricem14pc pricem14pc closed this Jan 27, 2025
@pricem14pc pricem14pc reopened this Jan 27, 2025
@pricem14pc pricem14pc self-requested a review February 3, 2025 08:10
Copy link
Contributor

@pricem14pc pricem14pc left a comment

Choose a reason for hiding this comment

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

Approving, non-blocking comments notwithstanding

Copy link
Contributor

@arroyoAle arroyoAle left a comment

Choose a reason for hiding this comment

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

Looks good and tested, just one comment but does not block as its the same as it currently is

@LJBabbage LJBabbage dismissed their stale review February 4, 2025 11:22

I took over the pr

@LJBabbage LJBabbage merged commit 67e3d41 into survey-respondent-help-journey Feb 4, 2025
1 check passed
@LJBabbage LJBabbage deleted the transfer-a-survey-improvment branch February 4, 2025 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants