Skip to content
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

add validation to capa problem authoring markdown #265

Conversation

HamzaIbnFarooq
Copy link

@HamzaIbnFarooq HamzaIbnFarooq commented May 31, 2021

Related Issue

#239
#240

What this PR does

It adds validation to CAPA problems and shows a notification to the course author regarding the issue, along with fixing old response report generation issues by providing a default value for the problems having no text values for answer options.

How to test this PR

To understand the purpose of this PR, follow the below steps to reproduce the issue:

  1. Create a Checkbox Problem on studio
  2. Don't put any text against one of the options
    image
  3. Through LMS select that option with no text
  4. Go to Data Download under Instructor tab
  5. Under the Reports section, select the Problem and press Create a report of problem responses
    The error will be generated.

Now use the current PR and go through the same process, and you will see that the editor will not allow saving such content and mentions an error message instead.

Screenshots

image

@HamzaIbnFarooq HamzaIbnFarooq changed the title Hamza/add validation to capa problem authoring markdown add validation to capa problem authoring markdown May 31, 2021
@HamzaIbnFarooq HamzaIbnFarooq requested a review from ziafazal May 31, 2021 11:32
Copy link

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

@HamzaIbnFarooq overall LGTM. few typos needs to be fixed and I have question.

@@ -402,6 +402,9 @@
line = lines[i].trim();
if (line.length > 0) {
textHint = extractHint(line, true);
if (!textHint.nothint) {
throw new Error("One of the provided options don't have valid text value")

Choose a reason for hiding this comment

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

replace don't have valid with doesn't have a valid. Are these error messages are marked for internationalization?

Copy link
Author

@HamzaIbnFarooq HamzaIbnFarooq Jun 1, 2021

Choose a reason for hiding this comment

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

I have fixed the typo and the messages are already internationalized in the showErrorMeassage method but I have added translations to individual strings as well.

@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/add_validation_to_capa_problem_authoring_markdown branch from 3dec514 to 88d0d01 Compare June 1, 2021 10:52
@HamzaIbnFarooq HamzaIbnFarooq requested a review from ziafazal June 1, 2021 10:53
Copy link

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

👍

@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/add_validation_to_capa_problem_authoring_markdown branch 2 times, most recently from 2975572 to 8621270 Compare June 4, 2021 10:44
@HamzaIbnFarooq HamzaIbnFarooq requested a review from ziafazal June 7, 2021 06:30
@HamzaIbnFarooq
Copy link
Author

@ziafazal I merged #264 into the current PR and added some tests as well. Kindly have a final look before I create an upstream PR.

Copy link

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

👍

@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/add_validation_to_capa_problem_authoring_markdown branch from 745c00d to 9dc9a34 Compare June 7, 2021 07:49
@HamzaIbnFarooq
Copy link
Author

@pdpinch I have created an upstream PR related to this feature: https://github.com/edx/edx-platform/pull/27858

@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/add_validation_to_capa_problem_authoring_markdown branch 3 times, most recently from 389595b to d85f98a Compare June 9, 2021 05:09
HamzaIbnFarooq and others added 3 commits June 17, 2021 15:59
Before this commit, if a course author created a capa mcqs or similar problem without providing any answer text for an option the question would be created causing abnormal behavior for learners. This commit will validate answer text of all options and raise an error message to author to fix the issue on the go.
…fix response report generation

If an author has created a capa problem like an mcqs or something similar without providing answer text to an option and some learner selected that option then the response report generation will fail due to that missing answer text. The current commit will add default text to be substituted and prevents report generation crash.
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/add_validation_to_capa_problem_authoring_markdown branch from d85f98a to 9e655a4 Compare June 17, 2021 11:19
@HamzaIbnFarooq HamzaIbnFarooq changed the base branch from hamza/edx_latest_master to mitx/lilac July 13, 2021 11:10
@HamzaIbnFarooq HamzaIbnFarooq changed the base branch from mitx/lilac to hamza/edx_latest_master July 13, 2021 11:10
@HamzaIbnFarooq
Copy link
Author

The upstream PR https://github.com/edx/edx-platform/pull/27858 has been merged so closing this internal PR as well

@HamzaIbnFarooq HamzaIbnFarooq deleted the hamza/add_validation_to_capa_problem_authoring_markdown branch July 13, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants