fix: hide course authoring content when enable_course_authoring is off#176
fix: hide course authoring content when enable_course_authoring is off#176bra-i-am wants to merge 8 commits into
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @bra-i-am! 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. |
9c86539 to
84b092e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #176 +/- ##
==========================================
+ Coverage 97.48% 97.64% +0.15%
==========================================
Files 66 68 +2
Lines 1552 1655 +103
Branches 400 436 +36
==========================================
+ Hits 1513 1616 +103
Misses 39 39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e268688 to
9e97e1e
Compare
93a14f8 to
b049a37
Compare
b88a859 to
92351b0
Compare
|
@bra-i-am does this change depend on new changes in openedx-authz? I tried to run it on my env which was built from main a couple of days ago and I see the following errors on the browser inspector when loading the admin-console form this branch: Request: POST http://local.openedx.io:8000/api/authz/v1/permissions/validate/me Payload: [{"action":"content_libraries.manage_library_team"},{"action":"courses.manage_course_team"}] Response: [{"scope":["This field is required."]},{"scope":["This field is required."]}] |
@rodmgwgu yes, it depends on release 1.20.0 https://github.com/openedx/openedx-authz/releases/tag/v1.20.0 |
|
@MaferMazu @BryanttV I have a question about the expected behavior. Specifically, is it okay to run the migrations when the flag is turned off? I just want to make sure I understand all the requirements before reviewing. I was able to see the assignment table filtered, (although not the filter options when using the admin role) by enabling 
However, I don't see the same behavior at the platform level. Am I missing a step, or is there something else that needs to be configured? |
|
@dcoa, in my understanding, this solution works because there is no assignment of new authorization course authoring roles (thanks to the migration), so you won't be able to see the information. Is that so, @bra-i-am? If so, I am a bit worried about it, because based on the comments in the issue, the criteria are not related to having a role; they are more related to the waffle flag.
That's a quote from openedx/openedx-authz#340 (comment) It makes me think because you can definitely have the migration enabled, but if not, based on Guille's comment, you shouldn't be able to see anything related to new course authoring roles if the waffle flag is off. For example, having the waffle flag active, assigning user Y as the course admin for course X, and then deactivating the waffle flag. - That user Y shouldn't be able to assign more users to the new authorization course authoring roles. <- This should be true regardless of whether migration conf is enabled. I tested exactly that: I enabled the waffle flag globally, assigned another user as the course admin for a course, and then deactivated the waffle flag... and that "another user" is still able to assign course authoring roles. <- and this shouldn't be possible. I remember a conversation where the proposal to solve this was to create a REST API in AuthZ that retrieves information about available domains (also taking into account the waffle flag). Also, we considered solving #172 with that approach, but since it was resolved directly with front, I wonder if we can do something similar here, or if we would need the rest api to have the proper info from the back because the waffle flag info is trickier. |
|
thanks @MaferMazu for the extra context Mi opinion here:
The implementation in this PR is not bad understanding the automated migration run, and how it impact the backend. However is not enough covering all the test cases, based on what we are discussing.
The other issue was a smaller scope that is why we can solved it, even though we still needed backend implementation. The issue here is more complicated and I think need a solution at the backend side first, the reason is there is no platform standalone endpoint that we can use from the frontend to validate The platform has
Scopes should be filtered properly, with that we can validate al front and there is not Or even the |
|
@dcoa I agree. |
|
This PR was meant to partially solve 340 & 341 by supporting the proposal made here #176 (comment) for the Verawood release - returning False for scopes based on the waffle flag state. But the approach still needs to address other dimensions, such as the performance and correctness of the entire framework, especially since these issues are release blockers and the modified endpoints are critical to the system's functionality. Internally (thanks @dcoa!), we discussed the best solution moving forward, given that we shouldn't introduce big changes that put the framework's correctness at risk, and went ahead with #358 (thanks @MaferMazu!) and supporting filtering via the admin-console MFE. This will definitely introduce complexity to the MFE logic, which will be addressed in the next release cycle. |
mariajgrimaldi
left a comment
mariajgrimaldi
left a comment
There was a problem hiding this comment.
Thank you so much for this! I'm currently rebuilding my environment to thoroughly test this, but in the meantime I left a few clarifying questions :)
| // Libraries span orgs and are always enabled, so they must keep their behavior: only | ||
| // filter orgs by the course-authoring flag for course-only users, and never while | ||
| // permissions or flag states are still loading (default to showing every org). | ||
| const filterByAuthoringFlag = !isLoading && !isFlagLoading && !isLibraryViewAllowed; |
There was a problem hiding this comment.
Does this work when a user has both library & course roles?
There was a problem hiding this comment.
I'm testing and it works!
| value: org.shortName, | ||
| })) || [], [orgsData]); | ||
|
|
||
| // Libraries span orgs and are always enabled, so they must keep their behavior: only |
There was a problem hiding this comment.
Libraries span orgs and...?
| const orgOff = new Set(flagStates?.orgOverrides.off ?? []); | ||
| const global = flagStates?.global ?? false; | ||
|
|
||
| const courseEnabled = (courseId: string): boolean => { |
There was a problem hiding this comment.
What about courseFlagOverrideEnabled?
|
|
||
| const orgsWithForcedOnCourse = new Set([...courseOn].map(orgOf)); | ||
|
|
||
| const orgAuthoringEnabled = (org: string): boolean => { |
There was a problem hiding this comment.
What about orgFlagOverrideEnabled?
| {/* Loaded pages can be entirely filtered out client-side while later pages | ||
| still hold visible scopes, so only report empty once every page is in. */} | ||
| {orderedOrgs.length === 0 && !hasNextPage && !isFetchingNextPage && ( |
There was a problem hiding this comment.
Does this affect the search? I mean, if I search for an org or course it's not on (forced off or globally off), would it appear in the results because it's on another page?
This comment only makes sense when this is true: https://github.com/openedx/frontend-app-admin-console/pull/176/changes#r3595738419
There was a problem hiding this comment.
If you search for an org or course that is not ON, it won't appear because of this line
| const courseOff = new Set(flagStates?.courseOverrides.off ?? []); | ||
| const orgOn = new Set(flagStates?.orgOverrides.on ?? []); | ||
| const orgOff = new Set(flagStates?.orgOverrides.off ?? []); | ||
| const global = flagStates?.global ?? false; |
There was a problem hiding this comment.
Did we also filter the role assignments list based on the flag as well? Does it make sense to implement it? I thought we were going to but it's not clear from the cover letter if that's our intent.
Although the scope course-v1:Demo+DM+2026 doesn't appear in the filters or in the assignment wizard, it does in the assignment list, suggesting it might be on. One relevant point is that I can delete the assignment from the assignment list within the disabled scope, which is what we're trying to avoid: actions on disabled scopes.
There was a problem hiding this comment.
@mariajgrimaldi I tried implementing client-side filtering on the assignment list, but it only introduces UX problems that are tricky to resolve cleanly: the table is server-driven and paginated, so filtering rows after they arrive from the API breaks the item count ("Showing 8 of 8" when only 6 are visible) and produces phantom pages (e.g. showing 2 pages when everything fits in 1, because the items all get filtered out client-side).
This is actually the reason ADR 0003 explicitly decided against it:
The assignment lists are server-driven and paginated. Client-side filtering would invalidate pagination, counts, and sorting. The backend therefore remains responsible for hiding authoring-only assignments.
The intent is that Casbin migration (ADR 0013 in openedx-authz) keeps the assignment data aligned with the flag. Once migration runs, disabled-scope assignments won't be returned by the API at all, so the list stays clean without any frontend workaround.
That said, your concern about actions on disabled scopes is valid, so I've disabled the view action button for course assignments where isCourseEnabled(scope) returns false. This prevents navigating into a disabled scope to delete the assignment
The scenario you observed where the scope appears is still the known migration-lag window acknowledged in the ADR. The disabled button is the temporary guard I think we can apply without breaking the table UX, what do you say?
There was a problem hiding this comment.
To avoid the behavior described by Brayan and keep everything consistent the solution is to convert the tables to client-side, that is a bigger refactor. Furthermore I do not recommend.
I think that is a good solution @bra-i-am, ideally for both tables (global assignments and user specific assignments, we still can do deep navigation) and may be add a tooltip explained that assignment/actions over the course is disabled.
There was a problem hiding this comment.
I think this makes sense. Considering we're not targeting changes in the backend, this works well!
|
@bra-i-am: Do you think we can add test cases for each row in this table? #176 (comment) |
|
@mariajgrimaldi @dcoa @MaferMazu, I've been testing this using the table shared by Diana, and this is what I've found: Platform flag [ CHECK ✅ ]
ORG Override [ CHECK ✅ ]
Course Override [ CHECK ✅ ]
Override Priority (Cascade)
Test ConclusionsWhat works correctly
Confirmed known limitationsAssignment list is not filtered — in every scenario, the table shows Casbin rows regardless of the flag state. This is the documented limitation in ADR 0003 (Decision 4 + Implications section). Fixing it requires the backend to filter Overall, the behavior matches expectations across all tested scenarios. The only known limitation (assignment list not filtered by flag state) is documented in ADR 0003 and intentionally out of scope for this PR |
06db9fe to
b3c5f7c
Compare
b3c5f7c to
843486d
Compare
mariajgrimaldi
added
the
create-sandbox
|
@bra-i-am: thanks a lot for all the testing! I don't want to block this further but I wonder whether we can implement all the manual testing using jest. In any case, this looks great to me! Thanks a lot for addressing all of my comments :) |
mariajgrimaldi
added
create-sandbox
|
@mariajgrimaldi, I added Jest coverage for those manual cases:
|
7 participants
Closes openedx/openedx-authz#340
Closes openedx/openedx-authz#341
When
authz.enable_course_authoringis off, the Admin Console was still displaying course-related content in filters, the Team Members table, the Audit User view, and the role assignment wizard. This PR makes all those surfaces aware of the flag state using the dedicatedGET /api/authz/v1/waffle-flag-states/endpoint introduced in openedx-authz#358.How it works
The flag state is fetched from a dedicated endpoint that returns the global value plus every org and course that has an active override, split into
onandofflists:{ "global": false, "org_overrides": { "on": ["Demo"], "off": [] }, "course_overrides": { "on": [], "off": ["course-v1:testing+CT01+CT01-2024"] } }A new
useCourseAuthoringFlaghook centralises all resolution logic and exposes three resolvers:isCourseAuthoringEnabled— true if the flag is on at any tier (global, org, or course). Used as a coarse domain-level gate for the role filter and assignment wizard.isCourseEnabled(courseId)— resolves enablement for a specific course using the precedence course override → org override → global. Used to filter individual courses out of the scope filter and wizard scope list.isOrgAuthoringEnabled(org)— true if the org is explicitly enabled, has at least one course with an explicitonoverride, or inherits a globalonwithout an explicit orgoff. Used to filter the org filter for course-only users.Flag-state resolution is intentionally separate from permission validation (
useViewTeamPermissions), because Casbin state can lag behind the flag when automatic migration is disabled or when the global flag changes. See ADR 0003 for the full rationale.Main changes
data/api.ts— addsgetCourseAuthoringFlagStatesand theCourseAuthoringFlagStates/WaffleFlagOverridesinterfaces.data/hooks.ts— addsuseCourseAuthoringFlagStateshooks/useCourseAuthoringFlag.ts— derivesisCourseAuthoringEnabled,isCourseEnabled,isOrgAuthoringEnabled,isLoading, andisErrorfrom the flag states query. Surfaces a single error toast with retry on fetch failure.RolesFilter— course roles require bothisCourseViewAllowedandisCourseAuthoringEnabled.ScopesFilter— filters each course individually withisCourseEnabled(scope.externalKey); libraries are always included.OrgFilter— filters orgs withisOrgAuthoringEnabledfor course-only users; users with library access always see all orgs.AssignRoleWizardPage— course roles gated byisCourseAuthoringEnabled(the flag) independently of management permissions.useScopeListData— filters the wizard's scope list client-side withisCourseEnabledScopeList— empty state only shown once all pages have loaded, since client-side filtering can empty pages while later pages still contain visible scopes.docs/decisions/0003-course-authoring-flag-enforcement.rst— ADR documenting the enforcement approach, its temporary nature, rejected alternatives, and future removal criteria.Test plan
Setup: you need an instance with the
authz.enable_course_authoringflag and at least one org or course override configured. The filter, scope, and wizard tests do not require migration — they read the flag state directly. The assignment list tests (TeamMembersTable, AuditUser) require migration to have run, either by enablingENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = Truefor org/course-level changes, or by runningauthz_rollback_course_authoringmanually for global flag changes.waffle-flag-statesendpoint unreachable