Bugfix: Text Input problem type expects numeric answer when correct answer starts with a number by dmitry-viskov · Pull Request #19510 · openedx/openedx-platform

dmitry-viskov · 2018-12-26T15:44:23Z

Example:

>>Question.<<

= 100 test
or=test 100

Because the correct answer option (= 100 test) is a string that begins with the number, edX is treating the problem as though it requires a number answer, rather than a text string that can include letters and numbers. If the user enters an answer with text instead of just numbers, an error message that says "Error, couldn't parse formula" appears.

openedx-webhooks · 2018-12-26T15:44:30Z

Thanks for the pull request, @dmitry-viskov! I've created OSPR-2930 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

natabene · 2018-12-26T16:55:24Z

@dmitry-viskov This is great, thank you for your contribution. Please let me know once it is ready to be reviewed.

…nswer starts with a number

dmitry-viskov · 2018-12-27T10:39:10Z

@natabene hi. it is already ready for review. jenkins quality issues are not mine

natabene · 2018-12-27T15:29:59Z

@edx/testeng Could you help us out with jenkins/quality test here?

estute · 2018-12-27T22:35:48Z

@dmitry-viskov - I checked out the quality failures, and you are right- the build failed because you edited a file containing preexisting xsscommit-lint failures. The issue of running into older xss lint issues was recently discussed on another pull request: https://github.com/edx/edx-platform/pull/19385#issuecomment-448678120. From the PR:
Fixing the XSS warnings is strongly encouraged, but not strictly required. We have a backlog of these "might be a serious problem" code spots identified by the linter, and are trying to fix them as other work is being done in the same files (when the context of that part of the code is already fresh in mind). Documentation on how to avoid and fix these problems can be found here, and this is information that we'd like anybody working on HTML templates or code-generated HTML in Open edX to understand. But if there's a good case for not tackling the issues surfaced by xsscommitlint right now, we can merge the PR despite the linter failure.

dmitry-viskov · 2018-12-28T08:48:52Z

@natabene ^

natabene · 2018-12-28T16:23:32Z

@dmitry-viskov Got it, I will queue it for engineering review, though might take a while due to holidays.

natabene · 2018-12-28T16:24:10Z

@edx/educator-admins Can you have a look when you have a chance?

schenedx

I do prefer you can fix the linting issue identified by our quality check. Do you have a strong a reason not to do so?

schenedx · 2018-12-28T19:46:23Z

+                        checkIsNumeric = function(stringValue) {
+                            // remove OLX feedback
+                            if ((stringValue.indexOf('{{') !== -1) && (stringValue.indexOf('}}') !== -1)) {
+                                stringValue = stringValue.replace(/{{[\s\S]*?}}/g, '').trim();


Can you add some comment on what this replace is meant to do? Is it trying to remove all white space from this string?

= 3.14159 +- .02 {{ Pie for everyone! }}

->

= 3.14159 +- .02

dmitry-viskov · 2018-12-28T21:26:40Z

Can you add some comment on what this replace is meant to do? Is it trying to remove all white space from this string?

@schenedx sorry but i'm not going to fix your internal issues

schenedx · 2019-02-22T21:06:57Z

jenkins run quality

edx-status-bot · 2019-02-22T21:36:14Z

Your PR has finished running tests. The following contexts failed:

jenkins/quality

schenedx · 2019-02-23T20:22:27Z

@robrap I need your help here.
Since this PR touches the file https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/js/src/problem/edit.js, xsslinter would complain about all the xss issues it identified on this file. There are 24 of these issues. I am not even sure how to fix these. What should we do in this case?

robrap · 2019-02-24T15:01:17Z

@schenedx: Happy to look on Monday with you. We can determine effort of fixing or commenting, or allow merge with it broken.

schenedx

Upon review, I believe the code change here looks good. 👍
The xss-lint failures have nothing to do with this code change. We will merge this in as it is.

natabene · 2019-02-25T15:24:47Z

@schenedx Great, thanks! Please merge when you are ready to do so - OSPR contributors don't have permissions to merge.

openedx-webhooks · 2019-02-25T15:28:35Z

@dmitry-viskov 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2019-02-25T16:43:08Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, February 26, 2019.

edx-pipeline-bot · 2019-02-26T18:15:32Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Dec 26, 2018

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Dec 26, 2018

dmitry-viskov force-pushed the text-input-problem-pr branch from cc8bcb9 to 069f430 Compare December 27, 2018 09:44

Bugfix: Text Input problem type expects numeric answer when correct a…

a30a53a

…nswer starts with a number

dmitry-viskov force-pushed the text-input-problem-pr branch from 069f430 to a30a53a Compare December 27, 2018 10:11

wittjeff requested review from DawoudSheraz and ormsbee December 27, 2018 15:40

schenedx reviewed Dec 28, 2018

View reviewed changes

natabene requested a review from schenedx February 22, 2019 21:11

openedx-webhooks added engineering review awaiting prioritization and removed engineering review labels Feb 22, 2019

schenedx approved these changes Feb 25, 2019

View reviewed changes

nasthagiri merged commit 3b41189 into openedx:master Feb 25, 2019

dmitry-viskov deleted the text-input-problem-pr branch May 21, 2019 13:44

openedx-webhooks added merged and removed awaiting prioritization waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Dec 18, 2020

Uh oh!

Conversation

dmitry-viskov commented Dec 26, 2018

Uh oh!

openedx-webhooks commented Dec 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natabene commented Dec 26, 2018

Uh oh!

dmitry-viskov commented Dec 27, 2018

Uh oh!

natabene commented Dec 27, 2018

Uh oh!

estute commented Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitry-viskov commented Dec 28, 2018

Uh oh!

natabene commented Dec 28, 2018

Uh oh!

natabene commented Dec 28, 2018

Uh oh!

schenedx left a comment

Choose a reason for hiding this comment

Uh oh!

schenedx Dec 28, 2018

Choose a reason for hiding this comment

Uh oh!

dmitry-viskov Dec 28, 2018

Choose a reason for hiding this comment

Uh oh!

dmitry-viskov commented Dec 28, 2018

Uh oh!

schenedx commented Feb 22, 2019

Uh oh!

edx-status-bot commented Feb 22, 2019

Uh oh!

schenedx commented Feb 23, 2019

Uh oh!

robrap commented Feb 24, 2019

Uh oh!

schenedx left a comment

Choose a reason for hiding this comment

Uh oh!

natabene commented Feb 25, 2019

Uh oh!

openedx-webhooks commented Feb 25, 2019

Uh oh!

edx-pipeline-bot commented Feb 25, 2019

Uh oh!

edx-pipeline-bot commented Feb 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

openedx-webhooks commented Dec 26, 2018 •

edited

Loading

estute commented Dec 27, 2018 •

edited

Loading