fix: remove waffle flag to show Certificates menu#3066
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
It's not a big deal, but this isn't working for me. It just shows a blank page (header and footer but nothing else). Otherwise, this PR is ready to go. I'm also fine merging it as is, because the link is being hidden correctly. |
…Page flag references These flags are being removed in openedx#3066 and should be treated as always true. Remove test scenarios for the disabled state and clean up mock overrides that set them to false.
…Page flag references These flags are being removed in openedx#3066 and should be treated as always true. Remove test scenarios for the disabled state and clean up mock overrides that set them to false.
…Page flag references These flags are being removed in openedx#3066 and should be treated as always true. Remove test scenarios for the disabled state and clean up mock overrides that set them to false.
|
Yep, my bad. I have removed that from the testing instructions. The behavior you're seeing is expected with the current implementation. Other than that, the PR should be ready to merge. |
|
Thanks! |
|
@Anas12091101 There is a test failure on |
|
Looking |
…Page flag references These flags are being removed in openedx#3066 and should be treated as always true. Remove test scenarios for the disabled state and clean up mock overrides that set them to false.
…Page flag references These flags are being removed in openedx#3066 and should be treated as always true. Remove test scenarios for the disabled state and clean up mock overrides that set them to false.
* feat: adding permission validations from authz for files page for view, create, edit and delete * test: adding tests to increase code coverage * refactor(authz): remove duplicate hook and gate lock/unlock behind canEditFiles - Remove useUserPermissionsWithAuthzCourse in favor of useCourseUserPermissions which provides the same functionality with better generic typing - Migrate all consumers (CourseFilesTable, FilesPage, header hooks) to use useCourseUserPermissions with flat destructuring - Hide Lock/Unlock option in FileMenu, MoreInfoColumn, and FileInfoModalSidebar when canEditFiles is false (course_auditor should not see lock/unlock) - Add unit tests for lock/unlock visibility based on permissions - Fix clipboard mock in tests using Object.defineProperty - Update FilesPage.test.jsx mocks to match new flat return shape * test: remove deprecated useNewCertificatesPage and useNewVideoUploadsPage flag references These flags are being removed in #3066 and should be treated as always true. Remove test scenarios for the disabled state and clean up mock overrides that set them to false.
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 removes the waffle flag requirement for the Certificates link to appear, restoring operator control over menu visibility. Additionally, the default value of
ENABLE_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.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.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'