-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: add never_but_include_grade visibility option #37399
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
base: master
Are you sure you want to change the base?
feat: add never_but_include_grade visibility option #37399
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. |
cea9397
to
c73c019
Compare
@Anas12091101 could you add some tests around this new functionality? |
e1d16b4
to
602d07e
Compare
@sarina I have added the tests |
Thanks @Anas12091101 - I just checked with @pdpinch and he's going to provide a review. |
afee826
to
9a055ec
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.
LGTM!
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.
Minor question and request. But also a high level question: Don't we need to modify the REST API to suppress the grade when the new status is applied to a subsection?
from datetime import datetime | ||
from logging import getLogger | ||
|
||
from pytz import UTC |
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.
Please use the standard library's datetime.timezone.utc
instead of using the pytz package.
%if hide_url: | ||
<p class="d-inline">${section.display_name} | ||
%if (total > 0 or earned > 0) and section.show_grades(staff_access): | ||
%if (total > 0 or earned > 0) and section.show_grades(staff_access) and not section.show_correctness == '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.
In general, we want to keep logic this specific out of the template. Is there a reason it can't be incorporated into the show_grades()
logic?
@Anas12091101: Did you use an LLM to help generate this PR? And if so, which one? If you have used an LLM, please make sure you have read the project policy on using generative AI tools. |
@ormsbee Thanks for the pointer. I did not use an LLM to create this PR. I have, however, used GitHub Copilot (GPT-5 agent) on a different Authoring MFE PR to help write tests and speed up some fixes |
Yes, we need to suppress the grade in the API. This is also being discussed here: openedx/frontend-app-learning#1797 (comment). |
Description
This PR 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.
This PR should be reviewed in correspondence with openedx/frontend-app-learning#1797
Useful information to include:
"Developer", and "Operator".
changes.
Supporting information
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
Testing instructions
Included in openedx/frontend-app-learning#1797
Deadline
Before Ulmo
Other information
Include anything else that will help reviewers and consumers understand the change.