Merge pull request #9197 from edx/ziafazal/SOL-1077-fix

SOL-1077: certificate config edit/delete is only allowed if user is edX PM or for those certificate configs not active

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,

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.

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();

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();

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()
             };
         },

cms/templates/js/certificate-details.underscore

@@ -30,10 +30,10 @@
    </ol>
 
     <ul class="actions certificate-actions">
+        <% if (CMS.User.isGlobalStaff || !is_active) { %>
         <li class="action action-edit">
             <button class="edit"><i class="icon fa fa-pencil" aria-hidden="true"></i> <%= gettext("Edit") %></button>
         </li>
-        <% if (CMS.User.isGlobalStaff) { %>
         <li class="action action-delete wrapper-delete-button" data-tooltip="<%= gettext('Delete') %>">
             <button class="delete action-icon"><i class="icon fa fa-trash-o" aria-hidden="true"></i><span><%= gettext("Delete") %></span></button>
         </li>

cms/templates/js/certificate-editor.underscore

@@ -48,7 +48,7 @@
     <div class="actions">
         <button class="action action-primary" type="submit"><% if (isNew) { print(gettext("Create")) } else { print(gettext("Save")) } %></button>
         <button class="action action-secondary action-cancel"><%= gettext("Cancel") %></button>
-        <% if (!isNew && CMS.User.isGlobalStaff) { %>
+        <% if (!isNew && (CMS.User.isGlobalStaff || !is_active)) { %>
             <span class="wrapper-delete-button">
                 <a class="button action-delete delete" href="#"><%= gettext("Delete") %></a>
             </span>

cms/templates/js/signatory-details.underscore

 <div class="signatory-panel-default">
+  <% if (CMS.User.isGlobalStaff || !certificate.get('is_active')) { %>
   <div class="actions certificate-actions signatory-panel-edit">
     <span class="action action-edit-signatory">
       <a href="javascript:void(0);" class="edit-signatory"><i class="icon fa fa-pencil" aria-hidden="true"></i> <%= gettext("Edit") %></a>
     </span>
   </div>
+  <% } %>
   <div class="signatory-panel-header">Signatory <%= signatory_number %>&nbsp;</div>
   <div class="signatory-panel-body">
     <div>