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

Create the structured data capture library #97

Merged
merged 52 commits into from Dec 23, 2020
Merged

Create the structured data capture library #97

merged 52 commits into from Dec 23, 2020

Conversation

jingtang10
Copy link
Collaborator

@jingtang10 jingtang10 commented Jul 22, 2020 •
edited

This module includes

  • QuestionnaireFragment that displays the Questionnaire and collects the QuestionnaireResponse from the user
  • QuestionnaireViewModel that works as the fragment's data backend

The QuestionnaireFragment class acts as the public API

jingtang10
jingtang10 commented Nov 27, 2020
View reviewed changes
Copy link
Collaborator Author

@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 florina!

i've decided to package this into a new module -- which can eventually be shipped as a separate library.

fhirengine/build.gradle Outdated Show resolved Hide resolved
fhirengine/src/main/java/com/google/fhirengine/ui/QuestionnaireFragment.kt Outdated Show resolved Hide resolved
@jingtang10 jingtang10 changed the title Create fragment that renders a Questionnaire resource and generates … Create a module for structured data collection Nov 27, 2020
@jingtang10 jingtang10 marked this pull request as ready for review December 2, 2020 19:15
@jingtang10 jingtang10 changed the title Create a module for structured data collection Create a module for structured data capture Dec 7, 2020
yigit
yigit reviewed Dec 8, 2020
View reviewed changes
datacapture/build.gradle Outdated Show resolved Hide resolved
datacapture/build.gradle Outdated Show resolved Hide resolved
datacapture/src/main/AndroidManifest.xml Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt Outdated Show resolved Hide resolved
datacapture/src/main/res/layout/questionnaire_fragment.xml Show resolved Hide resolved
datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelTest.kt Show resolved Hide resolved
example/src/main/java/com/google/fhirengine/example/QuestionnaireActivity.kt Outdated Show resolved Hide resolved
@jingtang10 jingtang10 force-pushed the questionnaire-rendering branch 2 times, most recently from 9880bad to 8b25a80 Compare December 15, 2020 09:51
@jingtang10 jingtang10 changed the title Create a module for structured data capture Create the structured data capture library and gallery application Dec 15, 2020
jingtang10
jingtang10 commented Dec 15, 2020
View reviewed changes
Copy link
Collaborator Author

@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 yigit for the review - ptal!

regarding integration tests vs unit tests, i actually think that test file itself is best kept as unit tests. it does not have any dependencies on UI or android components, it's purely about the logic to create a questionnaire response from a questionnaire and keep the same structure.

i will add more integration tests.

datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt Outdated Show resolved Hide resolved
datacapture/build.gradle Outdated Show resolved Hide resolved
datacapture/build.gradle Outdated Show resolved Hide resolved
@jingtang10
Copy link
Collaborator Author

@yigit sorry this PR has bloated quite a bit. If possible at all I'd love to be able to merge this asap as this will unblock future work as well for @pankajkatoch. Just wanted to make sure the core structure is good. thanks again.

yigit
yigit reviewed Dec 17, 2020
View reviewed changes
Copy link
Contributor

@yigit yigit left a comment

Choose a reason for hiding this comment

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

honestly i just skimmed through the PR because it is way too big to review properly :/

would be good to divide it up (e.g. merge skeleton, then merge type by type etc).

but also, i don't want to block the progress right before holidays so feel free to merge.

datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt Outdated Show resolved Hide resolved
...capture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireItemViewHolderType.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireItemAdapter.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireItemAdapter.kt Outdated Show resolved Hide resolved
...java/com/google/android/fhir/datacapture/views/QuestionnaireItemCheckBoxViewHolderFactory.kt Show resolved Hide resolved
...va/com/google/android/fhir/datacapture/views/QuestionnaireItemDatePickerViewHolderFactory.kt
// TODO: find a more robust way to do this.
val context = itemView.context as AppCompatActivity
DatePickerFragment().show(context.supportFragmentManager, DatePickerFragment.TAG)
context.supportFragmentManager.setFragmentResultListener(
Copy link
Contributor

Choose a reason for hiding this comment

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

we should take this out of the view model.
view model might disappear for various reasons.
ideally, such changes should be reflected just on the view model which can notify the change via submit list.

this CL is monster so might be a followup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you mean view holder?

so here the user is opening a new dialog to enter date, i think the challenge here is that view holder doesn't have the proper reference to the context (the cast here to appcompatactivity might crash if using non-appcompatactivity?).

datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireItemAdapterTest.kt Outdated Show resolved Hide resolved
jingtang10 added a commit that referenced this pull request Dec 17, 2020
@jingtang10
Remove data capture gallery from this branch to split PR #97
63d2b7d
jingtang10 added a commit that referenced this pull request Dec 17, 2020
@jingtang10
Re-add the data capture gallery module after it's been removed from t…
0c7789a 
…he branch in order to split PR #97
@jingtang10
Copy link
Collaborator Author

honestly i just skimmed through the PR because it is way too big to review properly :/

would be good to divide it up (e.g. merge skeleton, then merge type by type etc).

but also, i don't want to block the progress right before holidays so feel free to merge.

Thanks Yigit for the review I have separated the gallery app into a different branch now... Will do the same for the individual types.

With particularly big changes, would it make sense to merge them (with review) into a feature branch and then send a final review from feature branch to master branch? I know we have missed that boat for this PR now.

@jingtang10 jingtang10 changed the title Create the structured data capture library and gallery application Create the structured data capture library Dec 17, 2020
@jingtang10
Use map and flatten()
f731d4c
@jingtang10
Update the view holder type to 0-based to simplify the logic to get t…
72663aa 
…he enum from integer
@jingtang10
Remove unused test but leaving todo there.
ba204d0
@jingtang10
Create QuestionnaireResponseRecorder
3a259bf
@jingtang10
Use flatMap
51351f9
@jingtang10
Stop the adapter from referencing the view model
8c468ae 
- stop using ListAdapter and use RecyclerView.Adapter instead
- remove the QuestionnaireReponseRecorder interface as it's no longer needed
- create QuestionnaireItemViewItem data class to hold a pair of question and answer
- refactor QuestionnaireViewModel to create a list of QuestionnaireItemViewItem which is passed to the adapter
@jingtang10 jingtang10 mentioned this pull request Dec 18, 2020
Closed
jingtang10
jingtang10 commented Dec 18, 2020
View reviewed changes
Copy link
Collaborator Author

@jingtang10 jingtang10 left a comment •
edited

Choose a reason for hiding this comment

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

Thanks @yigit and Kashyap for your reviews. I think I addressed all comments now.

@yigit and I discussed the use of recycler view yesterday.

  • we will not use list adapter any more. we will use recycler view adapter instead. this is mainly because we don't really need it...
  • the view model will produce a list which is passed to the adapter. the adapter will no longer hold reference to the view model (which is incorrect)

Please take another look -- would love to merge this asap if poss :)

