Skip to content

More descriptive error messages for UndefinedVariable - #18158

Merged
pdpinch merged 1 commit into
openedx:masterfrom
mitodl:pdpinch/undefined-var-error
Sep 18, 2018
Merged

More descriptive error messages for UndefinedVariable#18158
pdpinch merged 1 commit into
openedx:masterfrom
mitodl:pdpinch/undefined-var-error

Conversation

@jolyonb

@jolyonb jolyonb commented May 8, 2018

Copy link
Copy Markdown
Contributor

This PR updates the error messages presented to students when they try to use an invalid variable or function in a formula.

Current behavior:

  • Using an invalid variable yields the message: Invalid input: x not permitted in answer. (using variable x)
  • Using an invalid function yields the message: Invalid input: f not permitted in answer. (using function f)

Note that there is no mention of how x or f are actually being used in the formula. This is particularly frustrating to students who write something like x(1+x), where x*(1+x) is required (and accounts for a significant percentage of forum posts in math-based courses). The error message simply tells students that x is not permitted, because x has been treated as an invalid function.

This PR addresses this issue by providing more intelligent error messages to students:

  • Using an invalid variable yields the message: Invalid Input: x not permitted in answer as a variable (using variable x)
  • Using a valid variable with the wrong case yields the message: "Invalid Input: X not permitted in answer as a variable (did you mean x?)" (using invalid variable X, valid variable x)
  • Using an invalid function yields the message: Invalid Input: f not permitted in answer as a function (using function f)
  • Using a valid function with the wrong case yields the message: Invalid Input: Sin not permitted in answer as a function (did you mean sin?) (using invalid function Sin, valid function sin)
  • Using a variable as a function, likely an indicator of a missing multiplication sign, yields the message: Invalid Input: x not permitted in answer as a function (did you forget to use * for multiplication?) (using variable x)

This is a WIP while we update old tests and construct new ones.

Mentions: @pdpinch @jefrench

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @jolyonb! I've created OSPR-2394 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

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

  • supporting documentation
  • edx-code email 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 still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 8, 2018
@mduboseedx

Copy link
Copy Markdown
Contributor

@jolyonb Feel free to ping me when this is ready to be reviewed

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

Copy link
Copy Markdown
Contributor

@jolyonb Are you still working on this PR? Is there anything we can do to help?

@jolyonb

jolyonb commented Jun 18, 2018

Copy link
Copy Markdown
Contributor Author

@mduboseedx I'm done with the coding part; all that's needed are tests I believe. @pdpinch was going to work on those...

@pdpinch

pdpinch commented Jul 1, 2018

Copy link
Copy Markdown
Contributor

jenkins run all

@pdpinch
pdpinch force-pushed the pdpinch/undefined-var-error branch 2 times, most recently from 4bba8cb to b41c986 Compare July 1, 2018 20:35
@pdpinch

pdpinch commented Jul 1, 2018

Copy link
Copy Markdown
Contributor

@mduboseedx this has (passing) tests now and is ready for review.

@mduboseedx

Copy link
Copy Markdown
Contributor

@edx/learner-product This adds a more descriptive and helpful error message for learners when inputting formulas. Could you take a look?

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jul 3, 2018
@pdpinch pdpinch changed the title More descriptive error messages for UndefinedVariable (WIP) More descriptive error messages for UndefinedVariable Jul 6, 2018
@jolyonb

jolyonb commented Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

Bump?

@mduboseedx

Copy link
Copy Markdown
Contributor

@edx/learner-product @marcotuts Have you had a chance to give this a look?

@marcotuts

Copy link
Copy Markdown
Contributor

This can move to awaiting prioritization on the engineering side, but this is actually something for the educator team to review, not learner as it turns out. :) @mduboseedx

@openedx-webhooks openedx-webhooks added awaiting prioritization and removed product review PR requires product review before merging labels Jul 13, 2018
@jolyonb

jolyonb commented Jul 30, 2018

Copy link
Copy Markdown
Contributor Author

Ok guys, bump? This one is pretty straightforward...

@mduboseedx

Copy link
Copy Markdown
Contributor

@edx/educator-all @scottrish Do you know when this could be given a review?

@jolyonb

jolyonb commented Aug 20, 2018

Copy link
Copy Markdown
Contributor Author

Bump...

@mduboseedx

Copy link
Copy Markdown
Contributor

@jolyonb I'm sorry for the long delay. Unfortunately our Educator team will actually be unable to review any PRs for the next few weeks. I'll see if I can find someone else to give this a look over.

@jolyonb

jolyonb commented Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

jenkins run bokchoy

1 similar comment
@jolyonb

jolyonb commented Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

jenkins run bokchoy

@jolyonb

jolyonb commented Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

I'm getting bokchoy errors:

PageLoadError: Timed out waiting to load page '<common.test.acceptance.pages.lms.course_home.CourseHomePage object at 0x7f0312e5d710>' at URL 'http://localhost:8003/courses/course-v1:test_org+15062424598459186396361364419204103494+test_run/course/'

Is this a known bad test? I've run the tests three times now and gotten the same error message three times. I don't touch anything related to this test with this PR...

@pdpinch

pdpinch commented Sep 1, 2018

Copy link
Copy Markdown
Contributor

jenkins run bokchoy

@mduboseedx

Copy link
Copy Markdown
Contributor

@jolyonb Sorry for the delay as I was out of the office last week. You may want to try a rebase and we can then run the bokchoy tests again.

@pdpinch
pdpinch force-pushed the pdpinch/undefined-var-error branch from 9afaf5b to 4f5ffd6 Compare September 10, 2018 17:30
@pdpinch

pdpinch commented Sep 10, 2018

Copy link
Copy Markdown
Contributor

Now I'm even more confused. The bokchoy tests ended in failure, yet there are no test failures? I guess we'll try again...

jenkins run bokchoy

@pdpinch
pdpinch force-pushed the pdpinch/undefined-var-error branch from 4f5ffd6 to 07eabea Compare September 16, 2018 21:00
@pdpinch

pdpinch commented Sep 16, 2018

Copy link
Copy Markdown
Contributor

jenkins run lettuce

@pdpinch
pdpinch force-pushed the pdpinch/undefined-var-error branch from 42616b9 to b43f224 Compare September 17, 2018 21:15
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@jolyonb

jolyonb commented Sep 17, 2018

Copy link
Copy Markdown
Contributor Author

@mduboseedx This is now tested and ready for review.

@mduboseedx

Copy link
Copy Markdown
Contributor

@pdpinch As this is an Educator type of pull request, if you feel comfortable with the code then you are welcome to merge this change.

@pdpinch
pdpinch merged commit 204a082 into openedx:master Sep 18, 2018
@openedx-webhooks

Copy link
Copy Markdown

@jolyonb 🎉 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

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Wednesday, September 19, 2018.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@pdpinch
pdpinch deleted the pdpinch/undefined-var-error branch September 20, 2018 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants