-
Notifications
You must be signed in to change notification settings - Fork 294
refactor: shift grade summary calculation to backend and display "hidden grades" label in the grade table #1797
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
refactor: shift grade summary calculation to backend and display "hidden grades" label in the grade table #1797
Conversation
|
Thanks for the pull request, @Anas12091101! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1797 +/- ##
==========================================
- Coverage 90.83% 90.82% -0.02%
==========================================
Files 344 344
Lines 5794 5785 -9
Branches 1374 1378 +4
==========================================
- Hits 5263 5254 -9
+ Misses 514 512 -2
- Partials 17 19 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
819f1af to
d99c306
Compare
|
@Anas12091101 Thank you for this contribution! Since it includes user-facing changes, you'll have to get approval on the product proposal before engineering review can start. I'll tag the team of product managers for you here. @openedx/openedx-product-managers Could you please have a look at this PR and the corresponding product proposal, and guide @Anas12091101 on next steps? This work might qualify as a fast-track change but I'm not completely sure. So if you could check what the right product review track would be as a first step, that would be great. -- CC @mphilbrick211 for openedx/edx-platform#37399 and openedx/frontend-app-authoring#2489 |
|
@itsjeyd the propsosal is linked in the PR description (https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5150769174/Proposal+Add+option+to+show+overall+assessment+results+after+due+date+but+hide+individual+assessment+results) and its been approved. |
|
Found it. @itsjeyd openedx/platform-roadmap#460 |
|
@Anas12091101 can you add some screenshots for the lower part of the progress page? We'd like to see how a hidden grade is handled there versus a non-hidden one. Along with that, it would be helpful to have more explanation of how the timing is handled differently between the old "never show assessment results" and this new "... but show overall assessment results after due date" |
b37a834 to
c183e89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good—just a minor enhancement and a few message updates.
| courseGradeFooterDueDateNotice: { | ||
| id: 'progress.courseGrade.footer.dueDateNotice', | ||
| defaultMessage: 'Some assignment scores are not yet included in your total grade. These grades will be released by {dueDate}.', | ||
| description: 'This shown when there are pending assignments with due date in the future', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| description: 'This shown when there are pending assignments with due date in the future', | |
| description: 'This is shown when there are pending assignments with a due date in the future', |
| hiddenScoreLockInfoText: { | ||
| id: 'progress.hiddenScoreLockInfoText', | ||
| defaultMessage: 'Shown when scores for an assignment type are hidden yet still counted toward the course grade.', | ||
| description: 'Information text about hidden score label when learner have limited access to grades feature', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| description: 'Information text about hidden score label when learner have limited access to grades feature', | |
| description: 'Information text about hidden score label when learners have limited access to grades feature', |
| }, | ||
| hiddenScoreLockInfoText: { | ||
| id: 'progress.hiddenScoreLockInfoText', | ||
| defaultMessage: 'Shown when scores for an assignment type are hidden yet still counted toward the course grade.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| defaultMessage: 'Shown when scores for an assignment type are hidden yet still counted toward the course grade.', | |
| defaultMessage: 'Scores for an assignment type are hidden but still counted toward the course grade.', |
| }, []); | ||
|
|
||
| // Returns True if this subsection is "hidden" | ||
| const isSubsectionHidden = (sub) => sub.showGrades && sub.showCorrectness === 'never_but_include_grade'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ShowGrades is false when due date is not passed. It results in 0% instead of a lock icon.
@pdpinch, In the In my test course, there are two sections, each with five subsections, configured with different visibility settings:
The learner will only see the visible subsection scores in the table, as shown in the screenshot below.
In the "Never show assessment results" option, the due date is not considered because those grades are excluded from the final results. In contrast, with the new option, the scores remain hidden from learners but are included in the final grades once the due date has passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code itself seems generally fine, but as noted in the inline comment, I'm trying to understand whether the premise is being addressed properly.
src/course-home/data/api.js
Outdated
| // Scores for visible grades (exclude never_but_include_grade) | ||
| const visibleScores = points.filter( | ||
| (_, idx) => visibilities[idx] !== 'never_but_include_grade', | ||
| ); | ||
|
|
||
| // Average all scores (for totalWeightedGrade) | ||
| const overallAverage = parseFloat( | ||
| (points.reduce((a, b) => a + b, 0) / points.length).toFixed(4), | ||
| ); | ||
| totalWeightedGrade = overallAverage * assignmentWeight; | ||
| if (visibleScores.length) { | ||
| const visibleAverage = parseFloat( | ||
| (visibleScores.reduce((a, b) => a + b, 0) / points.length).toFixed(4), | ||
| ); | ||
| averageGrade = visibleAverage; | ||
| weightedGrade = averageGrade * assignmentWeight; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First: I'm always leery of manipulating critical data (such as a learner's grade) in the frontend. This here seems like a pretty heavy lift. And while I'm not an expert on how grading works in the backend, I expect this is duplicating backend logic. Yes, I realize that this kind of calculation is already being done in this very file - which is unfortunate - but this is not all.
If I understand correctly from the proposal, the point of hiding scores is to prevent answer sharing. However - correct me if I'm wrong - I deduce from this that the individual scores are actually being sent to the client, so that a crafty user could in fact find out the correspondence between all answers and scores.
If the whole premise is to prevent cheating, shouldn't we avoid passing all the individual grades to the frontend in the first place?
CC @pdpinch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, you are correct @arbrandes. We should be hiding these grades in the API.
2acc837 to
db9a11e
Compare
|
@arbrandes, I have shifted the grades calculation logic to the backend in openedx/edx-platform#37399 and adjusted this PR accordingly. This PR is ready for another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in testing!
…ade (#37399) This commit migrates the data calculation logic for the GradeSummary table, which was previously in the frontend-app-learning. This commit also introduces a new visibility option for assignment scores: “Never show individual assessment results, but show overall assessment results after the due date.” With this option, learners cannot see question-level correctness or scores at any time. However, once the due date has passed, they can view their overall score in the total grades section on the Progress page. These two changes are coupled with each other because it compromises the integrity of this data to do the score hiding logic on the front end. The corresponding frontend PR is: openedx/frontend-app-learning#1797
|
@arbrandes, the grades calculation PR in edx-platform (openedx/edx-platform#37399) has been merged. This one is ready for review as well. |
|
I'm trying to test this but am seeing some weird behavior with scores and visibility (A "show after due" problem showing the answer before the due date, a grade not being calculated correctly, etc). Before I confirm this is a bug I'm going to reset my environment tomorrow and test from scratch. |
|
Ok, I ran into two issues, all of which you can see in the recording of today's Frontend Working Group meeting starting at 23:17.
This is all on Tutor main with edx-platform@master updated to the latest code as of this morning, and a checkout of this PR and the corresponding one on frontend-app-authoring. I created course.skzk4g2n.tar.gz from scratch. And this is a screenshot of my progress after getting all the answers right and setting the corresponding due dates in the past:
|
|
@arbrandes, regarding the first issue: you are currently viewing the problem in staff mode, which is why the grade appears despite the due date. The relevant code responsible for displaying the correctness of a problem can be found here: graders.py Line 491. For the second issue, I believe that edX is not updating the due date in the LMS after changes are made in the CMS. I replicated this behavior on my local setup as well. After multiple updates to the due date, it eventually reflected correctly, and the progress page was updated accordingly. Could you please verify if this is the same issue you're encountering? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, even though there might be an issue with resetting the due date of assignments after publishing, that would be a separate issue. This PR is good. Let's get it in!
|
Thanks @arbrandes. Could you please also merge the authoring MFE PR? openedx/frontend-app-authoring#2489 |


Supporting tickets
Proposal: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5150769174/Proposal+Add+option+to+show+overall+assessment+results+after+due+date+but+hide+individual+assessment+results
openedx/platform-roadmap#460
Internal ticket: https://github.com/mitodl/hq/issues/3859
Description
This PR refactors the grade summary logic to delegate all calculation responsibilities to the backend.
Previously, the frontend was performing grade summary computations using data fetched from the API. Now, the API itself provides the fully computed grade summary, simplifying the frontend and ensuring consistent results across clients.
Additionally, a "Hidden Grades" label has been added in the grade summary table to clearly indicate sections where grades are not visible to learners.
Additionally, for visibility settings that depend on the due date, this PR adds a banner on the Progress page indicating that grades are not yet released, along with the relevant due date information.
edx-platfom and authoring MFE changes are required to test this PR.
edx-platform PR: openedx/edx-platform#37399
authoring MFE PR: openedx/frontend-app-authoring#2489
Screenshots
Testing Instructions
Verify that changes don't break the logic of the progress page.
Use this course tarball for testing: course._4j9qu3f.tar.gz
Verify that with the new visibility option:
Confirm that subsections with the new visibility option are not listed in the Detailed Grades table.
In the Grade Summary table, check two cases: