-
Notifications
You must be signed in to change notification settings - Fork 160
feat: add never_but_include_grade visibility option #2489
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 #2489
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 #2489 +/- ##
=======================================
Coverage 94.68% 94.68%
=======================================
Files 1203 1203
Lines 26865 26865
Branches 6026 6027 +1
=======================================
Hits 25438 25438
- Misses 1357 1368 +11
+ Partials 70 59 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@Anas12091101 can you check off the boxes in the PR description? |
In the description, you wrote, “Never show individual assessment results, but show overall assessment results after the due date” but the UI doesn't mention the due date. Is the description wrong, or do we need to change the text of the UI to mention the due date? |
Thanks for catching this. I had intended to copy the description from the proposal but must have mistakenly used an AI-generated version instead. I’ve now updated the PR with the correct title and description for the new option, as well as an updated screenshot. |
ce42fc6
to
8adc1b7
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!
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".
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
Internal ticket: https://github.com/mitodl/hq/issues/3859
Testing instructions
Included in openedx/frontend-app-learning#1797
Other information
Include anything else that will help reviewers and consumers understand the change.
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts
,.tsx
).propTypes
,defaultProps
, andinjectIntl
patterns are not used in any new or modified code.src/testUtils.tsx
(specificallyinitializeMocks
)apiHooks.ts
in this repo for examples.messages.ts
files have adescription
for translators to use.../
. To import from parent folders, use@src
, e.g.import { initializeMocks } from '@src/testUtils';
instead offrom '../../../../testUtils'