fix perms instructor certificates #257

melvinsoft · 2018-07-26T15:25:16Z

The Certificates tab in the Instructor dashboard (and all the action) require global staff permissions, which doesn't makes sense with the rest of the Instructor dashboard architecture. The permissions check is always if the user is staff in the course, not global.

This bug came up in Tahoe since none of our users have global staff, but it's affecting other customers as well, also this is something we should push upstream.

bryanlandia

Hey @melvinsoft - looks great. A couple things:

Please add another test method below here:

edx-platform/lms/djangoapps/instructor/tests/test_certificates.py

Line 63 in a8cf3f8

def test_visible_only_to_global_staff(self):

to check for the non-global staff condition. Also, I wonder if there are business reasons edX has chosen global staff only. What do you think?

I checked, and this functionality hasn't changed at HEAD of edx/master so yeah, probably a good candidate for upstream.

melvinsoft · 2018-07-31T13:32:53Z

@bryanlandia Thanks for reviewing the PR. I've asked in the #general channel in edX slack, but I didn't received a response yet.

I'm gonna wait one more day and then ping again.

OmarIthawi · 2018-08-02T10:13:21Z

That's a huge change @melvinsoft. While it might work fine, but I think we should think twice before getting it merged.

Please also fix the import, otherwise courseware won't work: from util.views import require_global_staff like this:

13:05:36 omar@oryx:edx-platform $ git diff
diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py
index 73357f9..970f3dd 100644
--- a/lms/djangoapps/courseware/views/views.py
+++ b/lms/djangoapps/courseware/views/views.py
@@ -33,7 +33,7 @@ from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey, UsageKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from rest_framework import status
-from lms.djangoapps.instructor.views.api import require_global_staff
+from util.views import require_global_staff
 from lms.djangoapps.ccx.utils import prep_course_for_grading
 from lms.djangoapps.grades.new.course_grade import CourseGradeFactory
 from lms.djangoapps.instructor.enrollment import uses_shib

There's also the unpleasant fact that tests are failing:

$ paver test_system --disable-migrations --fasttest -t lms/djangoapps/instructor/tests/test_certificates.py --nologcapture

Succeeds without your change, all the 49. When adding the changes in this PR, 4 tests fails:

test_certificate_generation_api_without_global_staff
test_allow_only_global_staff
test_visible_only_to_global_staff

Could you please check them?

OmarIthawi

Thanks @melvinsoft! Please consider fixing the tests, and the import issue.

OmarIthawi · 2018-08-02T10:14:08Z

lms/djangoapps/instructor/views/api.py

@@ -35,7 +35,6 @@
    FileValidationException, UniversalNewlineIterator
 )
 from util.json_request import JsonResponse, JsonResponseBadRequest
-from util.views import require_global_staff


This line breaks the expectations in lms/djangoapps/courseware/views/views.py, please consider keeping it.

OmarIthawi · 2018-08-02T16:14:33Z

Bryan: to check for the non-global staff condition. Also, I wonder if there are business reasons edX has chosen global staff only. What do you think?

@bryanlandia @melvinsoft I thought that edX chose that for a business reason also, but looking at both of the:

JIRA Issue: https://openedx.atlassian.net/browse/SOL-1333
Original PR: https://github.com/edx/edx-platform/pull/10518

There seem to be nothing about which staff to use.

OmarIthawi · 2018-08-06T10:58:25Z

@melvinsoft Update: edX engineer Saleem Latif have just replied and it looks like our change is fine. I'm going to do a final test and mark this PR as ✔️

OmarIthawi

@melvinsoft Thanks for catching the error and fixing it.

If the change works for AMC, please merge it. Let's please consider contributing this upstream. It looks like an easy one.

OmarIthawi · 2018-08-06T11:40:14Z

lms/djangoapps/instructor/tests/test_certificates.py

@@ -53,6 +53,7 @@ def setUp(self):
        super(CertificatesInstructorDashTest, self).setUp()
        self.global_staff = GlobalStaffFactory()
        self.instructor = InstructorFactory(course_key=self.course.id)
+        self.staff = StaffFactory(course_key=self.course.id)


StaffFactory is not imported. Tests are still broken.

@OmarIthawi this line is still broken, right?

@melvinsoft take another look when you get a chance.

@bryanlandia @OmarIthawi import fixed! Sorry I cannot run tests properly on the AMC devstack ATM. Please give it another try and let me know.

melvinsoft · 2018-08-08T21:55:40Z

@bryanlandia @OmarIthawi Thanks for all your efforts here! Much appreciated! I've fixed the tests, but I cannot try them in my devstack, please let me know if it works to you, and if everything looks good, I'm going to squash and rebase the PR before merge. Thanks again!

OmarIthawi · 2018-08-09T13:11:20Z

@melvinsoft It fixes part of the broken tests, but not all of them.

Please consider having the latest edX docker devstack so you can run tests on. Having the tests fail quickly is better than waiting for someone else to report.

melvinsoft · 2018-09-17T21:25:10Z

@OmarIthawi @bryanlandia I tried to run the tests in a new docker devstack, and they're still failing. We need to deliver this to the customer, so we've two options:

Some of you volunteer to fix the tests and merge this.
We merge it and we create some tech debt, with a card to address it as soon as we can.

Please let me know.

OmarIthawi · 2018-09-18T06:28:52Z

We merge it and we create some tech debt, with a card to address it as soon as we can.

@melvinsoft We create tech debt regularly so this won't be an issue.

However, the fact that tests are failing, could mean an actual bug being pushed to production.

It depends on how much you'd like to ensure that another bug isn't happening. Both manual and automated tests work equally in this case.

