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

Support for multiple return to answer ids #1242

Merged
merged 28 commits into from
Nov 15, 2023

Conversation

petechd
Copy link
Contributor

@petechd petechd commented Oct 30, 2023

What is the context of this PR?

In this PR we modify return_to_answer_id to be a list of calculated summary ids and answer ids. The continue button forces new behaviour (returns to the calculated summary anchored at answer_id and pop it off the list so it becomes return_to_answer_id=calculated_summary_id). This change is similar to what we did to return_to_block_id and makes the page focus on the right elements on calculated summaries and grand calculated summaries.

How to review

Existing tests were modified and some new assertions were added where applicable (description of the test changed as well).

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

@petechd
Copy link
Contributor Author

petechd commented Oct 31, 2023

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Have only functionally reviewed so far as found a couple of issues in test_grand_calculated_summary_overlapping_answers

From calculated-summary-1 if I click a change link, and then press continue, the url has return_to_answer_id=q1-a1 but it should be None, because this goes back to the cs page anchored at q1-a1 so its not an answer to return to anymore

From the grand calculated summary, I was expecting return_to_answer_id=q1-a1,calculated-summary-1 on the answer page after clicking two change links, similar to how the other params have a list, but instead I only have q1-a1 on the answer, its not following the same pattern as return_to and return_to_block_id (which I think might be what causes the above)

@petechd
Copy link
Contributor Author

petechd commented Oct 31, 2023

Have only functionally reviewed so far as found a couple of issues in test_grand_calculated_summary_overlapping_answers

From calculated-summary-1 if I click a change link, and then press continue, the url has return_to_answer_id=q1-a1 but it should be None, because this goes back to the cs page anchored at q1-a1 so its not an answer to return to anymore

From the grand calculated summary, I was expecting return_to_answer_id=q1-a1,calculated-summary-1 on the answer page after clicking two change links, similar to how the other params have a list, but instead I only have q1-a1 on the answer, its not following the same pattern as return_to and return_to_block_id (which I think might be what causes the above)

First issue from your comment addressed, need to spend more time on the second.

@petechd
Copy link
Contributor Author

petechd commented Nov 1, 2023

Have only functionally reviewed so far as found a couple of issues in test_grand_calculated_summary_overlapping_answers

From calculated-summary-1 if I click a change link, and then press continue, the url has return_to_answer_id=q1-a1 but it should be None, because this goes back to the cs page anchored at q1-a1 so its not an answer to return to anymore

From the grand calculated summary, I was expecting return_to_answer_id=q1-a1,calculated-summary-1 on the answer page after clicking two change links, similar to how the other params have a list, but instead I only have q1-a1 on the answer, its not following the same pattern as return_to and return_to_block_id (which I think might be what causes the above)

The second issue fixed now (hopefully).

@MebinAbraham MebinAbraham added the dont merge Don't merge this PR label Nov 1, 2023
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

The return to answer id can still have too many items in it on a calc summary page, but have commented where I think causes it, looks correct on answer page now

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Functionally all working nicely now 👍 Just a couple minor comments
And I think there should be a test in test_router similar to the existing ones around multiple block ids for the multiple answer ids (might be able to just edit an existing one)

tests/app/views/contexts/summary/test_answer.py Outdated Show resolved Hide resolved
app/questionnaire/router.py Outdated Show resolved Hide resolved
app/views/contexts/summary/answer.py Show resolved Hide resolved
app/views/contexts/summary/answer.py Outdated Show resolved Hide resolved
@petechd
Copy link
Contributor Author

petechd commented Nov 7, 2023

Functionally all working nicely now 👍 Just a couple minor comments And I think there should be a test in test_router similar to the existing ones around multiple block ids for the multiple answer ids (might be able to just edit an existing one)

Added two tests in test_router that hopefully covers it all now.

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Changes and coverage all good 👍
The test_router tests test_return_to_calculated_summary_from_incomplete_section, test_return_to_grand_calculated_summary_from_answer and test_return_to_grand_calculated_summary_from_answer_incomplete_section and test_return_to_grand_calculated_summary_from_repeating_answer are all using multiple return tos and return to blocks, but not with your new answer change, and I think it would be good to fix so the tests are more similar to actual usage

@petechd
Copy link
Contributor Author

petechd commented Nov 13, 2023

Changes and coverage all good 👍 The test_router tests test_return_to_calculated_summary_from_incomplete_section, test_return_to_grand_calculated_summary_from_answer and test_return_to_grand_calculated_summary_from_answer_incomplete_section and test_return_to_grand_calculated_summary_from_repeating_answer are all using multiple return tos and return to blocks, but not with your new answer change, and I think it would be good to fix so the tests are more similar to actual usage

This one fixed now, thanks.

@MebinAbraham MebinAbraham removed the dont merge Don't merge this PR label Nov 13, 2023
…m:ONSdigital/eq-questionnaire-runner into support-for-multiple-return-to-answer-ids
@petechd petechd merged commit ef12715 into main Nov 15, 2023
15 checks passed
@petechd petechd deleted the support-for-multiple-return-to-answer-ids branch November 15, 2023 11:40
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.

4 participants