-
Notifications
You must be signed in to change notification settings - Fork 112
Conditional question fix for removing questions bug rails7 #3516
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
base: development
Are you sure you want to change the base?
Conditional question fix for removing questions bug rails7 #3516
Conversation
Generated by 🚫 Danger |
c388aa6
to
cca5a09
Compare
# rubocop pointed out that these variable is not used | ||
# all_answers = @plan.answers | ||
|
||
# Destroy all answers for removed questions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an important PR that fixes a number of lingering bugs. Also, I'm reading the "TODO:" and disabled rubocop offences at the top of this action:
# POST /answers/create_or_update
# TODO: Why!? This method is overly complex. Needs a serious refactor!
# We should break apart into separate create/update actions to simplify
# logic and we should stop using custom JSON here and instead use
# `remote: true` in the <form> tag and just send back the ERB.
# Consider using ActionCable for the progress bar(s)
# rubocop:disable Metrics/AbcSize, Metrics/MethodLength
# rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
I share the concerns expressed there and worry about the state of this controller action. It suggests creating separate create
and update
actions. Now it looks we are destroying answers within def create_or_update
as well. I'd like to branch off of this PR and make an attempt at cleaning things up here a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good if you can simplify.
In the case of a conditional question with answers that removed questions from different sections of a phase, any answers of removed questions were not removed, just hidden. Nor were the removed answers deleted in the database. Changes: - Added code to delete answers in AnswersController and ConditionsHelper in the database. - Added javascript code in answers/edit.js and utils/sectionUpdate.js to delete answers for removed questions. Rspec tests updated in next commit.
Changes: - added and updated tests for Conditional Questions. - Fixed a few broken RSpec tests by replacing click_link 'Write plan' --> click_link 'Write Plan'. - Updated CHANGELOG.md.
cca5a09
to
f236c8b
Compare
I had mentioned this bug in the following prior Conditional Question PR: #3497 (comment) The same bug seems to persist in this PR. I've created the following issue to track it: #3520 |
This a future suggestion. I like how the comments inform the user of the consequences of clicking the various buttons (e.g. "REMOVES: Q5 (checkbox)"). I know this was added explicitly to help with the testing. However, it could also improve user experience by helping them not delete answers by accident. Additionally/alternatively, in the future a warning could be sent before deleting any existing answers (e.g. have a confirmation button with a message like "Selecting this option will delete the following answers from your plan...") so they don't accidentally delete answers from their plan. |
rendancy and optimise code.
https://github.com/DMPRoadmap/roadmap/actions/runs/14705500339/job/41750808748 There is one intermittent test failure regarding
|
A Conditional Question (with action: "remove" and in a Phase with multiple sections) for this bug can be described structurally as follows:
Bug Description:
Currently in the code if we answer several questions in a phase in various sections, then subsequently answer an option of a Conditional question that removes some of the questions answered. The removed answers are hidden in UI, but the answers to the hidden questions remain persisted in the database and are hidden in the UI, not cleared. As a result if we change our answer to the same Conditional Question the removed questions with the "stale/old" answers reappear.
Expected Behavior
We would expect the removed questions answers to be deleted from the database and cleared from the UI. So if we change our answer to the same Conditional Question the removed questions should reappear without the "stale/old" answers. Further the removed questions could be in different sections in phase.
Testing
Attached is a script for testing in your development environment and running the content in a Rails console.
Test_script_for_setup_template_with_conditional_question.txt
We found convenient to use the seeds file to populate the database as follows:
We then brought up a Rails console and ran the content of the test script by copying and pasting.

You can view the template we created as the Admin User you logged in as.

Here is the structure of the conditional question we are testing, namely question
"Q2. Will your data be open? THE CONDITIONAL QUESTION"
Testing suggestions
With each branch login as user with email: [email protected] and password: password123 .
Create a plan with organisation Conditional Tests Organisation and no funder, e.g.,
Then fill in answers to following sets of questions:
You can use the same plan for both branches. You just need to fill in the values again to test each time.
Now answer question "Q2. Will your data be open? THE CONDITIONAL QUESTION" with
Yes, No and Don't know
As mentioned above, the branch for issue, should show the expected behaviour: