Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: consider user role staff for masquerading #27

Merged
merged 3 commits into from
Oct 2, 2023

Conversation

mariajgrimaldi
Copy link
Contributor

@mariajgrimaldi mariajgrimaldi commented Sep 28, 2023

Description

This PR considers the staff course role as well so masquerading as a course staff works too. I'm trying to figure out what causes this behavior. To reproduce:

  1. As a course admin (instructor), go to your course with this xblock installed
  2. Enter the unit
  3. Even though you're viewing the course as staff, select view course as staff.

The Grade Submissions should disappear after this. What happens is that when you enter the course you have the instructor role, but then you view the course as staff (after pressing view as staff) now you have the staff role. So even though you reload the page or enter the course again, the Grade Submissions button won't appear again unless you log out your account and login again.

@mariajgrimaldi mariajgrimaldi marked this pull request as ready for review September 28, 2023 22:06
@mariajgrimaldi mariajgrimaldi requested a review from a team September 29, 2023 12:41
@BryanttV
Copy link
Contributor

@mariajgrimaldi When I select view course as staff, and click the Grade Submissions button, I get that I don't have permission. Should that be the behavior?

  File "/openedx/requirements/xblock-mindmap/mindmap/mindmap.py", line 472, in get_instructor_grading_data
    require(self.is_instructor())
  File "/openedx/requirements/xblock-mindmap/mindmap/mindmap.py", line 778, in require
    raise PermissionDenied
django.core.exceptions.PermissionDenied

@mariajgrimaldi
Copy link
Contributor Author

@BryanttV: no, thanks. I'll make sure to fix it right away.

@mariajgrimaldi
Copy link
Contributor Author

@BryanttV: can you test again? Thanks!

@BryanttV
Copy link
Contributor

Perfect, it works!

@@ -589,7 +589,7 @@ def remove_grade(self, data, _suffix="") -> dict:
# Lazy import: import here to avoid app not ready errors
from submissions.api import reset_score # pylint: disable=import-outside-toplevel

require(self.is_instructor())
require(self.is_instructor() or self.is_course_staff(self.get_current_user()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. But I'll need to do some refactoring first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @Ian2012. Can you check again?

@mariajgrimaldi mariajgrimaldi force-pushed the MJG/consider-staff-perm branch 3 times, most recently from f4bc7c8 to bb8cbd5 Compare October 2, 2023 17:22
@mariajgrimaldi
Copy link
Contributor Author

There have been some changes after you @BryanttV reviewed, can you check again? Thanks!

@mariajgrimaldi mariajgrimaldi force-pushed the MJG/consider-staff-perm branch from bb8cbd5 to 0a36208 Compare October 2, 2023 18:25
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/consider-staff-perm branch from 0a36208 to 41b0055 Compare October 2, 2023 19:25
@mariajgrimaldi mariajgrimaldi merged commit 41e4cbe into main Oct 2, 2023
@mariajgrimaldi mariajgrimaldi deleted the MJG/consider-staff-perm branch October 2, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants