fix: require both ENABLE_CERTIFICATE_PAGE config and waffle flag to show Certificates menu#3066
fix: require both ENABLE_CERTIFICATE_PAGE config and waffle flag to show Certificates menu#3066Anas12091101 wants to merge 4 commits into
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. DetailsWhere 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 #3066 +/- ##
=======================================
Coverage 95.55% 95.55%
=======================================
Files 1393 1393
Lines 32999 32994 -5
Branches 7645 7641 -4
=======================================
- Hits 31532 31529 -3
- Misses 1401 1412 +11
+ Partials 66 53 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It sounds like the
See also #3063 which has a plan for removing most of the waffle flags, for the same reason. |
0e1e476 to
e5ef2d3
Compare
Description
The
Certificateslink in the Settings menu was always visible regardless of theENABLE_CERTIFICATE_PAGEconfiguration because the conditional used anOR(||) operator to check either the config value or theuseNewCertificatesPagewaffle flag (mapped tolegacy_studio.certificatesin the platform). A recent upstream change inopenedx-platformmadelegacy_studio.certificatesalways returnTrue, which meant theORcondition was always satisfied and operators could no longer hide the Certificates menu by settingENABLE_CERTIFICATE_PAGE=false. This PR changes the||to&&so that bothENABLE_CERTIFICATE_PAGEand the waffle flag must betruefor the Certificates link to appear, restoring operator control over menu visibility. Additionally, the default value ofENABLE_CERTIFICATE_PAGEhas been changed from'false'to'true'so that existing deployments that rely on the waffle flag continue to see the menu without needing to add a new config variable. Tests have been updated to cover the newANDlogic, including a case where the waffle flag is disabled but the config is enabled.Useful information to include:
Supporting information
Link to other information about the change, such as GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Manual Testing Steps
Test: Certificates visible by default
ENABLE_CERTIFICATE_PAGE=true(or remove the variable entirely — it now defaults totrue).http://apps.local.openedx.io:2001/course/course-v1:edX+DemoX+Demo_Course).Test: Certificates hidden when config is
falseENABLE_CERTIFICATE_PAGE=falsein .env.development.Test: Certificates hidden when config is removed and default was relied upon
ENABLE_CERTIFICATE_PAGEline entirely from .env.development.'true', so certificates should appear.Test: Direct URL access when disabled
ENABLE_CERTIFICATE_PAGE=falsein .env.development and restart.http://apps.local.openedx.io:2001/course/course-v1:edX+DemoX+Demo_Course/certificates.Run automated tests
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).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'