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

Disabling the "Other" Option Now Disables the Other Text and Hides the Keyboard #2528

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

NudurupatiSurya
Copy link
Contributor

@NudurupatiSurya NudurupatiSurya commented Jun 28, 2024

Fixes #2399

Demo:

Screen_recording_20240628_123808.mp4

@... PTAL?

NudurupatiSurya and others added 25 commits April 9, 2024 18:09
* Initial Commit

* Working - 1

* Small change

* Updated Code based on the requirements

* Removed Signout from NavDrawer

* Modified code to retrieve user details using authentication manager
Fixed this issue: Disabling "other" option doesn't hide/disable "Other" text
Copy link
Collaborator

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

Can we have a unit test to test this scenario?

@gino-m
Copy link
Collaborator

gino-m commented Jul 1, 2024

@NudurupatiSurya I believe to test this scenario you would want to add a UI test to MultipleChoiceTaskFragmentTest, since a unit test on MultipleChoiceTaskViewModel might be too trivial. @anandwana001 to confirm.

@anandwana001
Copy link
Collaborator

@NudurupatiSurya I believe to test this scenario you would want to add a UI test to MultipleChoiceTaskFragmentTest, since a unit test on MultipleChoiceTaskViewModel might be too trivial. @anandwana001 to confirm.

Yes,
We do have a file MultipleChoiceTaskFragmentTest.
@NudurupatiSurya You can once check the MultipleChoiceTaskFragmentTest and confirm if you can write a similar test to confirm this Other option disable thing.

@anandwana001
Copy link
Collaborator

Hi @NudurupatiSurya
Feel free to ask any question to make this PR to its completion.
Thanks

@anandwana001
Copy link
Collaborator

Gentle Reminder @NudurupatiSurya

@NudurupatiSurya
Copy link
Contributor Author

MultipleChoiceTaskFragmentTest
Hi, Sorry for the delay. Thank you for your patience and feedback. I will update the PR by tomorrow.

@NudurupatiSurya
Copy link
Contributor Author

Hi,

I wanted to give you a quick update. Before adding the new tests, I ran the existing ones in MultipleChoiceTaskFragmentTest.kt and found that the following tests are failing due to the recent changes:

  • selects other option on text input and deselects other radio inputs
  • no deselection of other option on text clear when not required
  • no deselection of non-other selection when other is cleared
  • deselects other option on text clear and required

I am currently working on resolving this and adding the new tests as suggested. I'll reach out if I get stuck anywhere. I should have an update for you later this week.

Thanks!

@anandwana001
Copy link
Collaborator

Hi @NudurupatiSurya

All the tests are passing for me. Could you please tell me how you are running the test cases?
also, is your branch up-to-date?

Screenshot 2024-08-01 at 3 29 18 PM

@anandwana001
Copy link
Collaborator

Hi @NudurupatiSurya
Gentle Reminder

@gino-m
Copy link
Collaborator

gino-m commented Aug 7, 2024

@NudurupatiSurya Updated branch here and rerunning test on CI to keep things moving foward.

/gcbrun

@gino-m
Copy link
Collaborator

gino-m commented Aug 7, 2024

@anandwana001 Confirmed tests are failing with the following when merged with HEAD:


com.google.android.ground.ui.datacollection.tasks.multiplechoice.MultipleChoiceTaskFragmentTest > selects other option on text input and deselects other radio inputs FAILED
    androidx.test.espresso.PerformException at MultipleChoiceTaskFragmentTest.kt:176
        Caused by: java.lang.RuntimeException at MultipleChoiceTaskFragmentTest.kt:176

com.google.android.ground.ui.datacollection.tasks.multiplechoice.MultipleChoiceTaskFragmentTest > no deselection of other option on text clear when not required FAILED
    androidx.test.espresso.PerformException at MultipleChoiceTaskFragmentTest.kt:210
        Caused by: java.lang.RuntimeException at MultipleChoiceTaskFragmentTest.kt:210

com.google.android.ground.ui.datacollection.tasks.multiplechoice.MultipleChoiceTaskFragmentTest > no deselection of non-other selection when other is cleared FAILED
    androidx.test.espresso.PerformException at MultipleChoiceTaskFragmentTest.kt:227
        Caused by: java.lang.RuntimeException at MultipleChoiceTaskFragmentTest.kt:227

com.google.android.ground.ui.datacollection.tasks.multiplechoice.MultipleChoiceTaskFragmentTest > deselects other option on text clear and required FAILED
    androidx.test.espresso.PerformException at MultipleChoiceTaskFragmentTest.kt:190
        Caused by: java.lang.RuntimeException at MultipleChoiceTaskFragmentTest.kt:190

@NudurupatiSurya Any updates?

@anandwana001
Copy link
Collaborator

ok,

I have the latest update, and tests are passing for me.
Let me try again and get back.

@NudurupatiSurya
Copy link
Contributor Author

Hi @gino-m & @anandwana001,

I am currently in the process of some job interviews, so I couldn't reply back promptly.

@anandwana001, even after updating my current branch, these tests are failing

I think the tests are failing because the condition I added is overriding some of the otherText's previous behavior of selecting the other radio button when the user selects otherText.

All the failed tests are trying to write something in otherText before selecting the radio button. I removed the condition I wrote and these tests passed.

I will update my logic so that these tests will not fail and otherText shows the expected behavior. I will update this by next week as I am currently busy with job interviews.

Thank you for your understanding.

@jcqli
Copy link

jcqli commented Aug 22, 2024

Hi @NudurupatiSurya any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Select one - other] Deselecting "other" option doesn't hide/disable "Other" text
5 participants