diff --git a/cms/djangoapps/contentstore/views/certificates.py b/cms/djangoapps/contentstore/views/certificates.py index deaa47decb..ed00f26ead 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 feffbbe071..24814be99e 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]) @@ -220,6 +220,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( @@ -388,6 +389,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': [] } @@ -419,6 +421,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': [] } @@ -493,9 +496,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) @@ -508,6 +511,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 ba808f2c7c..adbb6964fe 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(){ @@ -169,6 +175,16 @@ function(_, Course, CertificatesCollection, CertificateModel, CertificateDetails expect(this.view.$(SELECTORS.signatory_organization_value)).toContainText(''); }); + 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 @@