If you think it's ready, please feel free to deploy.

melvinsoft · 2018-09-18T22:14:52Z

@OmarIthawi Thanks! but I did one more effort, and I run the test, cherrypicking the commits, and I was able to fix them. Can you do a final review over the changes? // cc @bryanlandia

Thanks!

OmarIthawi

@melvinsoft Thanks for taking the extra efforts. This is really helpful for maintenance in the future.

Please address the the issue below, and feel free to merge.

OmarIthawi · 2018-09-19T05:27:16Z

lms/djangoapps/instructor/tests/test_certificates.py

@@ -284,25 +296,6 @@ def _assert_redirects_to_instructor_dash(self, response):
        expected_redirect += '#view-certificates'
        self.assertRedirects(response, expected_redirect)

-    def test_certificate_generation_api_without_global_staff(self):


Please add the test back, I've fixed it:

def test_certificate_generation_api_without_global_staff(self): """ Test certificates generation api endpoint returns permission denied if user who made the request is not member of global staff. """ user = UserFactory.create() self.client.login(username=user.username, password='test') url = reverse( 'start_certificate_generation', kwargs={'course_id': unicode(self.course.id)} ) response = self.client.post(url) self.assertEqual(response.status_code, 403) self.client.login(username=self.instructor.username, password='test') response = self.client.post(url) self.assertEqual(response.status_code, 200)

Thanks omar! Test added!

No longer applicable. Thanks Bryan!

…or dashboard change all require_global_staff for require_level fix tests and import fix import in test for StaffFactory fix more tests fix test

melvinsoft · 2018-09-19T15:30:13Z

Commits squashed!

Per 12. Make your fix public. I am merging this fix.

melvinsoft requested review from johnbaldwin, bezidejni, tkeemon, OmarIthawi, bryanlandia and amirtds and removed request for johnbaldwin July 26, 2018 15:33

bryanlandia previously requested changes Jul 27, 2018

View reviewed changes

OmarIthawi suggested changes Aug 2, 2018

View reviewed changes

OmarIthawi approved these changes Aug 6, 2018

View reviewed changes

OmarIthawi approved these changes Sep 19, 2018

View reviewed changes

Allow per course staff members to access Certificates tab in instruct…

1262ba9

…or dashboard change all require_global_staff for require_level fix tests and import fix import in test for StaffFactory fix more tests fix test

melvinsoft force-pushed the bugfix/fix-perms-instructor-certificates branch from f288e44 to 1262ba9 Compare September 19, 2018 15:29

melvinsoft merged commit a297ea0 into appsembler/amc/develop Sep 19, 2018

OmarIthawi deleted the bugfix/fix-perms-instructor-certificates branch November 12, 2019 14:54

OmarIthawi mentioned this pull request Nov 12, 2019

Broken Tahoe test for certificates #478

Closed

shadinaif pushed a commit that referenced this pull request Nov 16, 2022

fix: studio edit permissions (#257)

f9c3937

Per 12. Make your fix public. I am merging this fix.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix perms instructor certificates #257

fix perms instructor certificates #257

melvinsoft commented Jul 26, 2018 •

edited

bryanlandia left a comment

melvinsoft commented Jul 31, 2018 •

edited

OmarIthawi commented Aug 2, 2018

OmarIthawi left a comment

OmarIthawi Aug 2, 2018

OmarIthawi commented Aug 2, 2018

OmarIthawi commented Aug 6, 2018

OmarIthawi left a comment

OmarIthawi Aug 6, 2018

bryanlandia Aug 8, 2018

bryanlandia Aug 8, 2018

melvinsoft Aug 8, 2018

melvinsoft commented Aug 8, 2018

OmarIthawi commented Aug 9, 2018 •

edited

melvinsoft commented Sep 17, 2018

OmarIthawi commented Sep 18, 2018

melvinsoft commented Sep 18, 2018

OmarIthawi left a comment

OmarIthawi Sep 19, 2018

melvinsoft Sep 19, 2018

melvinsoft commented Sep 19, 2018

fix perms instructor certificates #257

fix perms instructor certificates #257

Conversation

melvinsoft commented Jul 26, 2018 • edited

bryanlandia left a comment

Choose a reason for hiding this comment

melvinsoft commented Jul 31, 2018 • edited

OmarIthawi commented Aug 2, 2018

OmarIthawi left a comment

Choose a reason for hiding this comment

OmarIthawi Aug 2, 2018

Choose a reason for hiding this comment

OmarIthawi commented Aug 2, 2018

OmarIthawi commented Aug 6, 2018

OmarIthawi left a comment

Choose a reason for hiding this comment

OmarIthawi Aug 6, 2018

Choose a reason for hiding this comment

bryanlandia Aug 8, 2018

Choose a reason for hiding this comment

bryanlandia Aug 8, 2018

Choose a reason for hiding this comment

melvinsoft Aug 8, 2018

Choose a reason for hiding this comment

melvinsoft commented Aug 8, 2018

OmarIthawi commented Aug 9, 2018 • edited

melvinsoft commented Sep 17, 2018

OmarIthawi commented Sep 18, 2018

melvinsoft commented Sep 18, 2018

OmarIthawi left a comment

Choose a reason for hiding this comment

OmarIthawi Sep 19, 2018

Choose a reason for hiding this comment

melvinsoft Sep 19, 2018

Choose a reason for hiding this comment

melvinsoft commented Sep 19, 2018

melvinsoft commented Jul 26, 2018 •

edited

melvinsoft commented Jul 31, 2018 •

edited

OmarIthawi commented Aug 9, 2018 •

edited