Skip to content

Input question on AnswerEdit is now a type=number field#2251

Merged
micahscopes merged 7 commits intolearningequality:developfrom
nucleogenesis:numerics-only-for-input-question
Sep 24, 2020
Merged

Input question on AnswerEdit is now a type=number field#2251
micahscopes merged 7 commits intolearningequality:developfrom
nucleogenesis:numerics-only-for-input-question

Conversation

@nucleogenesis
Copy link
Copy Markdown
Member

@nucleogenesis nucleogenesis commented Sep 11, 2020

Description

When editing an answer, Input fields can only accept numeric values.

Also - if you change from one question type to the Input question type any answers you've added to the previous question type will be removed if they include anything other than numbers.

The modal confirmation already says something along the lines of "if you change you will lose non-numeric answers".

Adds a validation string for non-numeric values:

new-string-numeric-validation

Issue Addressed (if applicable)

Fixes #2199

Steps to Test

NEW TO TEST:

In Chrome and Firefox, enter valid and invalid values. If you ever blur away from an invalid field, you should see the validation error. Note that e is allowed in an exponentiation context, + and - are allowed to sign the values and floats are valid as well.

Old instructions for testing below:

  1. Create a new exercise
  2. Create a "Single Select" question and add answers. Some of your questions should be number type (ie, 123123123 is what I mean but 1f2f or 123 456 are not - but you should test the latter examples too)
  3. Change the question to a "Input question" and confirm the modal
  4. You should now see all answers are correct (green) and the "valid" number type questions remain.
  5. Go to edit one of the remaining answers and try to type non-numerics and you should see nothing happen. Typing numbers, however, will work.

Please review the styling as well.

  • Unfocused number-type inputs have no underline
  • I removed the browser-native up/down number arrows harder to do than expected cross-browser while keeping the type="number"

Also test anything and everything related to creating questions with Input question type.

Implementation Notes (optional)

At a high level, how did you implement this?

  1. Revises the updateAnswersToQuestionType util to better handle changing to the Input question type so that it removes all non-numeric answers. Tested with regex /^[0-9]+$/
  2. Updates the template for the answer editor to show number type rather than markdown editor and applied the styling noted above.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 11, 2020

Codecov Report

Merging #2251 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2251   +/-   ##
========================================
  Coverage    81.48%   81.48%           
========================================
  Files          293      293           
  Lines        14066    14066           
========================================
  Hits         11462    11462           
  Misses        2604     2604           

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 c2c49af...5a0102e. Read the comment docs.

@MisRob
Copy link
Copy Markdown
Member

MisRob commented Sep 11, 2020

@nucleogenesis Could you please fix all failing tests and also cover the new situation in specs for both AnswersEditor and mapCorrectAnswers?

@MisRob
Copy link
Copy Markdown
Member

MisRob commented Sep 11, 2020

Components and utils related to assessment items editor have detailed test suite that is supposed to serve as documentation of various state too as there's quite lots of them, so it would be good to keep it up to date.

@nucleogenesis
Copy link
Copy Markdown
Member Author

Thanks @MisRob - I've updated the tests and all should pass now!

@MisRob
Copy link
Copy Markdown
Member

MisRob commented Sep 15, 2020

@nucleogenesis I've just tested new inputs on the local dev server and although I can't type in non-numeric values, it seems to be possible to copy-paste them.

@nucleogenesis
Copy link
Copy Markdown
Member Author

nucleogenesis commented Sep 17, 2020

@MisRob I'm not able to enter any character other than e - which I also thought was odd but apparently that's default for the type="number" HTML input field. A StackOverflow user made the point that this is because e is shorthand for exponentiation eg. 2e5 === 200,000.

I spent some time hacking at this but I didn't find it easy to ensure that the e doesn't appear. I can return to it later.

If you're experiencing something different (ie, you can put other letters than e) then perhaps there is something else going on that I'm not able to replicate.

Copy link
Copy Markdown
Contributor

@micahscopes micahscopes left a comment

Choose a reason for hiding this comment

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

I finally got a chance to dig into this and found that in Chrome, I was unable to enter non-numeric input. For some reason though, Firefox let me type in whatever I wanted. I was also able to paste non-numeric inputs in both Firefox and Chrome.

Just recently I worked on something similar to this for assessment items, using @input and @paste filters: https://github.com/learningequality/studio/pull/2098/files

The main difference there is that those filters were made for integers, whereas in this case I take it other kinds of numbers should also be allowed?

@nucleogenesis
Copy link
Copy Markdown
Member Author

nucleogenesis commented Sep 18, 2020

That's really helpful @micahscopes - I think this is going to be a bit more involved than I initially thought. If target browsers are not taking the type="number" into account in the same way, then we have to do it with JS. This means that we need to check input either on change/blur or on input and change the value of the text field directly as people type or otherwise move away - the UX being "I type a letter or symbol and see it immediately removed from my input" - which ought to also come with a warning/error saying "Only numbers 0-9, and e and so on"

Also - then we need to decide what kinds of non-numerics are allowed. Do we allow π or e for exponentiation or signed numbers + - or other common non-numeric arithmetic symbols alongside supporting floats?

Then for floats, do we force adding 0 to an input of .500? If we do that, then we have to do that across products because Kolibri will be using these answers to check against user inputs so we'd need to make sure however we assess answers given by Learners matches however we restrict formatting here.

I think that whatever decision we make should be included in the frontend both in Kolibri and Studio so that Learners and content creators are on the same page from the start about what kinds of inputs are accepted.

And piling on in general, the more complex this gets the more brittle we will end up getting because we're currently using regular expressions - saying "you can have +|- at the beginning or a . but if you start with a . then we prepend a 0 and if you use e it must be preceded AND followed by [0-9]+ and you can have as many e as you want but you can only ever have 1 . per number that precedes or follows an e if it is there...." and so on. So we have to match 5.124e10.101010e40e1000.0001 but not 1.004.00

So - in summary - the easiest thing to do is to allow signed floats because the rules for that are easy enough to do reliably with RegEx. We will need to apply the rules in Studio and Kolibri (possibly creating a KNumberTextField component to encapsulate this logic and make it easy to use cross-product).

Will return to this with some feedback from anybody who is interested - will raise a tension in next week's meeting.

(cc @jayoshih @MisRob )

@nucleogenesis
Copy link
Copy Markdown
Member Author

Also - our decision needs to consider backward compatibility. Studio currently permits e in the answers (appears to use same type="number" as I use in this PR) but in current Studio on Firefox, you can copy & paste anything you want into answers.

So this PR as it is seems to basically match current Studio - but that doesn't mean we shouldn't aim for improvement - we just need to make sure that we won't break people's exercises when they open them in the new Studio.

@nucleogenesis nucleogenesis force-pushed the numerics-only-for-input-question branch from 4eff6d5 to 33e284f Compare September 23, 2020 17:43
@nucleogenesis
Copy link
Copy Markdown
Member Author

@MisRob

I rebased this so it will be good to merge. Following up on our discussion in Monday's meeting, can you verify that Firefox isn't permitting characters besides e - even when copy-pasting?

Additionally - the conflict I fixed just added a <VSpacer> above the field so keep an eye out if that makes things look funny.

MisRob
MisRob previously requested changes Sep 24, 2020
Copy link
Copy Markdown
Member

@MisRob MisRob 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 verify that Firefox isn't permitting characters besides e - even when copy-pasting

Unfortunately, I can still write and also copy-paste non-numeric characters besides e in Firefox (80.0.1 64-bit, Ubuntu)

Selection_006

  1. One extra markdown editor component is rendered after adding a new answer. Please see the comment related to keep-alive.

Selection_001

Selection_002

@MisRob
Copy link
Copy Markdown
Member

MisRob commented Sep 24, 2020

@nucleogenesis Taking into account that it would be good to get this update in before the bug hunt, I think it's fine to merge this after the problem with one extra editor is fixed (2). Regarding (1), it's okay to open a follow-up issue.

cc @rtibbles

- Also ensure that all answers are cast to number type or removed
  when changing the kind of question it is.
and linting
- regex used to remove alphabetical answers when switching to question_input now accepts float and signed numbers
- tests test that incorrect data is removed properly
- tests also updated to account for using VTextField for question_input using shallowMount for one of the it() blocks since VTextField doesn't render output to the DOM with mount
removed some vendor styles that the linter got mad about.
@nucleogenesis nucleogenesis force-pushed the numerics-only-for-input-question branch from 7e2602b to 6abd2ce Compare September 24, 2020 18:45
@micahscopes micahscopes self-requested a review September 24, 2020 23:02
Copy link
Copy Markdown
Contributor

@micahscopes micahscopes left a comment

Choose a reason for hiding this comment

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

After much deliberation, @nucleogenesis and I decided to get this in for now, in spite of a remaining, relatively complex issue where Firefox users can still type and paste non-numeric input, since Firefox doesn't support type="number" on input elements.

I'm going to file a followup issue about the need for a good numeric input component in KDS tomorrow. VTextField doesn't quite do what we want for numeric input types. Ideally, we'd be able to block any input that doesn't pass the validation rules, but VTextField doesn't have a good way of doing that.

@micahscopes micahscopes dismissed MisRob’s stale review September 24, 2020 23:08

We decided to break out remaining issues into followup issues.

@micahscopes micahscopes merged commit a444fd2 into learningequality:develop Sep 24, 2020
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.

Input questions allow alphabetical input (should be numbers only)

4 participants