feat: remove block-specific handling from runtime role checks [FC-0026] #32356
Conversation
|
Thanks for the pull request, @Agrendalath! As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion. |
openedx-webhooks
added
open-source-contribution
| user_location=user_location, | ||
| request_token=request_token, | ||
| will_recheck_access=will_recheck_access, | ||
| ), |
There was a problem hiding this comment.
I moved the initialization of these services directly to the services dict to clarify that they are not used as dependencies elsewhere (e.g., in the wrappers).
| ) | ||
| user_service = self._runtime_services.get('user') or self._services.get('user') | ||
| if user_service: | ||
| return user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_GLOBAL_STAFF) |
There was a problem hiding this comment.
I added these shims to maintain backward compatibility. We already have one for user_is_staff, and we don't need one for days_early_for_beta because it's already in the XBlock fields.
|
@0x29a, @bradenmacdonald, this is ready for review. |
| @@ -493,32 +493,6 @@ def inner_get_block(block): | |||
| will_recheck_access=will_recheck_access, | |||
| ) | |||
|
|
|||
| user_is_staff = bool(has_access(user, 'staff', block.location, course_id)) | |||
There was a problem hiding this comment.
There are no block-specific staff users, so passing the course_id is enough.
|
@ormsbee, would you like to review this before we merge? openedx/edx-ora2#1977 should be merged and released tomorrow, so I'll bump the |
|
@Agrendalath: I skimmed through it and I don't see anything that worries me. I don't think I need to review in more detail. Thank you. |
Agrendalath
force-pushed
the
agrendalath/fc-0026-user-checks
branch
from
f281c47 to
629bc8f
Compare
Agrendalath
force-pushed
the
agrendalath/fc-0026-user-checks
branch
from
629bc8f to
bc17163
Compare
|
@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
|
EdX Release Notice: This PR has been deployed to the production environment. |
Description
The goal of FC-0026 is to remove the XBlock-specific handling from the prepare_runtime_for_user function. This part handles user role checks.
We have the following attributes in the runtime:
user_is_staff- this is redundant becausex_module.pyalready contains a shim with theuser_is_staffproperty. We can remove it without any consequences.user_is_adminanduser_is_beta_tester- these two are good candidates for theuserservice, so we can move them there. However, theora2XBlock uses these runtime attributes, so we need a compatibility shim inx_module.py. I tried switchingora2to the user service, but this will require a significant effort (preferably also to the workbench fromxblock-sdk), so adding shims is a safer approach at the moment.days_early_for_beta- this variable is already available directly in the XBlock (via theInheritanceMixin), so adding it to the runtime is redundant. However, this runtime attribute is used by theora2block, so we can remove it only after merging refactor: usedays_early_for_betafromInheritanceMixininstead of runtime [FC-0026] edx-ora2#1977.Supporting information
Private-ref: BB-7448
Testing instructions
days_early_for_betafromInheritanceMixininstead of runtime [FC-0026] edx-ora2#1977..wrapper--staff-toolbarelement in LMS when logged in as theedxuser.Other information
We are removing
days_early_for_betafrom the runtime. Therefore, this should be merged after openedx/edx-ora2#1977.