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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 21, 2024
Merged

Support updating dynamic required questions in field-list #6103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
561a370
Added a failing test
grzesiek2010 Apr 20, 2024
b92e638
Fixed updating required questions in a field-list
grzesiek2010 Apr 20, 2024
bc38868
Improved the new test
grzesiek2010 Apr 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ...p/src/androidTest/java/org/odk/collect/android/feature/formentry/FieldListUpdateTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,22 @@ public void recordingAudio_ShouldChangeRelevanceOfRelatedField() {
.assertTextDoesNotExist("Target16");
}

@Test
public void changeInValueUsedToDetermineIfAQuestionIsRequired_ShouldUpdateTheRelatedRequiredQuestion() {
new FormEntryPage("fieldlist-updates")
.clickGoToArrow()
.clickGoUpIcon()
.clickOnGroup("Dynamic required question")
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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

.clickOnQuestion("Source17")
.assertQuestion("Target17")
.answerQuestion(0, "blah")
.assertQuestion("Target17", true)
.swipeToNextQuestionWithConstraintViolation(org.odk.collect.strings.R.string.required_answer_error)
.answerQuestion(0, "")
.assertQuestion("Target17")
.assertTextDoesNotExist(org.odk.collect.strings.R.string.required_answer_error);
}

// Scroll down until the desired group name is visible. This is needed to make the tests work
// on devices with screens of different heights.
private void jumpToGroupWithText(String text) {
Expand Down
7 changes: 7 additions & 0 deletions collect_app/src/androidTest/java/org/odk/collect/android/support/pages/FormEntryPage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,13 @@ public ErrorDialog swipeToNextQuestionWithError() {
return new ErrorDialog().assertOnPage();
}

public FormEntryPage swipeToNextQuestionWithConstraintViolation(int constraintText) {
flingLeft();
assertText(constraintText);

return this;
}

public FormEntryPage swipeToNextQuestionWithConstraintViolation(String constraintText) {
flingLeft();
assertText(constraintText);
Expand Down
9 changes: 8 additions & 1 deletion collect_app/src/main/java/org/odk/collect/android/logic/ImmutableDisplayableQuestion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ public class ImmutableDisplayableQuestion {
*/
private final boolean isReadOnly;

/**
* Whether the question is required.
*/
private final boolean isRequired;

/**
* The choices displayed to a user if this question is of a type that has choices.
*/
Expand All @@ -78,6 +83,7 @@ public ImmutableDisplayableQuestion(FormEntryPrompt question) {
guidanceText = question.getSpecialFormQuestionText(question.getQuestion().getHelpTextID(), "guidance");
answerText = question.getAnswerText();
isReadOnly = question.isReadOnly();
isRequired = question.isRequired();

List<SelectChoice> choices = question.getSelectChoices();
if (choices != null) {
Expand Down Expand Up @@ -106,7 +112,8 @@ public boolean sameAs(FormEntryPrompt question) {
&& (getGuidanceHintText(question) == null ? guidanceText == null : getGuidanceHintText(question).equals(guidanceText))
&& (question.getAnswerText() == null ? answerText == null : question.getAnswerText().equals(answerText))
&& (question.isReadOnly() == isReadOnly)
&& selectChoiceListsEqual(question.getSelectChoices(), selectChoices);
&& selectChoiceListsEqual(question.getSelectChoices(), selectChoices)
&& question.isRequired() == isRequired;
}

private static boolean selectChoiceListsEqual(List<SelectChoice> selectChoiceList1, List<SelectChoice> selectChoiceList2) {
Expand Down
15 changes: 15 additions & 0 deletions test-forms/src/main/resources/forms/fieldlist-updates.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,10 @@
<source16/>
<target16/>
</audio>
<required>
<source17/>
<target17/>
</required>
<meta>
<instanceID/>
</meta>
Expand Down Expand Up @@ -372,6 +376,8 @@
<bind nodeset="/fieldlist-updates/long_list_of_questions/question20" type="integer"/>
<bind nodeset="/fieldlist-updates/audio/source16" type="binary"/>
<bind nodeset="/fieldlist-updates/audio/target16" relevant=" /fieldlist-updates/audio/source16 != ''" type="string"/>
<bind nodeset="/fieldlist-updates/required/source17" type="binary"/>
<bind nodeset="/fieldlist-updates/required/target17" required=" /fieldlist-updates/required/source17 != ''" type="string"/>
<bind calculate="concat('uuid:', uuid())" nodeset="/fieldlist-updates/meta/instanceID" readonly="true()" type="string"/>
</model>
</h:head>
Expand Down Expand Up @@ -729,5 +735,14 @@
<label>Target16</label>
</input>
</group>
<group appearance="field-list" ref="/fieldlist-updates/required">
<label>Dynamic required question</label>
<input ref="/fieldlist-updates/required/source17">
<label>Source17</label>
</input>
<input ref="/fieldlist-updates/required/target17">
<label>Target17</label>
</input>
</group>
</h:body>
</h:html>