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

Jump to bottom

Support Stringtype, Integertype as answer option type when item type is choice. #1399

Merged
merged 9 commits into from
Jul 1, 2022
Merged

Support Stringtype, Integertype as answer option type when item type is choice. #1399

merged 9 commits into from
Jul 1, 2022

Conversation

santosh-pingle
Copy link
Collaborator

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

Fixes #1360

Description

  • Support IntegerType and StringType as answer option type when item type is Choice.
  • QuestionnaireResponse will always have answerOption type as Coding when item type is choice.

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type
Choose one: Bug fix

Screenshots (if applicable)

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).

@santosh-pingle santosh-pingle marked this pull request as ready for review May 18, 2022 13:43
@codecov
Copy link

codecov bot commented May 18, 2022 •
edited
Loading

Codecov Report

Merging #1399 (62b5597) into master (be98cd5) will decrease coverage by 0.00%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##             master    #1399      +/-   ##
============================================
- Coverage     90.08%   90.07%   -0.01%     
- Complexity      646      648       +2     
============================================
  Files           129      129              
  Lines          9839     9843       +4     
  Branches        713      716       +3     
============================================
+ Hits           8863     8866       +3     
  Misses          591      591              
- Partials        385      386       +1
Impacted Files Coverage Δ
...ogle/android/fhir/datacapture/MoreAnswerOptions.kt 66.66% <77.77%> (-2.57%) ⬇️
...droid/fhir/datacapture/QuestionnaireItemAdapter.kt 62.16% <100.00%> (+1.05%) ⬆️
...QuestionnaireItemCheckBoxGroupViewHolderFactory.kt 76.47% <100.00%> (ø)
...iews/QuestionnaireItemDropDownViewHolderFactory.kt 65.71% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be98cd5...62b5597. Read the comment docs.

@KhumboLihonga KhumboLihonga mentioned this pull request May 24, 2022
Closed
jingtang10
jingtang10 requested changes Jun 16, 2022
View reviewed changes
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.

can you please add integer|date|time|string|Coding|Reference(Any) as per the spec.

...com/google/android/fhir/datacapture/views/QuestionnaireItemCheckBoxGroupViewHolderFactory.kt Outdated Show resolved Hide resolved
jingtang10
jingtang10 requested changes Jun 21, 2022
View reviewed changes
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.

Santosh can you also implement this: #1360 (comment)

@jingtang10
Copy link
Collaborator

@santosh-pingle this is now a high priorty item - thanks for fixing this.

@santosh-pingle
Copy link
Collaborator Author

in progress.

@Tarun-Bhardwaj Tarun-Bhardwaj added the P1 High priority issue label Jun 27, 2022
jingtang10
jingtang10 requested changes Jun 30, 2022
View reviewed changes
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireItemAdapter.kt Show resolved Hide resolved
datacapture/src/test/java/com/google/android/fhir/datacapture/MoreAnswerOptionsTest.kt Outdated Show resolved Hide resolved
datacapture/src/test/java/com/google/android/fhir/datacapture/MoreAnswerOptionsTest.kt Outdated Show resolved Hide resolved
datacapture/src/test/java/com/google/android/fhir/datacapture/MoreAnswerOptionsTest.kt Outdated Show resolved Hide resolved
datacapture/src/test/java/com/google/android/fhir/datacapture/MoreAnswerOptionsTest.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/MoreAnswerOptions.kt
display
}
}
else -> throw IllegalArgumentException("$value is not supported.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

test this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To test else part, it is required to set value of QuestionnaireItemAnswerOptionComponent with not supported type, if you do so then fhir library api throws error.
throw new Error("Not the right type for Questionnaire.item.answerOption.value[x]: "+value.fhirType());

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok so the QuestionnaireItemAnswerOptionComponent class doesn't allow you to set a type that's not one of the types here. so it's sort of non-strict type safety. got it.

@santosh-pingle santosh-pingle requested a review from jingtang10 July 1, 2022 10:20
jingtang10
jingtang10 approved these changes Jul 1, 2022
View reviewed changes
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 @santosh-pingle

datacapture/src/main/java/com/google/android/fhir/datacapture/MoreAnswerOptions.kt
display
}
}
else -> throw IllegalArgumentException("$value is not supported.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok so the QuestionnaireItemAnswerOptionComponent class doesn't allow you to set a type that's not one of the types here. so it's sort of non-strict type safety. got it.

@santosh-pingle santosh-pingle merged commit 63fc76f into google:master Jul 1, 2022
This was referenced Aug 6, 2022
Closed
Closed
@jingtang10 jingtang10 linked an issue Aug 15, 2022 that may be closed by this pull request
Use 'choice' widgets to save to fields besides those of type <code> #1098
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingtang10 jingtang10 jingtang10 approved these changes

@aditya-07 aditya-07 Awaiting requested review from aditya-07

@kevinmost kevinmost Awaiting requested review from kevinmost

@ktarasenko ktarasenko Awaiting requested review from ktarasenko

@omarismail94 omarismail94 Awaiting requested review from omarismail94

@stevenckngaa stevenckngaa Awaiting requested review from stevenckngaa

@vvikramraj vvikramraj Awaiting requested review from vvikramraj

@wantingzhang77 wantingzhang77 Awaiting requested review from wantingzhang77

@yigit yigit Awaiting requested review from yigit

Assignees
No one assigned
Labels
P1 High priority issue
Projects
Android FHIR SDK
Archived in project
Milestone
No milestone
3 participants
@santosh-pingle @jingtang10 @Tarun-Bhardwaj