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

Jt/visually segmented case search group #1389

Merged
merged 16 commits into from
Dec 13, 2023

Conversation

Jtang-1
Copy link
Contributor

@Jtang-1 Jtang-1 commented Dec 5, 2023

Product Description

Parses the group block and populates a new queryGroup model. Also parses a group_key from prompt block and stores that in queryPrompt model. Both are added to the RemoteQueryDatum.

Technical Summary

Tech Spec
Jira Ticket
Research Doc

Formplayer PR: dimagi/formplayer#1511
HQ PR: dimagi/commcare-hq#33828

Safety Assurance

Safety story

Locally tested.
In parsing suite, the new group block and group-key for prompt block are optional so will not affect existing applications where these blocks do not exist.
groupKey in queryPrompt model is nullable. groupPrompts passed to remoteQueryDatum is instantiated as empty OrderedHashtable.

Automated test coverage

Will add tests

QA Plan

Will QA

Special deploy instructions

  • This PR can be deployed after merge with no further considerations.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations.

Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change.

Duplicate PR

Automatically duplicate this PR as defined in contributing.md.

@shubham1g5
Copy link
Contributor

Can we populate PR description to facilitiate review here. Thanks!

@Jtang-1 Jtang-1 marked this pull request as ready for review December 8, 2023 00:58
Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

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

Kudos for a well structured PR!

src/main/java/org/commcare/xml/QueryGroupParser.java Outdated Show resolved Hide resolved
src/main/java/org/commcare/xml/SessionDatumParser.java Outdated Show resolved Hide resolved
src/main/java/org/commcare/xml/SessionDatumParser.java Outdated Show resolved Hide resolved
src/main/java/org/commcare/xml/SessionDatumParser.java Outdated Show resolved Hide resolved
Copy link
Contributor

@nospame nospame left a comment

Choose a reason for hiding this comment

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

Makes sense overall, not sure I feel personally confident enough to approve.

@Override
public QueryGroup parse() throws InvalidStructureException, IOException, XmlPullParserException,
UnfullfilledRequirementsException {
checkNode("group");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use NAME_PROMPT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes 2b4ba65

Comment on lines 30 to 34
while (nextTagInBlock(NAME_PROMPT)) {
if (NAME_DISPLAY.equalsIgnoreCase(parser.getName())) {
display = parseDisplayBlock();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what this block is doing? It seems like display may get set multiple times here, not sure to what end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suite would look like this for a group block. nextTagInblock iterates through the elements within group (it automatically iterates to the next element within the block without needing to increment anything in the body of the while loop). The first would be display and that whole display block will be parsed via parseDisplayBlock. The parsed values will then be used to created an instance of QueryGroup so that instance can be used to pull values as needed.

<group key="group_header_0">
    <display>
      <text>
          <locale id="search_property.m0.group_header_0"/>
      </text>
    </display>
</group>

You're right that if there were multiple display blocks in the group element, only the last display block will be saved. But there should be only one

@Jtang-1 Jtang-1 requested a review from shubham1g5 December 13, 2023 00:52
@Jtang-1 Jtang-1 merged commit 34e5a81 into formplayer Dec 13, 2023
2 checks passed
@Jtang-1
Copy link
Contributor Author

Jtang-1 commented Dec 13, 2023

duplicate this PR 0556f8b 15a0d9e

@Jtang-1 Jtang-1 deleted the jt/visually-segmented-case-search-group branch December 13, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants