From 57f1dc92b38cd75c19838985a7a20ca068313723 Mon Sep 17 00:00:00 2001 From: Zia Fazal Date: Wed, 8 Jul 2015 16:02:45 +0500 Subject: [PATCH] ziafazal/SOL-1021: Capture additional certificate configuration events * Changed certificate_id to configuration_id * Fix for acceptance test faiiure * Add waits to bok choy tests fix for flaky test --- .../contentstore/views/certificates.py | 33 +++++++++++++ .../views/tests/test_certificates.py | 40 ++++++++++++++-- .../pages/studio/settings_certificates.py | 48 ++++++++++++------- .../test_studio_settings_certificates.py | 7 ++- lms/djangoapps/certificates/api.py | 6 ++- lms/djangoapps/certificates/tests/test_api.py | 10 +++- 6 files changed, 118 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/views/certificates.py b/cms/djangoapps/contentstore/views/certificates.py index 96e2f522e6..13bde7476a 100644 --- a/cms/djangoapps/contentstore/views/certificates.py +++ b/cms/djangoapps/contentstore/views/certificates.py @@ -33,6 +33,7 @@ from django.views.decorators.http import require_http_methods from contentstore.utils import reverse_course_url from edxmako.shortcuts import render_to_response from opaque_keys.edx.keys import CourseKey, AssetKey +from eventtracking import tracker from student.auth import has_studio_write_access from util.db import generate_int_id, MYSQL_MAX_INT from util.json_request import JsonResponse @@ -247,6 +248,20 @@ class CertificateManager(object): store.update_item(course, request.user.id) break + @staticmethod + def track_event(event_name, event_data): + """Track certificate configuration event. + + Arguments: + event_name (str): Name of the event to be logged. + event_data (dict): A Dictionary containing event data + Returns: + None + + """ + event_name = '.'.join(['edx', 'certificate', 'configuration', event_name]) + tracker.emit(event_name, event_data) + class Certificate(object): """ @@ -296,6 +311,10 @@ def certificate_activation_handler(request, course_key_string): break store.update_item(course, request.user.id) + cert_event_type = 'activated' if is_active else 'deactivated' + CertificateManager.track_event(cert_event_type, { + 'course_id': unicode(course.id), + }) return HttpResponse(status=200) @@ -375,6 +394,10 @@ def certificates_list_handler(request, course_key_string): kwargs={'certificate_id': new_certificate.id} # pylint: disable=no-member ) store.update_item(course, request.user.id) + CertificateManager.track_event('created', { + 'course_id': unicode(course.id), + 'configuration_id': new_certificate.id + }) course = _get_course_and_check_access(course_key, request.user) return response else: @@ -414,12 +437,18 @@ def certificates_detail_handler(request, course_key_string, certificate_id): return JsonResponse({"error": err.message}, status=400) serialized_certificate = CertificateManager.serialize_certificate(new_certificate) + cert_event_type = 'created' if match_cert: + cert_event_type = 'modified' certificates_list[match_index] = serialized_certificate else: certificates_list.append(serialized_certificate) store.update_item(course, request.user.id) + CertificateManager.track_event(cert_event_type, { + 'course_id': unicode(course.id), + 'configuration_id': serialized_certificate["id"] + }) return JsonResponse(serialized_certificate, status=201) elif request.method == "DELETE": @@ -431,6 +460,10 @@ def certificates_detail_handler(request, course_key_string, certificate_id): course=course, certificate_id=certificate_id ) + CertificateManager.track_event('deleted', { + 'course_id': unicode(course.id), + 'configuration_id': certificate_id + }) return JsonResponse(status=204) diff --git a/cms/djangoapps/contentstore/views/tests/test_certificates.py b/cms/djangoapps/contentstore/views/tests/test_certificates.py index ec3a8a8f87..37e6a41a99 100644 --- a/cms/djangoapps/contentstore/views/tests/test_certificates.py +++ b/cms/djangoapps/contentstore/views/tests/test_certificates.py @@ -24,6 +24,7 @@ from student.tests.factories import UserFactory from contentstore.views.certificates import CertificateManager from django.test.utils import override_settings from contentstore.utils import get_lms_link_for_certificate_web_view +from util.testing import EventTestMixin FEATURES_WITH_CERTS_ENABLED = settings.FEATURES.copy() FEATURES_WITH_CERTS_ENABLED['CERTIFICATES_HTML_VIEW'] = True @@ -194,7 +195,7 @@ class CertificatesBaseTestCase(object): # pylint: disable=no-member @override_settings(FEATURES=FEATURES_WITH_CERTS_ENABLED) -class CertificatesListHandlerTestCase(CourseTestCase, CertificatesBaseTestCase, HelperMethods): +class CertificatesListHandlerTestCase(EventTestMixin, CourseTestCase, CertificatesBaseTestCase, HelperMethods): """ Test cases for certificates_list_handler. """ @@ -202,7 +203,7 @@ class CertificatesListHandlerTestCase(CourseTestCase, CertificatesBaseTestCase, """ Set up CertificatesListHandlerTestCase. """ - super(CertificatesListHandlerTestCase, self).setUp() + super(CertificatesListHandlerTestCase, self).setUp('contentstore.views.certificates.tracker') def _url(self): """ @@ -229,8 +230,13 @@ class CertificatesListHandlerTestCase(CourseTestCase, CertificatesBaseTestCase, self.assertEqual(response.status_code, 201) self.assertIn("Location", response) content = json.loads(response.content) - self._remove_ids(content) # pylint: disable=unused-variable + certificate_id = self._remove_ids(content) # pylint: disable=unused-variable self.assertEqual(content, expected) + self.assert_event_emitted( + 'edx.certificate.configuration.created', + course_id=unicode(self.course.id), + configuration_id=certificate_id, + ) def test_cannot_create_certificate_if_user_has_no_write_permissions(self): """ @@ -347,13 +353,19 @@ class CertificatesListHandlerTestCase(CourseTestCase, CertificatesBaseTestCase, @ddt.ddt @override_settings(FEATURES=FEATURES_WITH_CERTS_ENABLED) -class CertificatesDetailHandlerTestCase(CourseTestCase, CertificatesBaseTestCase, HelperMethods): +class CertificatesDetailHandlerTestCase(EventTestMixin, CourseTestCase, CertificatesBaseTestCase, HelperMethods): """ Test cases for CertificatesDetailHandlerTestCase. """ _id = 0 + def setUp(self): # pylint: disable=arguments-differ + """ + Set up CertificatesDetailHandlerTestCase. + """ + super(CertificatesDetailHandlerTestCase, self).setUp('contentstore.views.certificates.tracker') + def _url(self, cid=-1): """ Return url for the handler. @@ -388,6 +400,11 @@ class CertificatesDetailHandlerTestCase(CourseTestCase, CertificatesBaseTestCase ) content = json.loads(response.content) self.assertEqual(content, expected) + self.assert_event_emitted( + 'edx.certificate.configuration.created', + course_id=unicode(self.course.id), + configuration_id=666, + ) def test_can_edit_certificate(self): """ @@ -415,6 +432,11 @@ class CertificatesDetailHandlerTestCase(CourseTestCase, CertificatesBaseTestCase ) content = json.loads(response.content) self.assertEqual(content, expected) + self.assert_event_emitted( + 'edx.certificate.configuration.modified', + course_id=unicode(self.course.id), + configuration_id=1, + ) self.reload_course() # Verify that certificate is properly updated in the course. @@ -440,6 +462,11 @@ class CertificatesDetailHandlerTestCase(CourseTestCase, CertificatesBaseTestCase HTTP_X_REQUESTED_WITH="XMLHttpRequest", ) self.assertEqual(response.status_code, 204) + self.assert_event_emitted( + 'edx.certificate.configuration.deleted', + course_id=unicode(self.course.id), + configuration_id='1', + ) self.reload_course() # Verify that certificates are properly updated in the course. certificates = self.course.certificates['certificates'] @@ -553,6 +580,11 @@ class CertificatesDetailHandlerTestCase(CourseTestCase, CertificatesBaseTestCase course = self.store.get_course(self.course.id) certificates = course.certificates['certificates'] self.assertEqual(certificates[0].get('is_active'), is_active) + cert_event_type = 'activated' if is_active else 'deactivated' + self.assert_event_emitted( + '.'.join(['edx.certificate.configuration', cert_event_type]), + course_id=unicode(self.course.id), + ) @ddt.data(True, False) def test_certificate_activation_without_write_permissions(self, activate): diff --git a/common/test/acceptance/pages/studio/settings_certificates.py b/common/test/acceptance/pages/studio/settings_certificates.py index 6d521efbaa..5916de4359 100644 --- a/common/test/acceptance/pages/studio/settings_certificates.py +++ b/common/test/acceptance/pages/studio/settings_certificates.py @@ -79,8 +79,18 @@ class CertificatesPage(CoursePage): We can't use confirm_prompt because its wait_for_notification is flaky when asynchronous operation completed very quickly. """ - self.wait_for_element_visibility('.prompt', 'Prompt is visible') - self.wait_for_element_visibility('.prompt .action-primary', 'Confirmation button is visible') + EmptyPromise( + lambda: self.q(css='.prompt').present, + 'Confirmation prompt is displayed' + ).fulfill() + EmptyPromise( + lambda: self.q(css='.prompt .action-primary').present, + 'Primary button is displayed' + ).fulfill() + EmptyPromise( + lambda: self.q(css='.prompt .action-primary').visible, + 'Primary button is visible' + ).fulfill() def wait_for_first_certificate_button(self): """ @@ -108,17 +118,23 @@ class CertificatesPage(CoursePage): """ Clicks the 'Create your first certificate' button, which is only displayed at zero state """ + self.wait_for_first_certificate_button() self.q(css=self.certficate_css + " .new-button").first.click() def click_add_certificate_button(self): """ Clicks the 'Add new certificate' button, which is displayed when certificates already exist """ + self.wait_for_add_certificate_button() self.q(css=self.certficate_css + " .action-add").first.click() - ################ - # Workflows - ################ + def click_confirmation_prompt_primary_button(self): + """ + Clicks the main action presented by the prompt (such as 'Delete') + """ + self.wait_for_confirmation_prompt() + self.q(css='a.button.action-primary').first.click() + self.wait_for_ajax() class Certificate(object): @@ -240,13 +256,19 @@ class Certificate(object): """ Returns whether or not the certificate delete icon is present. """ - return self.find_css('.actions .delete').present + EmptyPromise( + lambda: self.find_css('.actions .delete').present, + 'Certificate delete button is displayed' + ).fulfill() def wait_for_hide_details_toggle(self): """ Certificate details are expanded. """ - return self.find_css('a.detail-toggle.hide-details').present + EmptyPromise( + lambda: self.find_css('a.detail-toggle.hide-details').present, + 'Certificate details are expanded' + ).fulfill() ################ # Click Actions @@ -290,20 +312,12 @@ class Certificate(object): """ self.find_css('a.detail-toggle').first.click() - ################ - # Workflows - ################ - - def delete_certificate(self): + def click_delete_certificate_button(self): """ - Delete the certificate + Remove the first (possibly the only) certificate from the set """ self.wait_for_certificate_delete_button() - self.find_css('.actions .delete').first.click() - self.page.wait_for_confirmation_prompt() - self.page.q(css='a.button.action-primary').first.click() - self.page.q(css='a.button.action-primary').first.click() self.page.wait_for_ajax() diff --git a/common/test/acceptance/tests/studio/test_studio_settings_certificates.py b/common/test/acceptance/tests/studio/test_studio_settings_certificates.py index ced72eb795..8bd566678f 100644 --- a/common/test/acceptance/tests/studio/test_studio_settings_certificates.py +++ b/common/test/acceptance/tests/studio/test_studio_settings_certificates.py @@ -126,9 +126,12 @@ class CertificatesTest(StudioCourseTest): self.assertEqual(len(self.certificates_page.certificates), 1) - # Delete certificate - certificate.delete_certificate() + # Delete the certificate we just created + certificate.click_delete_certificate_button() + self.certificates_page.click_confirmation_prompt_primary_button() + self.certificates_page.wait_for_first_certificate_button() + # Reload the page and confirm there are no certificates self.certificates_page.visit() self.assertEqual(len(self.certificates_page.certificates), 0) diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 3691ac4d45..f8142c62c4 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -154,7 +154,11 @@ def set_cert_generation_enabled(course_key, is_enabled): """ CertificateGenerationCourseSetting.set_enabled_for_course(course_key, is_enabled) - + cert_event_type = 'enabled' if is_enabled else 'disabled' + event_name = '.'.join(['edx', 'certificate', 'generation', cert_event_type]) + tracker.emit(event_name, { + 'course_id': unicode(course_key), + }) if is_enabled: log.info(u"Enabled self-generated certificates for course '%s'.", unicode(course_key)) else: diff --git a/lms/djangoapps/certificates/tests/test_api.py b/lms/djangoapps/certificates/tests/test_api.py index 9942c434e4..6e2edcdd60 100644 --- a/lms/djangoapps/certificates/tests/test_api.py +++ b/lms/djangoapps/certificates/tests/test_api.py @@ -229,13 +229,13 @@ class GenerateUserCertificatesTest(EventTestMixin, ModuleStoreTestCase): @attr('shard_1') @ddt.ddt -class CertificateGenerationEnabledTest(TestCase): +class CertificateGenerationEnabledTest(EventTestMixin, TestCase): """Test enabling/disabling self-generated certificates for a course. """ COURSE_KEY = CourseLocator(org='test', course='test', run='test') def setUp(self): - super(CertificateGenerationEnabledTest, self).setUp() + super(CertificateGenerationEnabledTest, self).setUp('certificates.api.tracker') # Since model-based configuration is cached, we need # to clear the cache before each test. @@ -256,6 +256,12 @@ class CertificateGenerationEnabledTest(TestCase): if is_course_enabled is not None: certs_api.set_cert_generation_enabled(self.COURSE_KEY, is_course_enabled) + cert_event_type = 'enabled' if is_course_enabled else 'disabled' + event_name = '.'.join(['edx', 'certificate', 'generation', cert_event_type]) + self.assert_event_emitted( + event_name, + course_id=unicode(self.COURSE_KEY), + ) self._assert_enabled_for_course(self.COURSE_KEY, expect_enabled)