Slider Question functionality added #287

MuhammadSalman-7214 · 2021-03-05T11:11:47Z

No description provided.

jingtang10 · 2021-03-05T13:05:19Z

Thanks @MuhammadSalman-7214! Can you please always make sure to link to the issue this PR is addressing (and close the issue after this is merged).

@joiskash do you have bandwidth to take a look at this? Thanks!

joiskash · 2021-03-05T13:15:51Z

Sure I'll have a look at this PR

joiskash

Add tests in the QuestionnaireItemAdapterTest and address the comments below :)

joiskash · 2021-03-05T13:39:59Z

datacapture/src/main/res/layout/questionnaire_item_slider.xml

+        android:id="@+id/slider"
+        android:layout_width="match_parent"
+        android:layout_height="wrap_content"
+        android:valueFrom="0.0"


I think these values should not be hardcoded. They must be obtained from the Questionnaire from sliderStepValue , maxValue & minValue. If not specified in the questionnaire then they must be in values.xml (?) @jingtang10 please comment

currently these values should remain hard coded as discussed with @jingtang10, and for their values, I am not able to find values.xml currently in the code

don't hardcode these in the xml file. hardcode these in the kotlin files.

done @jingtang10

joiskash · 2021-03-05T13:44:19Z

...e/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactoryInstrumentedTest.kt

+            ) {}
+        )
+
+        Truth.assertThat(viewHolder.itemView.findViewById<TextView>(R.id.slider_header).text)


Can we import com.google.common.truth.Truth.assertThat and just use assertThat? Just to keep it consistent with all the other Unit tests that have already been written

please comment again once done.

done @jingtang10

joiskash · 2021-03-05T13:51:50Z

...e/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactoryInstrumentedTest.kt

+    }
+
+    @Test
+    fun singleAnswerOrNull_singleAnswer_shouldReturnSingleAnswer() {


You have to write tests verifying that the slider is working as it is supposed to . Check the answerFalse_shouldSetCheckBoxUnchecked unit test in QuestionnaireItemCheckBoxViewHolderFactoryInstrumentedTest to get a better understanding. The assert statement should use the value returned from the viewHolder.findViewById(R.id.slider)

please comment again once done.

done @jingtang10

Please see https://github.com/google/android-fhir/blob/master/datacapture/src/androidTest/java/com/google/android/fhir/datacapture/views/QuestionnaireItemEditTextSingleLineViewHolderFactoryInstrumentedTest.kt. So there're two types of test cases, you have the ones that update the UI, and you have the ones that change the UI and update the questionnaire respoonse.

so you'll need to add at least another test case that tests the answer is set after you modify the slider.

joiskash · 2021-03-05T14:08:13Z

...n/java/com/google/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactory.kt

+                val questionnaireItem = questionnaireItemViewItem.questionnaireItem
+                val answer = questionnaireItemViewItem.singleAnswerOrNull
+                sliderHeader.text = questionnaireItem.text.value
+                val sliderValue = answer?.value?.integer?.value?.toString() ?: "0.0"


This cant be hardcoded.. imagine if the slider is from 5 to 10

for now, this is hard coded, will create a new issue for this as discussed with @jingtang10 to read values from extensions

yes i think we'll have to do this in a follow up and especially considering the ongoing work in #224

joiskash · 2021-03-05T14:10:10Z

...n/java/com/google/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactory.kt

+                                .setInteger(Integer.newBuilder().setValue(newValue.toInt())
+                                    .build()).build()
+
+                            questionnaireItemViewItem.singleAnswerOrNull = this


can you make this questionnaireItemViewItem.singleAnswerOrNull = for better readability

sorry, not understanding this. Can you explain this?

instead of questionnaireItemViewItem.singleAnswerOrNull = this inside the apply block can you refactor such that it is something like this

questionnaireItemViewItem.singleAnswerOrNull = QuestionnaireResponse.Item.Answer.newBuilder().apply { value = .... }

You can refer to the other factories to find this pattern

@joiskash but this is already doing the same way as QuestionnaireResponse.Item.Answer.newBuilder().apply in the code

Yes it is the same thing. Just suggesting to maintain uniformity in the code. Also why is the callback called inside apply?

do not set questionnaireItemViewItem.singleAnswerOrNull inside the apply

also keep questionnaireItemViewItem.questionnaireResponseItemChangedCallback() outside of apply

MuhammadSalman-7214 · 2021-03-08T12:40:05Z

@jingtang10 and @joiskash made the requested changes, please check, thanks.

jingtang10 · 2021-03-08T16:54:45Z

...e/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactoryInstrumentedTest.kt

+    }
+
+    @Test
+    fun singleAnswerOrNull_singleAnswer_shouldReturnSingleAnswer() {


Please see https://github.com/google/android-fhir/blob/master/datacapture/src/androidTest/java/com/google/android/fhir/datacapture/views/QuestionnaireItemEditTextSingleLineViewHolderFactoryInstrumentedTest.kt. So there're two types of test cases, you have the ones that update the UI, and you have the ones that change the UI and update the questionnaire respoonse.

so you'll need to add at least another test case that tests the answer is set after you modify the slider.

jingtang10 · 2021-03-08T16:55:23Z

...e/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactoryInstrumentedTest.kt

+    }
+
+    @Test
+    fun singleAnswerOrNull_singleAnswer_shouldReturnSingleAnswer() {


Suggested change

fun singleAnswerOrNull_singleAnswer_shouldReturnSingleAnswer() {

fun shouldSetSliderValue() {

jingtang10 · 2021-03-08T16:56:22Z

...e/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactoryInstrumentedTest.kt

+    }
+
+    @Test
+    fun singleAnswerOrNull_multipleAnswers_shouldReturnNull() {


Suggested change

fun singleAnswerOrNull_multipleAnswers_shouldReturnNull() {

fun shouldSetSliderValueToDefault() {

jingtang10 · 2021-03-08T17:02:35Z

...n/java/com/google/android/fhir/datacapture/views/QuestionnaireItemSliderViewHolderFactory.kt

+                                .setInteger(Integer.newBuilder().setValue(newValue.toInt())
+                                    .build()).build()
+
+                            questionnaireItemViewItem.singleAnswerOrNull = this


do not set questionnaireItemViewItem.singleAnswerOrNull inside the apply

also keep questionnaireItemViewItem.questionnaireResponseItemChangedCallback() outside of apply

MuhammadSalman-7214 · 2021-03-09T08:26:15Z

#287 (comment)
@jingtang10 to take callback out of apply is fine, but how can I take questionnaireItemViewItem.questionnaireResponseItemChangedCallback() outside of apply? as it requires assigning within this apply block. Please mention. Thanks.

jingtang10 · 2021-03-09T09:54:49Z

thanks @MuhammadSalman-7214. I see you have moved the callback call outside of the apply function.

Have you read this comment from @joiskash https://github.com/google/android-fhir/pull/287/files#r589390062 ?

so maybe this is where the confusion comes from, have you read about scope functions https://kotlinlang.org/docs/scope-functions.html?

you need to do the assignment outside of the apply function:

something = something.newBuilder().apply{...}

MuhammadSalman-7214 · 2021-03-09T10:58:46Z

thanks @MuhammadSalman-7214. I see you have moved the callback call outside of the apply function.

Have you read this comment from @joiskash https://github.com/google/android-fhir/pull/287/files#r589390062 ?

so maybe this is where the confusion comes from, have you read about scope functions https://kotlinlang.org/docs/scope-functions.html?

you need to do the assignment outside of the apply function:

something = something.newBuilder().apply{...}

@jingtang10 yes I have already read about scope functions, just there was a bit confusion, but now everything is done. All other requested changes are done as well. Thanks.

jingtang10

thanks @MuhammadSalman-7214

MuhammadSalman-7214 · 2021-03-09T15:03:04Z

thanks @MuhammadSalman-7214

your welcome @jingtang10 👍

jingtang10 · 2021-03-10T21:22:40Z

#294 has been merged. So please rebase your branch onto master (or merge master into your feature branch if rebase is too difficult) and follow this section in the developer's guide to set up your environment to use the new coding conventions. Thanks!

# Conflicts: # datacapture/src/main/java/com/google/android/fhir/datacapture/MoreQuestionnaireItemExtensions.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireItemAdapter.kt # datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireItemViewHolderType.kt # datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireItemViewHolderTypeTest.kt

…ogle/android-fhir into ms/add-handle-slider-question

Because all the changes have been done already

salman added 2 commits March 5, 2021 16:09

Slider Question Functionality Added

dd698ed

Merge branch 'master' into ms/add-handle-slider-question

18fa9dd

MuhammadSalman-7214 requested review from deepankarb, jingtang10, joiskash, pankajkatoch and yigit as code owners March 5, 2021 11:11

jingtang10 enabled auto-merge March 5, 2021 11:42

jingtang10 disabled auto-merge March 5, 2021 11:42

salman added 2 commits March 5, 2021 16:50

Spotless Apply Formatting Done

35c7a44

One Failing test case has been corrected

b133777

joiskash previously requested changes Mar 5, 2021

View reviewed changes

Catered some comments

c090554

MuhammadSalman-7214 requested a review from joiskash March 8, 2021 12:32

Moved hardcoded values from xml to kotlin file

b4f516a

jingtang10 requested changes Mar 8, 2021

View reviewed changes

Comments Handled

6b7de2d

MuhammadSalman-7214 requested a review from jingtang10 March 9, 2021 09:47

All Changes Requested Done

e720019

jingtang10 approved these changes Mar 9, 2021

View reviewed changes

Merge branch 'master' into ms/add-handle-slider-question

904d4d4

salman and others added 5 commits March 11, 2021 20:08

Resolved Conflicts and Updated the code with latest Coding Conventions

78fad29

Merge branch 'ms/add-handle-slider-question' of https://github.com/go…

1f39818

…ogle/android-fhir into ms/add-handle-slider-question

Merge branch 'master' into ms/add-handle-slider-question

5944688

Merge branch 'master' into ms/add-handle-slider-question

d44fe9d

Merge branch 'master' into ms/add-handle-slider-question

1b02ebf

MuhammadSalman-7214 merged commit a3c4916 into master Mar 13, 2021

MuhammadSalman-7214 deleted the ms/add-handle-slider-question branch March 13, 2021 07:13

s-ayush2903 mentioned this pull request Mar 16, 2021

Add slider control #201

Closed

jingtang10 linked an issue Mar 16, 2021 that may be closed by this pull request

Add slider control #201

Closed

	fun singleAnswerOrNull_singleAnswer_shouldReturnSingleAnswer() {
	fun shouldSetSliderValue() {

	fun singleAnswerOrNull_multipleAnswers_shouldReturnNull() {
	fun shouldSetSliderValueToDefault() {

Slider Question functionality added #287

Slider Question functionality added #287

Conversation

MuhammadSalman-7214 commented Mar 5, 2021

jingtang10 commented Mar 5, 2021 • edited Loading

joiskash commented Mar 5, 2021

joiskash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuhammadSalman-7214 Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingtang10 Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuhammadSalman-7214 commented Mar 8, 2021

jingtang10 Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuhammadSalman-7214 commented Mar 9, 2021 • edited Loading

jingtang10 commented Mar 9, 2021

MuhammadSalman-7214 commented Mar 9, 2021 • edited Loading

jingtang10 left a comment

Choose a reason for hiding this comment

MuhammadSalman-7214 commented Mar 9, 2021

jingtang10 commented Mar 10, 2021

jingtang10 commented Mar 5, 2021 •

edited

Loading

MuhammadSalman-7214 Mar 8, 2021 •

edited

Loading

jingtang10 Mar 8, 2021 •

edited

Loading

jingtang10 Mar 8, 2021 •

edited

Loading

MuhammadSalman-7214 commented Mar 9, 2021 •

edited

Loading

MuhammadSalman-7214 commented Mar 9, 2021 •

edited

Loading