...capture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireItemViewHolderType.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireItemAdapter.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt Show resolved Hide resolved
...java/com/google/android/fhir/datacapture/views/QuestionnaireItemCheckBoxViewHolderFactory.kt Show resolved Hide resolved
...va/com/google/android/fhir/datacapture/views/QuestionnaireItemDatePickerViewHolderFactory.kt
// TODO: find a more robust way to do this.
val context = itemView.context as AppCompatActivity
DatePickerFragment().show(context.supportFragmentManager, DatePickerFragment.TAG)
context.supportFragmentManager.setFragmentResultListener(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you mean view holder?

so here the user is opening a new dialog to enter date, i think the challenge here is that view holder doesn't have the proper reference to the context (the cast here to appcompatactivity might crash if using non-appcompatactivity?).

jingtang10 added a commit that referenced this pull request Dec 18, 2020
@jingtang10
Re-add the data capture gallery module after it's been removed from t…
7fcfcfa 
…he branch in order to split PR #97
jingtang10 added a commit that referenced this pull request Dec 18, 2020
@jingtang10
Re-add the data capture gallery module after it's been removed from t…
08c8a1e 
…he branch in order to split PR #97
@jingtang10 jingtang10 mentioned this pull request Dec 19, 2020
Merged
deepankarb
deepankarb approved these changes Dec 23, 2020
View reviewed changes
Copy link
Contributor

@deepankarb deepankarb left a comment

Choose a reason for hiding this comment

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

Left a couple of nits. Approved!

datacapture/proguard-rules.pro Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/views/DatePickerDialogFragment.kt Outdated
import androidx.core.os.bundleOf
import androidx.fragment.app.DialogFragment
import androidx.fragment.app.setFragmentResult
import java.util.Calendar
Copy link
Contributor

Choose a reason for hiding this comment

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

We should switch to java.time

E.g.

LocalDate today = LocalDate.now();
int month = today.getMonthValue();
...

Perhaps that can be a dedicated PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks done. had to add suppress warning though - but is ok as long as user of library uses desugaring.

jingtang10
jingtang10 commented Dec 23, 2020
View reviewed changes
Copy link
Collaborator Author

@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!

datacapture/src/main/java/com/google/android/fhir/datacapture/views/DatePickerDialogFragment.kt Outdated
import androidx.core.os.bundleOf
import androidx.fragment.app.DialogFragment
import androidx.fragment.app.setFragmentResult
import java.util.Calendar
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks done. had to add suppress warning though - but is ok as long as user of library uses desugaring.

datacapture/proguard-rules.pro Outdated Show resolved Hide resolved
@jingtang10 jingtang10 merged commit 1065a23 into master Dec 23, 2020
@jingtang10 jingtang10 deleted the questionnaire-rendering branch December 23, 2020 19:00
jingtang10 added a commit that referenced this pull request Dec 23, 2020
@jingtang10
Re-add the data capture gallery module after it's been removed from t…
b4efa24 
…he branch in order to split PR #97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joiskash joiskash joiskash left review comments

@deepankarb deepankarb deepankarb approved these changes

@pankajkatoch pankajkatoch Awaiting requested review from pankajkatoch

@florina-muntenescu florina-muntenescu Awaiting requested review from florina-muntenescu

@yigit yigit Awaiting requested review from yigit

Assignees
No one assigned
Labels
None yet
Projects
Android FHIR SDK
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jingtang10 @yigit @florina-muntenescu @deepankarb @joiskash