Skip to content

feat: remove block-specific handling from runtime role checks [FC-0026] - #32356

Merged
Agrendalath merged 2 commits into
openedx:masterfrom
open-craft:agrendalath/fc-0026-user-checks
Jun 15, 2023
Merged

feat: remove block-specific handling from runtime role checks [FC-0026]#32356
Agrendalath merged 2 commits into
openedx:masterfrom
open-craft:agrendalath/fc-0026-user-checks

Conversation

@Agrendalath

@Agrendalath Agrendalath commented Jun 2, 2023

Copy link
Copy Markdown
Member

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:

  1. user_is_staff - this is redundant because x_module.py already contains a shim with the user_is_staff property. We can remove it without any consequences.
  2. user_is_admin and user_is_beta_tester - these two are good candidates for the user service, so we can move them there. However, the ora2 XBlock uses these runtime attributes, so we need a compatibility shim in x_module.py. I tried switching ora2 to the user service, but this will require a significant effort (preferably also to the workbench from xblock-sdk), so adding shims is a safer approach at the moment.
  3. days_early_for_beta - this variable is already available directly in the XBlock (via the InheritanceMixin), so adding it to the runtime is redundant. However, this runtime attribute is used by the ora2 block, so we can remove it only after merging refactor: use days_early_for_beta from InheritanceMixin instead of runtime [FC-0026] edx-ora2#1977.

Supporting information

Private-ref: BB-7448

Testing instructions

  1. Use testing instructions from refactor: use days_early_for_beta from InheritanceMixin instead of runtime [FC-0026] edx-ora2#1977.
  2. Check that you can see the .wrapper--staff-toolbar element in LMS when logged in as the edx user.

Other information

We are removing days_early_for_beta from the runtime. Therefore, this should be merged after openedx/edx-ora2#1977.

@Agrendalath Agrendalath self-assigned this Jun 2, 2023
@openedx-webhooks

openedx-webhooks commented Jun 2, 2023

Copy link
Copy Markdown

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 openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core committer labels Jun 2, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread xmodule/x_module.py Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Agrendalath
Agrendalath marked this pull request as ready for review June 8, 2023 19:55
@Agrendalath

Copy link
Copy Markdown
Member Author

@0x29a, @bradenmacdonald, this is ready for review.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no block-specific staff users, so passing the course_id is enough.

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

  • I tested this: as described on the ORA2 PR.
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: requested more comments

Comment thread common/djangoapps/xblock_django/constants.py Outdated
@Agrendalath

Agrendalath commented Jun 14, 2023

Copy link
Copy Markdown
Member Author

@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 ora2 version then.

@ormsbee

ormsbee commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

@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
Agrendalath force-pushed the agrendalath/fc-0026-user-checks branch from f281c47 to 629bc8f Compare June 15, 2023 09:07
@Agrendalath
Agrendalath force-pushed the agrendalath/fc-0026-user-checks branch from 629bc8f to bc17163 Compare June 15, 2023 09:09
@Agrendalath
Agrendalath merged commit e439b3f into openedx:master Jun 15, 2023
@Agrendalath
Agrendalath deleted the agrendalath/fc-0026-user-checks branch June 15, 2023 13:33
@openedx-webhooks

Copy link
Copy Markdown

@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-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core committer open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants