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

feat: add grading method field in problem editor #430

Closed

Conversation

BryanttV
Copy link

@BryanttV BryanttV commented Dec 11, 2023

Description

This PR adds a new field in the Problem Editor for choosing a Grading Method. Currently, the only Grading Method is the Last Score. From now on, the new grading methods are:

  1. Last Score (Default): The last score made is taken for grading.
  2. First Score: The first score made is taken for grading.
  3. Highest Score: The highest score made is taken for grading.
  4. Average Score: The average of all scores made is taken for grading.

Note

This field will only appear if the following feature flag is activated: ENABLE_GRADING_METHOD_IN_PROBLEMS

Supporting information

These changes are part of the effort made to implement Configurable grading method for problems with multiple attempts

Demo

grading-method-demo.mp4

Dependencies

Important

This PR needs the changes made from these PRs:

These are the last PRs needed to add support to the MFE for the Configurable grading method for problems with multiple attempts.

Testing Instructions

Using tutor you can test these changes:

  1. In your tutor environment clone the Course Authoring MFE with these changes.

  2. Enable grading method in the Course Authoring MFE, you can use this tutor plugin:

    name: mfe_course_authoring_config
    version: 0.1.0
    patches:
      mfe-lms-development-settings: |
        MFE_CONFIG["ENABLE_GRADING_METHOD_IN_PROBLEMS"] = True
    
      mfe-lms-production-settings: |
        MFE_CONFIG["ENABLE_GRADING_METHOD_IN_PROBLEMS"] = True
  3. Create a mount with tutor mounts add frontend-app-course-authoring

  4. Run tutor config save and tutor dev start -d

  5. Inside the frontend-app-course-authoring folder clone this repository and checkout to this branch.

  6. Inside the frontend-app-course-authoring folder create a new file called module.config.js with this content:

    module.exports = {
      localModules: [
        {
          moduleName: "@edx/frontend-lib-content-components/dist",
          dir: "./frontend-lib-content-components",
          dist: "src",
        },
        {
          moduleName: "@edx/frontend-lib-content-components",
          dir: "./frontend-lib-content-components",
          dist: "src",
        },
      ],
    };
  7. Then run npm install (First in frontend-lib-content-components, then in frontend-app-course-authoring)

  8. Active in the Django admin (admin/waffle/flag/) this waffle flags: new_core_editors.use_new_problem_editor and new_core_editors.use_new_text_editor like this:

    image

  9. Go to Studio and create a new component with a problem, for example: Single select

  10. In the component settings you should see the new field.

@openedx-webhooks
Copy link

openedx-webhooks commented Dec 11, 2023

Thanks for the pull request, @BryanttV!

What's next?

Please work through the following steps to get your changes 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.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

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:

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 11, 2023
@BryanttV BryanttV force-pushed the bav/add-grading-strategy-field branch 2 times, most recently from ddb78dc to e5e4016 Compare December 11, 2023 20:29
@BryanttV BryanttV force-pushed the bav/add-grading-strategy-field branch from d035480 to 5045fb5 Compare December 12, 2023 21:34
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 89.16%. Comparing base (905bea0) to head (8b64635).
Report is 6 commits behind head on main.

❗ Current head 8b64635 differs from pull request most recent head fcd649f. Consider uploading reports for the commit fcd649f to get more accurate results

Files Patch % Lines
.../SettingsWidget/settingsComponents/ScoringCard.jsx 50.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #430      +/-   ##
==========================================
- Coverage   89.25%   89.16%   -0.10%     
==========================================
  Files         246      246              
  Lines        4475     4493      +18     
  Branches      917      923       +6     
==========================================
+ Hits         3994     4006      +12     
- Misses        454      460       +6     
  Partials       27       27              

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

@BryanttV BryanttV force-pushed the bav/add-grading-strategy-field branch from 5045fb5 to f61d254 Compare February 19, 2024 16:07
@BryanttV BryanttV changed the title feat: add grading strategy field in problem editor feat: add grading method field in problem editor Feb 19, 2024
@BryanttV BryanttV marked this pull request as ready for review February 19, 2024 21:44
@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Feb 21, 2024
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

I tested this alongside openedx/edx-platform#33911, and it worked as expected. Once the edx-platform PR is merged and this review is done, we can merge it!

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

After testing it out, I left an approval yesterday for this PR, but now the backend will suffer some changes. Please, review this thread for more context: https://openedx.slack.com/archives/C02QN50TYMD/p1710176417562869

So we'll have to add that feature flag here too. I'll remove my approval for the time being.

@BryanttV BryanttV force-pushed the bav/add-grading-strategy-field branch from 6c3fb22 to 7af8d8c Compare April 1, 2024 23:36
@BryanttV BryanttV force-pushed the bav/add-grading-strategy-field branch 4 times, most recently from 705d22c to 2716b5f Compare April 10, 2024 13:03
@BryanttV BryanttV force-pushed the bav/add-grading-strategy-field branch from 2716b5f to fcd649f Compare April 10, 2024 18:16
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all my comments. LGTM!

@BryanttV
Copy link
Author

Hi @openedx/2u-tnl, Could you take a look at this PR? Thanks!
@brian-smith-tcril, @arbrandes, I don't know if you can also help me with a review, I would appreciate it very much!

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

Overall this looks good from a code perspective!

  • Uses getConfig ✔️
  • Uses Paragon components ✔️

I left a couple nits about relative import paths, and I haven't tested this out locally, maybe it makes sense to get a sandbox up for it?

import { Form, Hyperlink } from '@openedx/paragon';
import { selectors } from '../../../../../../data/redux';
import SettingsOption from '../SettingsOption';
import messages from '../messages';
import { scoringCardHooks } from '../hooks';
import { GradingMethod, GradingMethodKeys } from '../../../../../../data/constants/problem';
Copy link
Contributor

Choose a reason for hiding this comment

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

This could become problematic if we ever want to move this file. Does

Suggested change
import { GradingMethod, GradingMethodKeys } from '../../../../../../data/constants/problem';
import { GradingMethod, GradingMethodKeys } from 'data/constants/problem';

