-
Notifications
You must be signed in to change notification settings - Fork 166
validation added to NumericalInput to just accept numerical values on block creation form #2615
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
Open
jesusbalderramawgu
wants to merge
3
commits into
openedx:master
Choose a base branch
from
WGU-Open-edX:OEXCOM-245/AUTH-BLOCK-FORM-VALIDATION
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Numeric input problems can also include
sin,pi, and a few other functions.I would ask you to reference the docs (https://docs.openedx.org/en/latest/educators/references/course_development/exercise_tools/numerical_input.html), but those docs aren't entirely correct either 🤦🏻 In particular, the docs say that
gis allows, but this does not work in practice.I'll have to do some digging in the code to figure out what's really allowed; I'll circle back. In the meantime, can you try updating the regex to include
pi,sqrt,sin,arcscin,cos,arccos,tan,arctan,log2, and parenthases()?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, here's the function which perform the numeric evaluation: https://github.com/openedx/openedx-calc/blob/master/calc/calc.py#L239
And here's the call site: https://github.com/openedx/edx-platform/blob/f4f14a69874d5804ef33778486f7d6cab400e206/xmodule/capa/responsetypes.py#L1563
Note that both
variablesandunary_functionsare passed in as{}.@jesusbalderramawgu , would you or your team be able to reverse engineer this function in order to figure out exactly what is allowed in a numeric input formula?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I will check this with the team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, just to follow up on the ticket, I'm still checking because now needs more work the validation part.
After this feedback, the frontend shouldn't validate if is a valid math expression, it should be on the backend. After some review I found an endpoint to do this validation but it's really attached to django views, so I'm looking for a way of request this from react and validate the input component.
I'll keep you posted, thanks in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking that the validation might need to happen on the backend, glad to see you came to the same conclusion.
Perhaps we need to factor out a new REST API for the validation? Curious to hear what you find.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree on the new rest API for this validation, but gonna check with the team first in case they know something about how to reuse it.
this is where the validation is handled inside edx platform
xmodule/capa/inputtypes.pyin this function, line 1277
def preview_formcalc(self, get):it handles every formula as latex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a solution, I would like your thoughts on this.
after some digging and checking with @holaontiveros
we found that is possible to request the validation endpoint in this view, but it is required to save the numeric input first.
Basically when we select "problem" in the options and the modal opens up
a block is created with this endpoint
http://studio.local.openedx.io:8001/xblock/
returning a blockId which is required to validate the formula, but the input is not created because is not saved yet.
so we can't validate the formula without the creation of the input because even though we have a blockId it is like a temporary id until we create the input. We need to save the input first to get a valid id and be able to validate.
my proposal is to save an empty response (numerical input) when the modal opens up, that way we would have a valid id as reference and when the user types in the answer we can validate successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @jesusbalderramawgu , thanks for looking into this. That particular approach is not viable, because we need the editor to be able to work without saving the Problem first. You can see the importance of this by going into Libraries (Beta), creating a Problem, and hitting "Cancel" instead of saving: notice that no Problem is created.
(In Courses, on the other hand, if you being creating a Problem and hit "Cancel", you will end up with a blank problem which you have to then delete. This is an undesirable legacy behavior which we'll want to fix one day.)
I think we'll need a new endpoint, maybe one that just takes an answer string and validates it for a particular response type (formula, numeric, etc.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your feedback @kdmccormick, I checked what you said about the flow and I agree.
a new endpoint is required for this then, it should receive a string and return a boolean to know if is valid or not.
do you know who can help with the creation of this endpoint?