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

Repeated groups #1559

Merged
merged 4 commits into from
Nov 10, 2022
Merged

Repeated groups #1559

merged 4 commits into from
Nov 10, 2022

Conversation

kevinmost
Copy link
Contributor

@kevinmost kevinmost commented Aug 18, 2022

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #726

Description
Adds support for repeated groups.

Also added a sample form that uses repeated groups to record multiple COVID vaccine doses.

Several features are still missing. Given the size of this PR, it seems better to add those features in a follow-up. Most of those features would involve a header per instance of the repeated group which would allow for:

  • Expanding/collapsing that instance of the group (via a caret button, using the .isHidden extension, maybe?)
  • Deleting that instance of the group (via an X button)
  • Labeling that instance of the group with its index
  • Better visual clarity of where one group instance ends and the next starts

Alternative(s) considered
N/A

Type
Choose one: (Feature)

Screenshots (if applicable)

Example form with 2 group response instances:
image

Resulting JSON:
image

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

@jingtang10 jingtang10 mentioned this pull request Aug 21, 2022
7 tasks
@jingtang10 jingtang10 linked an issue Aug 21, 2022 that may be closed by this pull request
@kevinmost kevinmost force-pushed the km/repeatedGroups branch 2 times, most recently from 73c0694 to 61fb146 Compare September 14, 2022 19:07
@kevinmost kevinmost marked this pull request as ready for review September 14, 2022 19:51
@jingtang10 jingtang10 self-assigned this Sep 23, 2022
@jingtang10 jingtang10 changed the title WIP: Repeated groups Repeated groups Sep 23, 2022
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

thanks @kevinmost! this is amazing!

@Tarun-Bhardwaj
Copy link

Hi @kevinmost , could you please provide an ETA by when this can be done?

@kevinmost
Copy link
Contributor Author

I think this is ready for re-review, @jingtang10

@f-odhiambo
Copy link
Collaborator

f-odhiambo commented Oct 18, 2022

Thanks, @kevinmost Will review it. Not sure what is the issue with the failing tests CC @jingtang10

@kevinmost
Copy link
Contributor Author

Hm, the changes from #1670 seem to be breaking my tests (reverting that commit locally shows them working again).

The issue is definitely with my PR though (I agree that the ViewModel should be the source-of-truth for answers, and QuestionnaireItemViewItem should just be a snapshot).

Testing locally, the logic still seems to work. Any suggestions on how to restructure this test so it passes, @jingtang10?

@Tarun-Bhardwaj Tarun-Bhardwaj modified the milestones: Data Capture Library 0.1.0-beta06, Data Capture Library 0.1.0-beta07 Oct 24, 2022
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

thanks kevin - approved.

just a commen to add some tests for the group header widget. once that's done please feel free to merge

@jingtang10 jingtang10 assigned kevinmost and unassigned jingtang10 Nov 7, 2022
@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #1559 (92f3122) into master (b505aff) will decrease coverage by 0.13%.
The diff coverage is 28.84%.

@@             Coverage Diff              @@
##             master    #1559      +/-   ##
============================================
- Coverage     41.54%   41.41%   -0.14%     
+ Complexity      377      376       -1     
============================================
  Files           150      150              
  Lines          5300     5332      +32     
  Branches        952      960       +8     
============================================
+ Hits           2202     2208       +6     
- Misses         2767     2789      +22     
- Partials        331      335       +4     
Impacted Files Coverage Δ
...hir/datacapture/MoreQuestionnaireItemComponents.kt 35.32% <ø> (ø)
...e/views/QuestionnaireItemGroupViewHolderFactory.kt 42.85% <10.00%> (-18.26%) ⬇️
...android/fhir/datacapture/QuestionnaireViewModel.kt 48.93% <33.33%> (-1.79%) ⬇️
.../android/fhir/datacapture/QuestionnaireFragment.kt 68.96% <0.00%> (+0.78%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kevinmost kevinmost merged commit c888d5c into google:master Nov 10, 2022
ktarasenko pushed a commit to ktarasenko/android-fhir that referenced this pull request Nov 19, 2022
Support repeated groups
ktarasenko pushed a commit to ktarasenko/android-fhir that referenced this pull request Nov 19, 2022
Support repeated groups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Handle repeated groups
6 participants