-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 updating dynamic required questions in field-list #6103
Conversation
new FormEntryPage("fieldlist-updates") | ||
.clickGoToArrow() | ||
.clickGoUpIcon() | ||
.clickOnGroup("Dynamic required question") |
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.
Could we create a new form for this so we don't need to use the form hierarchy to navigate in the test? I also find the smaller forms make it easier to comprehend what's going on in each test.
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.
I was thinking about that but I wanted to have this test in FieldListUpdateTest
as it's similar to other tests in that class. However FieldListUpdateTest
is started with that particular form directly so I would either move that test to a separate class or rework FieldListUpdateTest
to start at the main menu and allow me to choose a form. Do you think it makes sense?
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.
Alternatively, we could switch FieldListUpdateTest
to use FormEntryActivityTestRule
instead of BlankFormTestRule
? That would allow each test to set up a different form, but also not require them to go through the full app. For the moment, all the other tests could start with setUpProjectAndCopyForm
for the current fieldlist-updates
form and this one test could use a different one.
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.
That makes sense but this test is pretty big so let's not block this pr. I've filed a separate issue #6143
new FormEntryPage("fieldlist-updates") | ||
.clickGoToArrow() | ||
.clickGoUpIcon() | ||
.clickOnGroup("Dynamic required question") |
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.
Alternatively, we could switch FieldListUpdateTest
to use FormEntryActivityTestRule
instead of BlankFormTestRule
? That would allow each test to set up a different form, but also not require them to go through the full app. For the moment, all the other tests could start with setUpProjectAndCopyForm
for the current fieldlist-updates
form and this one test could use a different one.
Tested with Success! Verified on a device with Android 10 Verified Cases:
|
Tested with Success! Verified on a device with Android 14 |
Closes #5941
Why is this the best possible solution? Were any other approaches considered?
When we update questions on one screen we compare them before the changes and after (for example before answering one of the questions and after). If the same question is still exactly the same we do nothing otherwise we update it. The problem was that we didn't take into account whether a question was required while comparing the states. I've updated the logis and now everything is fine.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This should just fix the issue. I've updated the logic responsible for updating questions on one screen that might sound dangerous but the change is rather small and safe so I don't think we need to test a lot other similar cases.
Do we need any specific form for testing your changes? If so, please attach one.
The form attached to the issue.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest
(or./gradlew testLab
) and confirmed all checks still pass