not work?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for your review! I was trying your suggestions and unfortunately, it doesn't work. I get that it doesn't find the module :(. It must be an absolute path, and that's how it's currently done in all the code.

import { formatMessage } from '../../../../../../../testUtils';
import { scoringCardHooks } from '../hooks';
import { ScoringCard } from './ScoringCard';
import { GradingMethodKeys } from '../../../../../../data/constants/problem';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about relative import paths as before (I see that formatMessage is also doing this here, could be nice to clean that up too)

@BryanttV
Copy link
Author

@brian-smith-tcril, It would be great if you could try these changes. Note that to show or hide the grading method field, changes in this PR are required:

@BryanttV
Copy link
Author

Hi @brian-smith-tcril, just a little reminder. The course authoring PR has been approved.

@brian-smith-tcril
Copy link
Contributor

brian-smith-tcril commented Apr 25, 2024

Thanks for the reminder! I plan on doing a bit of local testing of this PR this afternoon and trying to get a PR sandbox deployed for it tomorrow morning.

edit: I didn't get a chance to set up local testing for this yesterday. I plan on getting the sandbox up at the beginning of next week.

@brian-smith-tcril
Copy link
Contributor

I got a sandbox deployed for this here: openedx/frontend-app-authoring#969 (comment)

@jmakowski1123 if you or anyone else who is familiar with the intended behavior of this could test it out in the sandbox that would be wonderful!

@BryanttV
Copy link
Author

Hi @brian-smith-tcril, thanks for generating the sandbox!

If it might help, I copy a comment with the different test cases that was included in the backend PR.


Note for the reviewer: Below is a list of all the test cases, and I have attached a course that can be used for this purpose.

Test Course: grading-method.tar.gz

Test Cases

  1. As an Instructor, the old problems I have created now have Last Score assigned as the grading method. As a Student, my score, and therefore my progress, remains the same.
  2. As an Instructor, I create a new problem, and the default grading method is Last Score.
  3. As an Instructor, I select Last Score as the grading method. As a Student, the last submission I make will be taken as my score (also from the progress section).
  4. As an Instructor, I select Highest Score as the grading method. As a Student, the submission with the highest score I have made will be taken as my score (also from the progress section).
  5. As an Instructor, I select First Score as the grading method. As a Student, the first submission I have made will be taken as my score (also from the progress section).
  6. As an Instructor, I select Average Score as the grading method. As a Student, my score will be the average of all submissions I have made (also from the progress section).
  7. As an Instructor, after defining a problem with a maximum number of attempts, I can reset the students' attempts. As a Student, I see that the attempts are reset to zero, and I can make attempts until reaching the maximum number again.

Rescoring

  1. As an Instructor, I can switch between grading methods for a problem and perform the corresponding rescore for the student(s). As a Student, I can see my new grade according to the grading method defined in the problem (also from the progress section).
  2. As an Instructor, I can change the correct answers for a problem and perform the corresponding rescore for the student(s). As a Student, I can see my new grade according to the new correct answers defined in the problem (also from the progress section).
  3. As an Instructor, I can switch between grading methods for a problem and perform the corresponding rescore for the student(s) ONLY if it improves their grade. I can do this in three different ways. As a Student, I can see my new grade according to the grading method defined in the problem (also from the progress section).

Note

Keep in mind that in cases of rescoring, the instructor can perform it in three different ways:

  • From the Staff Debug in the unit by entering the student's username.
  • In the instructor's Dashboard under Student Admin enter the student's username and the location of the problem in the course.
  • In the instructor's Dashboard under Student Admin enter the location of the problem in the course (for all students).

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented May 6, 2024

Hi @brian-smith-tcril! Here are the instructions I've used for testing:

  1. (Operations) Add in your environment these settings to turn on the feature:
FEATURES["ENABLE_GRADING_METHOD_IN_PROBLEMS"] = True
MFE_CONFIG["ENABLE_GRADING_METHOD_IN_PROBLEMS"] = True
  1. Go to Studio and create a problem component from a Unit. e.g. Advanced > Blank Problem.

  2. In component settings, you should see a new field: Grading Method. This field is a dropdown list with the Grading Methods mentioned above.

  3. Choose a Grading Method and save changes. Here's a description of each grading method:

    Last Score (Default): The last score made is taken for grading.
    First Score: The first score made is taken for grading.
    Highest Score: The highest score made is taken for grading.
    Average Score: The average of all scores made is taken for grading.

  4. From the LMS, answer the problem and check with the different Grading Methods. You can also check from the progress section.

Those were taken from the backend PR which I reviewed. About testing from a product POV, we can also help with testing following the approved product proposal. Please, let us know.

We only have a few project hours left (~2 weeks), but we'll do our best to move this forward. Thank you!

@mphilbrick211
Copy link

Hi @brian-smith-tcril! Is this something you're still able to look at?

export const GradingMethod = StrictDict({
[GradingMethodKeys.LAST_SCORE]: {
id: 'authoring.problemeditor.settings.gradingmethod.last_score',
defaultMessage: 'Last Score',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'Last Score',
defaultMessage: 'Last score (Default)',

},
[GradingMethodKeys.HIGHEST_SCORE]: {
id: 'authoring.problemeditor.settings.gradingmethod.highest_score',
defaultMessage: 'Highest Score',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'Highest Score',
defaultMessage: 'Highest score',

},
[GradingMethodKeys.AVERAGE_SCORE]: {
id: 'authoring.problemeditor.settings.gradingmethod.average_score',
defaultMessage: 'Average Score',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'Average Score',
defaultMessage: 'Average score',

},
[GradingMethodKeys.FIRST_SCORE]: {
id: 'authoring.problemeditor.settings.gradingmethod.first_score',
defaultMessage: 'First Score',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'First Score',
defaultMessage: 'First score',

Comment on lines +64 to +65
{isGradingMethodEnabled && <FormattedMessage {...messages.scoringSettingsLabelWithGradingMethod} />}
{!isGradingMethodEnabled && <FormattedMessage {...messages.scoringSettingsLabel} />}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{isGradingMethodEnabled && <FormattedMessage {...messages.scoringSettingsLabelWithGradingMethod} />}
{!isGradingMethodEnabled && <FormattedMessage {...messages.scoringSettingsLabel} />}
{isGradingMethodEnabled ?
(<FormattedMessage {...messages.scoringSettingsLabelWithGradingMethod} />
) : (
<FormattedMessage {...messages.scoringSettingsLabel} />
)}

@KristinAoki
Copy link
Member

Hi! Thank you for your contribution to this repo. This repo is being deprecated and all the code is being moved to frontend-app-course-authoring. If you would like your work to be included in the transition please resolve outstanding requests for this PR. This PR will automatically close once frontend-app-course-authoring PR #1208 is merged.

@KristinAoki KristinAoki added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Aug 28, 2024
@bradenmacdonald
Copy link
Contributor

As mentioned above, we've now moved this code into https://github.com/openedx/frontend-app-course-authoring/ so we'll need you to please re-open this PR against frontend-app-course-authoring if you'd like to continue it. Feel free to reach out to me if you aren't sure how to do that using git and I can help (basically: add your local content-components repo as a remote in course-authoring, then pull the branch and rebase it).

@openedx-webhooks
Copy link

@BryanttV Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

1 similar comment
@openedx-webhooks
Copy link

@BryanttV Even though your pull request wasn’t 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 waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants