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

fix: switch to advanced editor for partial credit support #386

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

navinkarkera
Copy link
Contributor

This commit reverts to advanced editor when partial_credit attribute is added to multichoice, single select and numerical problems. Without this change, the partial_credit attribute is removed from the problem on the next edit.

Test instructions:

  • Setup master devstack and follow steps provided in the README to use the new problem editor.
  • Don't forget to enable new_core_editors.use_new_text_editor waffle flag.
  • Create a blank advanced problem and paste below olx to create a multiselect with partial credit:
<problem>
  <choiceresponse partial_credit="EDC">
    <label>Which of the following is a fruit?</label>
    <description>Select all that apply.</description>
    <checkboxgroup>
      <choice correct="true">apple</choice>
      <choice correct="true">pumpkin</choice>
      <choice correct="false">potato</choice>
      <choice correct="true">tomato</choice>
    </checkboxgroup>
  </choiceresponse>
</problem>
  • Save and test the problem in CMS, it should work as expected.
  • Without this commit/change, if you click on edit, it will load default GUI editor. This removes partial_credit attribute from the component.
  • Checkout this branch and run make build.
  • Refresh CMS and edit the component. Change to advanced editor and add partial_credit="EDC" back to the problem and save.
  • Now if you edit this component, it will automatically switch to advanced editor preserving the attribute.
  • You can also test other problems like single select and numerical problems as mentioned in the documentation here.

This commit reverts to advanced editor when partial_credit attribute is
added to multichoice, single select and numerical problems. Without this
change, the partial_credit attribute is removed from the problem on the
next edit.
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 11, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @navinkarkera! 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 as you can:

  • 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.

@navinkarkera navinkarkera self-assigned this Sep 11, 2023
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (9ebe187) 90.58% compared to head (e3f5bbf) 90.59%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
+ Coverage   90.58%   90.59%   +0.01%     
==========================================
  Files         230      230              
  Lines        4109     4116       +7     
  Branches      824      826       +2     
==========================================
+ Hits         3722     3729       +7     
  Misses        366      366              
  Partials       21       21              
Files Changed Coverage Δ
...editors/containers/ProblemEditor/data/OLXParser.js 95.45% <100.00%> (+0.06%) ⬆️
...tainers/ProblemEditor/data/mockData/olxTestData.js 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@tecoholic tecoholic left a comment

Choose a reason for hiding this comment

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

@navinkarkera 👍 LGTM. In the testing instructions, did you mean to mention new_core_editors.use_new_problem_editor waffle flag instead of new_core_editors.use_new_text_editor ? Because, I kept getting confused why changing that flag isn't doing anything and I kept loading the CMS editor for the Advanced problem.

  • I tested this: Tested that with this change the advanced editor is always launched when partial credits are in the problems.
  • I read through the code
  • I checked for accessibility issues - NA
  • Includes documentation

@navinkarkera
Copy link
Contributor Author

@navinkarkera 👍 LGTM. In the testing instructions, did you mean to mention new_core_editors.use_new_problem_editor waffle flag instead of new_core_editors.use_new_text_editor ? Because, I kept getting confused why changing that flag isn't doing anything and I kept loading the CMS editor for the Advanced problem.

@tecoholic Sorry, I meant new_core_editors.use_new_text_editor flag in addition to new_core_editors.use_new_problem_editor as I had to enable both flags to get it working.

@tecoholic
Copy link

@navinkarkera Ah. That makes sense. I added all the 3 flags mentioned in the docs for good measure after the use_new_text_editor alone didn't work. :)

Copy link
Contributor

@kenclary kenclary left a comment

Choose a reason for hiding this comment

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

ok

@kenclary kenclary merged commit eb320ab into openedx:main Sep 20, 2023
@openedx-webhooks
Copy link

@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants