From e11c1d120c7ed6f1ec1a106991ef2f5ee3812076 Mon Sep 17 00:00:00 2001 From: Zia Fazal Date: Wed, 5 Aug 2015 12:56:10 +0500 Subject: [PATCH] certificate edit/delete is only allowed if user is edX PM or certificate is not active removed org helper --- .../contentstore/views/certificates.py | 28 +++++++++---- .../views/tests/test_certificates.py | 39 +++++++++++++++++-- .../spec/views/certificate_details_spec.js | 24 ++++++++++-- .../spec/views/certificate_editor_spec.js | 5 ++- .../certificates/views/certificate_editor.js | 1 + .../js/certificate-details.underscore | 2 +- .../js/certificate-editor.underscore | 2 +- cms/templates/js/signatory-details.underscore | 2 + 8 files changed, 84 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/contentstore/views/certificates.py b/cms/djangoapps/contentstore/views/certificates.py index b07c762ef8..5803a6cac7 100644 --- a/cms/djangoapps/contentstore/views/certificates.py +++ b/cms/djangoapps/contentstore/views/certificates.py @@ -178,6 +178,7 @@ class CertificateManager(object): "description": certificate_data['description'], "version": CERTIFICATE_SCHEMA_VERSION, "org_logo_path": certificate_data.get('org_logo_path', ''), + "is_active": certificate_data.get('is_active', False), "signatories": certificate_data['signatories'] } @@ -209,13 +210,17 @@ class CertificateManager(object): return certificate @staticmethod - def get_certificates(course): + def get_certificates(course, only_active=False): """ - Retrieve the certificates list from the provided course + Retrieve the certificates list from the provided course, + if `only_active` is True it would skip inactive certificates. """ # The top-level course field is 'certificates', which contains various properties, # including the actual 'certificates' list that we're working with in this context - return course.certificates.get('certificates', []) + certificates = course.certificates.get('certificates', []) + if only_active: + certificates = [certificate for certificate in certificates if certificate['is_active']] + return certificates @staticmethod def remove_certificate(request, store, course, certificate_id): @@ -436,6 +441,12 @@ def certificates_detail_handler(request, course_key_string, certificate_id): store = modulestore() if request.method in ('POST', 'PUT'): + if certificate_id: + active_certificates = CertificateManager.get_certificates(course, only_active=True) + if int(certificate_id) in [int(certificate["id"]) for certificate in active_certificates]: + # Only global staff (PMs) are able to edit active certificate configuration + if not GlobalStaff().has_user(request.user): + raise PermissionDenied() try: new_certificate = CertificateManager.deserialize_certificate(course, request.body) except CertificateValidationError as err: @@ -457,12 +468,15 @@ def certificates_detail_handler(request, course_key_string, certificate_id): return JsonResponse(serialized_certificate, status=201) elif request.method == "DELETE": - # Only global staff (PMs) are able to activate/deactivate certificate configuration - if not GlobalStaff().has_user(request.user): - raise PermissionDenied() - if not match_cert: return JsonResponse(status=404) + + active_certificates = CertificateManager.get_certificates(course, only_active=True) + if int(certificate_id) in [int(certificate["id"]) for certificate in active_certificates]: + # Only global staff (PMs) are able to delete active certificate configuration + if not GlobalStaff().has_user(request.user): + raise PermissionDenied() + CertificateManager.remove_certificate( request=request, store=store, diff --git a/cms/djangoapps/contentstore/views/tests/test_certificates.py b/cms/djangoapps/contentstore/views/tests/test_certificates.py index 72f80e3f61..341ad21181 100644 --- a/cms/djangoapps/contentstore/views/tests/test_certificates.py +++ b/cms/djangoapps/contentstore/views/tests/test_certificates.py @@ -67,7 +67,7 @@ class HelperMethods(object): ) contentstore().save(content) - def _add_course_certificates(self, count=1, signatory_count=0): + def _add_course_certificates(self, count=1, signatory_count=0, is_active=False): """ Create certificate for the course. """ @@ -96,7 +96,7 @@ class HelperMethods(object): 'org_logo_path': '/c4x/test/CSS101/asset/org_logo{}.png'.format(i), 'signatories': signatories, 'version': CERTIFICATE_SCHEMA_VERSION, - 'is_active': False + 'is_active': is_active } for i in xrange(0, count) ] self._create_fake_images([certificate['org_logo_path'] for certificate in certificates]) @@ -221,6 +221,7 @@ class CertificatesListHandlerTestCase(EventTestMixin, CourseTestCase, Certificat u'name': u'Test certificate', u'description': u'Test description', u'org_logo_path': '', + u'is_active': False, u'signatories': [] } response = self.client.ajax_post( @@ -389,6 +390,7 @@ class CertificatesDetailHandlerTestCase(EventTestMixin, CourseTestCase, Certific u'description': u'Test description', u'course_title': u'Course Title Override', u'org_logo_path': '', + u'is_active': False, u'signatories': [] } @@ -420,6 +422,7 @@ class CertificatesDetailHandlerTestCase(EventTestMixin, CourseTestCase, Certific u'description': u'New test description', u'course_title': u'Course Title Override', u'org_logo_path': '', + u'is_active': False, u'signatories': [] } @@ -494,9 +497,9 @@ class CertificatesDetailHandlerTestCase(EventTestMixin, CourseTestCase, Certific def test_delete_certificate_without_global_staff_permissions(self): """ - Tests certificate deletion without global staff permission on course. + Tests deletion of an active certificate without global staff permission on course. """ - self._add_course_certificates(count=2, signatory_count=1) + self._add_course_certificates(count=2, signatory_count=1, is_active=True) user = UserFactory() for role in [CourseInstructorRole, CourseStaffRole]: role(self.course.id).add_users(user) @@ -509,6 +512,34 @@ class CertificatesDetailHandlerTestCase(EventTestMixin, CourseTestCase, Certific ) self.assertEqual(response.status_code, 403) + def test_update_active_certificate_without_global_staff_permissions(self): + """ + Tests update of an active certificate without global staff permission on course. + """ + self._add_course_certificates(count=2, signatory_count=1, is_active=True) + cert_data = { + u'id': 1, + u'version': CERTIFICATE_SCHEMA_VERSION, + u'name': u'New test certificate', + u'description': u'New test description', + u'course_title': u'Course Title Override', + u'org_logo_path': '', + u'is_active': False, + u'signatories': [] + } + user = UserFactory() + for role in [CourseInstructorRole, CourseStaffRole]: + role(self.course.id).add_users(user) + self.client.login(username=user.username, password='test') + response = self.client.put( + self._url(cid=1), + data=json.dumps(cert_data), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest", + ) + self.assertEqual(response.status_code, 403) + def test_delete_non_existing_certificate(self): """ Try to delete a non existing certificate. It should return status code 404 Not found. diff --git a/cms/static/js/certificates/spec/views/certificate_details_spec.js b/cms/static/js/certificates/spec/views/certificate_details_spec.js index f51db0d849..b8822b8a46 100644 --- a/cms/static/js/certificates/spec/views/certificate_details_spec.js +++ b/cms/static/js/certificates/spec/views/certificate_details_spec.js @@ -80,8 +80,8 @@ function(_, Course, CertificatesCollection, CertificateModel, CertificateDetails this.model = new CertificateModel({ name: 'Test Name', description: 'Test Description', - course_title: 'Test Course Title Override' - + course_title: 'Test Course Title Override', + is_active: true }, this.newModelOptions); this.collection = new CertificatesCollection([ this.model ], { @@ -139,11 +139,17 @@ function(_, Course, CertificatesCollection, CertificateModel, CertificateDetails expect(this.model.get('editing')).toBe(true); }); + it('should not present a Edit action if user is not global staff and certificate is active', function () { + window.CMS.User = {isGlobalStaff: false}; + appendSetFixtures(this.view.render().el); + expect(this.view.$('.action-edit .edit')).not.toExist(); + }); + it('should present a Delete action', function () { expect(this.view.$('.action-delete .delete')).toExist(); }); - it('should not present a Delete action if user is not global staff', function () { + it('should not present a Delete action if user is not global staff and certificate is active', function () { window.CMS.User = {isGlobalStaff: false}; appendSetFixtures(this.view.render().el); expect(this.view.$('.action-delete .delete')).not.toExist(); @@ -159,7 +165,7 @@ function(_, Course, CertificatesCollection, CertificateModel, CertificateDetails describe('Signatory details', function(){ beforeEach(function() { - this.view.render(true); + this.view.render(); }); it('displays certificate signatories details', function(){ @@ -171,6 +177,16 @@ function(_, Course, CertificatesCollection, CertificateModel, CertificateDetails ).toContainText('Organization of the signatory'); }); + it('should present Edit action on signaotry', function () { + expect(this.view.$(SELECTORS.edit_signatory)).toExist(); + }); + + it('should not present Edit action on signaotry if user is not global staff and certificate is active', function () { + window.CMS.User = {isGlobalStaff: false}; + this.view.render(); + expect(this.view.$(SELECTORS.edit_signatory)).not.toExist(); + }); + it('supports in-line editing of signatory information', function() { this.view.$(SELECTORS.edit_signatory).click(); diff --git a/cms/static/js/certificates/spec/views/certificate_editor_spec.js b/cms/static/js/certificates/spec/views/certificate_editor_spec.js index 3e63777548..c934a8d2a2 100644 --- a/cms/static/js/certificates/spec/views/certificate_editor_spec.js +++ b/cms/static/js/certificates/spec/views/certificate_editor_spec.js @@ -113,7 +113,8 @@ function(_, Course, CertificateModel, SignatoryModel, CertificatesCollection, Ce this.newModelOptions = {add: true}; this.model = new CertificateModel({ name: 'Test Name', - description: 'Test Description' + description: 'Test Description', + is_active: true }, this.newModelOptions); @@ -151,7 +152,7 @@ function(_, Course, CertificateModel, SignatoryModel, CertificatesCollection, Ce expect(this.view.$('.action-delete')).toExist(); }); - it('should not have delete button is user is not global staff', function() { + it('should not have delete button if user is not global staff and certificate is active', function() { window.CMS.User = {isGlobalStaff: false}; appendSetFixtures(this.view.render().el); expect(this.view.$('.action-delete')).not.toExist(); diff --git a/cms/static/js/certificates/views/certificate_editor.js b/cms/static/js/certificates/views/certificate_editor.js index 1de9415f41..84f1307ecf 100644 --- a/cms/static/js/certificates/views/certificate_editor.js +++ b/cms/static/js/certificates/views/certificate_editor.js @@ -102,6 +102,7 @@ function($, _, Backbone, gettext, description: this.model.escape('description'), course_title: this.model.escape('course_title'), org_logo_path: this.model.escape('org_logo_path'), + is_active: this.model.escape('is_active'), isNew: this.model.isNew() }; }, diff --git a/cms/templates/js/certificate-details.underscore b/cms/templates/js/certificate-details.underscore index 97bb5b3175..6dcac84cef 100644 --- a/cms/templates/js/certificate-details.underscore +++ b/cms/templates/js/certificate-details.underscore @@ -30,10 +30,10 @@