feat(numericalInput): endpoint added to validate a numerical input

[jesusbalderramawgu]

Description

This is related to this issue [#1680].
We need to validate the numeric input value to see if is a valid math expression.

Useful information to include:

• this is gonna be useful inside Author because now numerical answers will be validate correctly when a problem block is created.

Other information

The frontend now will validate the input using this endpoint, in this PR you can find the whole conversation and the decision of this PR. Also you will find the instructions to test.
We are not affecting the current flow

Screenshots

• Valid expression

• Invalid expression

[openedx-webhooks]

Thanks for the pull request, @jesusbalderramawgu!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[kdmccormick]

Overall -- good approach! I like that it is not a ProblemBlock handler, because that would require having a ProblemBlock instance, which as we've discussed would be bad because we need the editor to function before the ProblemBlock is saved.

[kdmccormick]

In order to prevent abuse of this API by bots, can you validate the the user is logged in? Check out the IsAuthenticated class and how it's used in other API views.

[kdmccormick]

I think that it would be a better API contract if we returned 400 only if the request itself was bad (for example, if it was missing the formula field, or the formula was an array instead of a string, etc.)

In the case that the request is proper but the formula is bad, I recommend returning 200 but with is_valid: False in the response. That way, frontends which call this API can always just check the value of is_valid instead of having to check the return code and the valid of is_valid.

[kdmccormick]

I see that you took this logic from inputtypes.py, which is good. However, I'm concerned with duplicating the validation logic between the REST API and inputtypes.py.

(In general, when developing REST APIs, it's good to keep the business logic deeper in the code, and use the REST API only for permissions and basic schema validation.)

Here's what I recommend:

• Add a @classmethod to ProblemBlock called preview_numeric_input.
  • This can call preview_formcalc from inputtypes.py.
• In this REST API, call ProblemBlock.preview_numeric_input.

The benefit of this is that the validation logic is encapsulated within the ProblemBlock, but it is exposed in a way that a REST API like this one can use it.

[kdmccormick]

Can you use api_doc_tools so that this API shows up in the automated API docs?

Here's an example of how it's used: https://github.com/openedx/edx-platform/blob/master/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py#L104 . Note that the first line of the docstring becomes the short-description of the API, and the other lines of the docstring become the long-description.

[kdmccormick]

Could you define serializer classes for the API input and output? These come with built-in validation and will make the API docs better.

I think something like this would work (haven't tested, but you can poke around edx-platform for other examples):

from rest_framework import serializers

class NumericalInputValidationRequestSerializer(serializers.Serializer):
    formula = serializers.CharField()

class NumericalInputValidationReponseSerializer(serializers.Serializer):
    preview = serializers.CharField()
    is_valid = serializers.BooleanField()
    error = serializers.CharField(allow_null=True)

[kdmccormick]

Let's put this new API under rest_api/v2/urls.py instead. That's where all the newer stuff has been going lately.

[jesusbalderramawgu]

thank you @kdmccormick for your feedback.
I've updated the PR with your comments.
The only thing I couldn't do was to add the new endpoint to swagger, that was because I realized in my local I wasn't able to see the new V2 endpoints so I pulled the latest from master to check and then the swagger stopped working, there are errors there.
that is the only thing left to do ah and need to update the tests.

I made a change in the FE as well to point to the new path and I added a loader.
I appreciate your support with this, thank you

[jesusbalderramawgu]

I've fixed the issues that were blocking the CI checks

[bradenmacdonald]

@kdmccormick could you please take another look here?

[kdmccormick]

Sorry for the delay @jesusbalderramawgu ! Just one more comment , and then I think the code looks good.

Could you also explain in the PR description the steps which you took in order to manually test this? It's fine if you tested both this and openedx/frontend-app-authoring#2615 simultaneously, please just explain the testing steps on one of the PRs, and then add a note/link in the other PR.

[kdmccormick]

I presume that the import is here at the function-level in order to avoid a circular dependency between capa/inputtypes.py and capa_block.py, correct? When you see these sort of circular dependencies, it's best not to "fix" them using a function-level import, because that doesn't really fix the circularity, it just hides it.

In this part of the codebase, capa_block.py generally depends on the modules within capa/, not the other way around. Preserving this relationship makes the code easier to test and understand, and also will help us avoid circular dependencies from forming. Could you refactor the new code to preserve that relationship? I think, for example, the validation function could be defined here in inputtypes.py, and both this function and capa_block.py could call it.

[jesusbalderramawgu]

Thanks for your feedback, you are right.
I've updated the PR with your latest comments. Let me know if is what you expected or if it requires other change.

About the swagger, thanks to @brianjbuck-wgu it is working again, but for some reason is not loading properly in local the contentstore section with the list of endpoints, He is taking a look.

[kdmccormick]

Whoops, I didn't see this comment before I merged. @brianjbuck-wgu @jesusbalderramawgu , feel free to just open up a follow-up PR to fix the swagger docs.

[jesusbalderramawgu]

Sorry for the delay @jesusbalderramawgu ! Just one more comment , and then I think the code looks good.

Could you also explain in the PR description the steps which you took in order to manually test this? It's fine if you tested both this and openedx/frontend-app-authoring#2615 simultaneously, please just explain the testing steps on one of the PRs, and then add a note/link in the other PR.

Hi, Thank you for your response. I've updated this PR description with the steps that I follow to test this.

[kdmccormick]

Nice, thank you!