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

Bug fix: Backwards routing does not work when dependencies exist #1306

Merged
merged 6 commits into from
Jan 25, 2024

Conversation

MebinAbraham
Copy link
Contributor

@MebinAbraham MebinAbraham commented Jan 24, 2024

What is the context of this PR?

Fixes an issue with backwards routing due to us overwriting the reference to routing_path_block_ids when building the path. The variable routing_path_block_ids is passed to _evaluate_routing_rules by reference and mutated when routing backwards, however when dependent block ids exist, this reference is lost as we were re-assigning the variable: https://github.com/ONSdigital/eq-questionnaire-runner/blob/main/app/questionnaire/path_finder.py#L178-L181

This change ensures the reference is not overwritten, but instead we use a new variable and a copy of routing_path_block_ids. In future, we will look at rewriting pathfinder to not do such mutation which not only is unclear but prone to error as proven by this.

The updated schema simply moves the Confirmation question into the second section, which creates a when rule dependencies given the confirmation question's routing uses the first question. The additional dependencies are now populated because it is now in another section, previously it didn't need additional dependencies as it was part of the same routing path.

How to review

  1. Use the updated test schema from this PR on main.
  2. Get to the Confirmation Question (confirm-zero-employees-block) and select No I need to correct this
  3. Confirm that on submit, the answer is removed, but you are not routed back. You should still be on the same page.
  4. Checkout this branch
  5. Repeat step 2
  6. Confirm that on submit, you are routed back to the previous page (number-of-employees-total-block)

  1. Test the scenario attached on the card and confirm it is no longer an issue.
  2. Feel free to test any live surveys that have confirmation questions, which also has dependencies.

Since the issue is with a memory reference being overwritten, no new tests were needed other than updating the schema and changing some section ids in the existing test. You can ensure that the existing unit test test_remove_answer_and_block_if_routing_backwards is adequate but running this schema and unit test on main and you should see that the test fails as the routing paths do not match.

Checklist

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

@MebinAbraham MebinAbraham added the Bug Fix Something isn't working label Jan 24, 2024
Copy link
Contributor

@berroar berroar left a comment

Choose a reason for hiding this comment

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

Tested vs Main, changes fix the broken journey 👍

app/questionnaire/path_finder.py Outdated Show resolved Hide resolved
app/questionnaire/path_finder.py Show resolved Hide resolved
@MebinAbraham MebinAbraham merged commit a9c29e9 into main Jan 25, 2024
16 checks passed
@MebinAbraham MebinAbraham deleted the fix-backwards-routing-with-dependencies branch January 25, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants