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

Text-based interactions need real-time answer validation #376

Closed
BenHenning opened this issue Nov 16, 2019 · 14 comments
Closed

Text-based interactions need real-time answer validation #376

BenHenning opened this issue Nov 16, 2019 · 14 comments
Assignees
Labels
Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

Although we limit the characters we can freely input to text-based interactions, it's still possible to fill in impossible answers that will likely fail or crash the app, e.g. '2/3/4/3' in a fractions lesson.

We should have an error label appear when an incorrect answer is typed in a text-based interaction and prevent answer submission.

@seanlip
Copy link
Member

seanlip commented Nov 16, 2019 via email

@nikitamarysolomanpvt
Copy link
Contributor

nikitamarysolomanpvt commented Nov 18, 2019

@BenHenning I had gone through https://github.com/oppia/oppia-android/blob/develop/app/src/main/java/org/oppia/app/parser/StringToFractionParser.kt updates from your end. Seems even some valid fraction inputs are failing with the regx used. I had updated the test cases as per the new changes.
Please suggest the expected changes as per #376 issue.

Note that the interactions in the web version have custom logic for this. Probably worth following it exactly.

@seanlip you mean https://github.com/oppia/oppia/blob/develop/extensions/interactions/FractionInput/directives/oppia-interactive-fraction-input.directive.ts as

custom logic for this

Or
Do you want me to use
https://github.com/oppia/oppia/blob/develop/core/templates/dev/head/domain/objects/FractionObjectFactory.ts
or other validations in files of
https://github.com/oppia/oppia/tree/develop/extensions/interactions/FractionInput/directives folder
Adding Banglore team in loop @veena14cs @rt4914

@seanlip
Copy link
Member

seanlip commented Nov 19, 2019

@BenHenning
Copy link
Member Author

That's interesting. @nikitamarysolomanpvt which inputs fail?

I'm not against using the web code, but mine was meant to be a simplification, actually. I intended for it to be correct, so I'm curious which inputs make it fail.

@BenHenning
Copy link
Member Author

BenHenning commented Nov 19, 2019

Aha, @nikitamarysolomanpvt you spotted a bug with the denominator patterns. I accidentally kept the + out of the parentheses. The following should be the correct patterns:

  private val wholeNumberOnlyRegex = """^-? ?(\d+)$""".toRegex()
  private val fractionOnlyRegex = """^-? ?(\d+) ?/ ?(\d+)$""".toRegex()
  private val mixedNumberRegex = """^-? ?(\d+) ?(\d+) ?/ ?(\d+)$""".toRegex()

The following bugs existed:

  • Fraction-only inputs would not retrieve denominators with more than one digit
  • Mixed number inputs would not retrieve whole numbers with more than one digit
  • Mixed-number inputs would not retrieve denominators with more than one digit

I'm pretty sure these inputs are now right, but we really ought to add tests for them to be sure.

@nikitamarysolomanpvt
Copy link
Contributor

nikitamarysolomanpvt commented Nov 19, 2019

Aha, @nikitamarysolomanpvt you spotted a bug with the denominator patterns. I accidentally kept the + out of the parentheses. The following should be the correct patterns:

  private val wholeNumberOnlyRegex = """^-? ?(\d+)$""".toRegex()
  private val fractionOnlyRegex = """^-? ?(\d+) ?/ ?(\d+)$""".toRegex()
  private val mixedNumberRegex = """^-? ?(\d+) ?(\d+) ?/ ?(\d+)$""".toRegex()

The following bugs existed:

  • Fraction-only inputs would not retrieve denominators with more than one digit
  • Mixed number inputs would not retrieve whole numbers with more than one digit
  • Mixed-number inputs would not retrieve denominators with more than one digit

I'm pretty sure these inputs are now right, but we really ought to add tests for them to be sure.

@BenHenning
Ya and also normalizeWhitespace(), bug with the wholenumber patterns(fails when its more than one digit).

@rt4914
Copy link
Contributor

rt4914 commented Nov 25, 2019

@nikitamarysolomanpvt This issue is somewhat connected to #419, please have a look.

@rt4914
Copy link
Contributor

rt4914 commented Nov 26, 2019

My interpretation of this issue:
While learner is typing an answer, if they enter invalid answer like 3/4/5/6 we should provide label for invalid answer even when learner has not even clicked on the submit button.

@BenHenning Is this what you mean or something else?

@nikitamarysolomanpvt @seanlip

@seanlip
Copy link
Member

seanlip commented Nov 26, 2019

Yes, that's correct. You can see an example of the expected behaviour in Q2 of this exploration: https://www.oppia.org/explore/rfX8jNkPnA-1?collection_id=4UgTQUc1tala

@rt4914
Copy link
Contributor

rt4914 commented Nov 26, 2019

Yes, that's correct. You can see an example of the expected behaviour in Q2 of this exploration: https://www.oppia.org/explore/rfX8jNkPnA-1?collection_id=4UgTQUc1tala

Okay. @nikitamarysolomanpvt check this, maybe this can clarify things.

@BenHenning
Copy link
Member Author

I suspect that this issue only affects fractions since text & numeric input shouldn't be able to have incorrect values passed in (though maybe numeric does if you paste in something without numbers?)

@seanlip
Copy link
Member

seanlip commented Nov 30, 2019

Yep -- there's no issue with text. For numeric, assuming only numbers, negative signs and dots can be entered in the field, you could have things like: (a) blank input, (b) multiple dots, (c) multiple negative signs, (d) negative signs in the middle of the number, (e) dot before the negative sign.

nikitamarysolomanpvt added a commit that referenced this issue Jan 22, 2020
* nit

* UI hi-fi for text,number,and fraction input views

* UI hi-fi for text,number,and fraction input views

* UI hi-fi for text,number,and fraction input views nit

* nit

* nit

* test cases update

* accent color

* input type in fraction input type

* input type in fraction input type

* Merge branches 'develop' and 'hi-fi-input-interaction-views' of https://github.com/oppia/oppia-android into hi-fi-input-interaction-views

# Conflicts:
#	app/src/main/res/layout/text_input_interaction_item.xml

* text color in input type views

* changed inputtype in edit text

* margin updated in input views

* nit

* keyboardhelper to handle softinoutkeyboard

* Edit text focus removed.
On click of input type interaction item, it requires two clicks to display keyboard, which should actually be just a single click.
thus preventing scroll due to edit text focus

* as per review suggestion added binding.stateRecyclerView.smoothScrollToPosition(0) in processEphemeralStateResult

* nit

* Fix-406

* nit changes and keybord helper class renamed.

* nit

* kdoc for keyboardhelper.nit changes

* kdoc for keyboardhelper

* nit

* nit

* nit

* nit

* nit

* validation in fraction input

* nit

* nit

* nit

* errorcode enum

* errorcode enum

* nit

* nit

* nit

* nit

* error text on Fraction input

* error text on Fraction input

* nit

* nit

* updated FractionParsingErrors Enum with string resources, added getPendingAnswerError in InteractionAnswerHandler,in error text of fraction set minimum height 32dp and text size 12sp ,color code updated,and othere nit changes

* nit

* nit

* nit

* Merge conflict issue fix
Merge branches 'develop' and 'hi-fi-input-interaction-views-validation' of https://github.com/oppia/oppia-android into hi-fi-input-interaction-views-validation

# Conflicts:
#	app/src/main/java/org/oppia/app/parser/StringToFractionParser.kt
#	app/src/main/java/org/oppia/app/player/state/itemviewmodel/ContinueInteractionViewModel.kt
#	app/src/main/java/org/oppia/app/player/state/itemviewmodel/FractionInteractionViewModel.kt
#	app/src/main/java/org/oppia/app/player/state/itemviewmodel/InteractionViewModelModule.kt
#	app/src/main/java/org/oppia/app/player/state/itemviewmodel/NumericInputViewModel.kt
#	app/src/main/java/org/oppia/app/player/state/itemviewmodel/TextInputViewModel.kt
#	app/src/main/res/values/strings.xml

* not showing error on fiest - symbol in fraction input, new method for submit button click

* not showing error on fiest - symbol in fraction input, new method for submit button click

* nit

* partial answer removed error message display by regex for partial values

* nit import optimisation changes reverted

* nit import optimisation changes reverted

* nit import optimisation changes reverted

* carsh fix in EditTextBindingAdapters

* on submit button displays error for partial input and divided by 0

* instance check

* moved FractionParsingError to StringToFractionParser,
In InteractionAnswerHandler added isExplicitErrorCheckRequired, and updated existing with getPendingAnswerErrorOnSubmit and other changes required for the above.

* color names updated casing

* Introduced disparity between the patterns used to validate vs the ones we use to parse. Suggested by ben.

* test cases for error messages.

* nit

* partial mixed fraction issue fix

* nit

* nit

* nit

* added few testcases

* nit changes.

* parseFunction introduced and the helper is used for both for parsing and validation

* getter and setter for PendingAnswerError in FractionInteractionViewModel, removed valid string resource and returns string "valid" inside enum,
setPendingAnswerError,hasPendingAnswerError flags in InteractionAnswerHandler, and other nit

* removed digit filter in test activity nit

* removed digit filter in test activity nit

* added new method onAnswerRealTimeError in InteractionAnswerHandler

* added new method onAnswerRealTimeError in InteractionAnswerHandler

* state fragment is added as parameter to FractionInteractionViewModel

* submit button active/inactive on realtime error implementation on StateFragmentPresenter

* nit changes

* nit changes, InputInteractionViewTestActivity updated with new realtime error implemenatations and submit button enable and disable.

* submit button issue fix.

* interactionVieewModelModule code fix for on continue button click for multiplechoice issue/crash.

* nit

* setting error on submit is moved to stateFragmentPresenter

* nit

* InteractionAnswerErrorReceiver

* 3rd approach

* fraction input validation final approach

* nit

* nit

* nit

* nit

* New fraction submit time error "None of the numbers of the fraction should be larger than 7 digits."

* test-case to check long number, nit changes ,kDoc

* nit
@BenHenning
Copy link
Member Author

@nikitamarysolomanpvt what work remains for this issue?

@BenHenning BenHenning added this to the Alpha milestone Jun 23, 2020
@BenHenning
Copy link
Member Author

Looks like this has been done #419 and #640.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

No branches or pull requests

4 participants