From c4823c5ed6c86ddff55fa540176040a5955d25cb Mon Sep 17 00:00:00 2001 From: rabia23 Date: Mon, 22 May 2017 11:21:27 +0000 Subject: [PATCH 01/12] EDUCATOR-394 Disable self generation certificates when course is reset to instructor-paced. --- .../tests/lms/test_certificate_web_view.py | 7 ++++-- lms/djangoapps/certificates/models.py | 12 ++++++---- lms/djangoapps/certificates/signals.py | 22 ++++++++----------- .../certificates/tests/test_signals.py | 20 ++++++++++++----- lms/djangoapps/courseware/tests/test_views.py | 18 ++++++++++++--- 5 files changed, 52 insertions(+), 27 deletions(-) diff --git a/common/test/acceptance/tests/lms/test_certificate_web_view.py b/common/test/acceptance/tests/lms/test_certificate_web_view.py index 34ab8e178d..acc1e12ad1 100644 --- a/common/test/acceptance/tests/lms/test_certificate_web_view.py +++ b/common/test/acceptance/tests/lms/test_certificate_web_view.py @@ -12,6 +12,7 @@ from common.test.acceptance.pages.lms.course_info import CourseInfoPage from common.test.acceptance.pages.lms.courseware import CoursewarePage from common.test.acceptance.pages.lms.progress import ProgressPage from common.test.acceptance.pages.lms.tab_nav import TabNavPage +from common.test.acceptance.pages.lms.instructor_dashboard import InstructorDashboardPage @attr(shard=5) @@ -158,6 +159,7 @@ class CertificateProgressPageTest(UniqueCourseTest): self.courseware_page = CoursewarePage(self.browser, self.course_id) self.course_home_page = CourseHomePage(self.browser, self.course_id) self.tab_nav = TabNavPage(self.browser) + self.instructor_dashboard_page = InstructorDashboardPage(self.browser, self.course_id) def log_in_as_unique_user(self): """ @@ -168,7 +170,8 @@ class CertificateProgressPageTest(UniqueCourseTest): username="testprogress", email="progress@example.com", password="testuser", - course_id=self.course_id + course_id=self.course_id, + staff=True, ).visit() def test_progress_page_has_view_certificate_button(self): @@ -245,4 +248,4 @@ class CertificateProgressPageTest(UniqueCourseTest): # Submit the answer self.courseware_page.q(css='button.submit').click() - self.courseware_page.wait_for_ajax() + self.courseware_page.wait_for_ajax() \ No newline at end of file diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 736a648959..bbed49f87f 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -54,6 +54,7 @@ from django.conf import settings from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.db import models, transaction +from django.db.utils import IntegrityError from django.db.models import Count from django.dispatch import receiver from django.utils.translation import ugettext_lazy as _ @@ -890,10 +891,13 @@ class CertificateGenerationCourseSetting(TimeStampedModel): is_enabled (boolean): Whether to enable or disable self-generated certificates. """ - CertificateGenerationCourseSetting.objects.create( - course_key=course_key, - enabled=is_enabled - ) + certificate_generation_course_setting = CertificateGenerationCourseSetting(course_key=course_key, + enabled=is_enabled) + try: + with transaction.atomic(): + certificate_generation_course_setting.save() + except IntegrityError: + pass class CertificateGenerationConfiguration(ConfigurationModel): diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 4cab739d5f..e5b87df239 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -1,29 +1,25 @@ -""" Signal handler for enabling self-generated certificates by default -for self-paced courses. +""" Signal handler for enabling self-generated certificates for self-paced +courses and disabling for instructor-paced courses. """ from celery.task import task from django.dispatch.dispatcher import receiver from certificates.models import CertificateGenerationCourseSetting from opaque_keys.edx.keys import CourseKey -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from xmodule.modulestore.django import SignalHandler +from xmodule.modulestore.django import SignalHandler, modulestore @receiver(SignalHandler.course_published) def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument """ Catches the signal that a course has been published in Studio and - enable the self-generated certificates by default for self-paced - courses. + enable/disable the self-generated certificates for courses according to pacing. """ - enable_self_generated_certs.delay(unicode(course_key)) + set_self_generated_certs.delay(unicode(course_key)) @task() -def enable_self_generated_certs(course_key): - """Enable the self-generated certificates by default for self-paced courses.""" +def set_self_generated_certs(course_key): + """Enable or disable self-generated certificates for a course according to pacing.""" course_key = CourseKey.from_string(course_key) - course = CourseOverview.get_from_id(course_key) - is_enabled_for_course = CertificateGenerationCourseSetting.is_enabled_for_course(course_key) - if course.self_paced and not is_enabled_for_course: - CertificateGenerationCourseSetting.set_enabled_for_course(course_key, True) + course = modulestore().get_course(course_key) + CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course.self_paced) diff --git a/lms/djangoapps/certificates/tests/test_signals.py b/lms/djangoapps/certificates/tests/test_signals.py index 7e27c496b0..16106d4d2d 100644 --- a/lms/djangoapps/certificates/tests/test_signals.py +++ b/lms/djangoapps/certificates/tests/test_signals.py @@ -1,5 +1,5 @@ -""" Unit tests for enabling self-generated certificates by default -for self-paced courses. +""" Unit tests for enabling self-generated certificates for +self-paced courses and disabling for instructor-paced courses. """ from certificates import api as certs_api from certificates.models import CertificateGenerationConfiguration @@ -10,8 +10,8 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): - """ Tests for enabling self-generated certificates by default - for self-paced courses. + """ Tests for enabling/disabling self-generated certificates + according to pacing. """ def setUp(self): @@ -22,10 +22,20 @@ class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): CertificateGenerationConfiguration.objects.create(enabled=True) def test_cert_generation_enabled_for_self_paced(self): - """ Verify the signal enable the self-generated certificates by default for + """ Verify the signal enable the self-generated certificates for self-paced courses. """ self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) _listen_for_course_publish('store', self.course.id) self.assertTrue(certs_api.cert_generation_enabled(self.course.id)) + + def test_cert_generation_disabled_for_instructor_paced(self): + """ Verify the signal disable the self-generated certificates for + instructor-paced courses. + """ + self.course.self_paced = False + self.store.update_item(self.course, self.user.id) + + _listen_for_course_publish('store', self.course.id) + self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 4457ddaac9..8be980bd52 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1331,9 +1331,6 @@ class ProgressPageTests(ModuleStoreTestCase): # Enable the feature, but do not enable it for this course CertificateGenerationConfiguration(enabled=True).save() - # Enable certificate generation for this course - certs_api.set_cert_generation_enabled(self.course.id, True) - # Course certificate configurations certificates = [ { @@ -1352,6 +1349,13 @@ class ProgressPageTests(ModuleStoreTestCase): self.course.save() self.store.update_item(self.course, self.user.id) + # verify that certificate web view button disappears for disabled self-generation certificates + resp = self._get_progress_page() + self.assertNotContains(resp, u"View Certificate") + + # Enable certificate self-generation for this course + certs_api.set_cert_generation_enabled(self.course.id, True) + resp = self._get_progress_page() self.assertContains(resp, u"View Certificate") @@ -1363,6 +1367,11 @@ class ProgressPageTests(ModuleStoreTestCase): certificates[0]['is_active'] = False self.store.update_item(self.course, self.user.id) + # Re-enable certificate self-generation for this course because whenever + # course content changes then self-generation changes according to pacing + # (EDUCATOR-394) + certs_api.set_cert_generation_enabled(self.course.id, True) + resp = self._get_progress_page() self.assertNotContains(resp, u"View Your Certificate") self.assertNotContains(resp, u"You can now view your certificate") @@ -1493,6 +1502,9 @@ class ProgressPageTests(ModuleStoreTestCase): self.course.save() self.store.update_item(self.course, self.user.id) + # Enable certificate generation for this course (EDUCATOR-394) + certs_api.set_cert_generation_enabled(self.course.id, True) + resp = self._get_progress_page() self.assertContains(resp, u"View Certificate") self.assert_invalidate_certificate(generated_certificate) From d10be68166227b96bb956abd2d32958a9b7c7587 Mon Sep 17 00:00:00 2001 From: rabia23 Date: Sun, 28 May 2017 15:15:23 +0000 Subject: [PATCH 02/12] fix bokchoy test --- .../pages/lms/instructor_dashboard.py | 7 +++ .../tests/lms/test_certificate_web_view.py | 48 ++++++++++++++++--- 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index b6000e0e3f..2e3d80e9f1 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -1414,6 +1414,13 @@ class CertificatesPage(PageObject): """ self.get_selector('#invalidate-certificate').click() + @property + def self_generation_certificate_enabled_button(self): + """ + Returns the "Self Generation Certificate Enable" button. + """ + return self.get_selector('#enable-certificates-submit') + @property def generate_certificates_button(self): """ diff --git a/common/test/acceptance/tests/lms/test_certificate_web_view.py b/common/test/acceptance/tests/lms/test_certificate_web_view.py index 35bf75d0b9..f34d8d4a83 100644 --- a/common/test/acceptance/tests/lms/test_certificate_web_view.py +++ b/common/test/acceptance/tests/lms/test_certificate_web_view.py @@ -1,8 +1,14 @@ """ Acceptance tests for the certificate web view feature. """ +from common.test.acceptance.tests.helpers import ( + UniqueCourseTest, + EventsTestMixin, + load_data_str, + get_element_padding, + get_modal_alert, +) from nose.plugins.attrib import attr - from common.test.acceptance.fixtures.certificates import CertificateConfigFixture from common.test.acceptance.fixtures.course import CourseFixture, CourseUpdateDesc, XBlockFixtureDesc from common.test.acceptance.pages.lms.auto_auth import AutoAuthPage @@ -13,8 +19,8 @@ from common.test.acceptance.pages.lms.courseware import CoursewarePage from common.test.acceptance.pages.lms.progress import ProgressPage from common.test.acceptance.pages.lms.tab_nav import TabNavPage from common.test.acceptance.pages.lms.instructor_dashboard import InstructorDashboardPage -from common.test.acceptance.tests.helpers import EventsTestMixin, UniqueCourseTest, get_element_padding, load_data_str - +from common.test.acceptance.tests.helpers import disable_animations +from common.test.acceptance.pages.common.logout import LogoutPage @attr(shard=5) @@ -172,8 +178,20 @@ class CertificateProgressPageTest(UniqueCourseTest): username="testprogress", email="progress@example.com", password="testuser", + course_id=self.course_id + ).visit() + + def log_in_as_staff(self): + """ + Log in as a staff. + """ + AutoAuthPage( + self.browser, + username="teststaff", + email="test_staff@example.com", + password="teststaff", course_id=self.course_id, - staff=True, + staff=True ).visit() def test_progress_page_has_view_certificate_button(self): @@ -190,8 +208,10 @@ class CertificateProgressPageTest(UniqueCourseTest): And their should be no padding around Certificate info box. """ self.cert_fixture.install() - self.log_in_as_unique_user() + self.enable_self_generation_certificates() + + self.log_in_as_unique_user() self.complete_course_problems() self.course_info_page.visit() @@ -250,4 +270,20 @@ class CertificateProgressPageTest(UniqueCourseTest): # Submit the answer self.courseware_page.q(css='button.submit').click() - self.courseware_page.wait_for_ajax() \ No newline at end of file + self.courseware_page.wait_for_ajax() + + def enable_self_generation_certificates(self): + """ + Enable self-generation certificates for instructor-paced courses. + By default, it is disabled. (EDUCATOR-394) + """ + self.log_in_as_staff() + + self.instructor_dashboard_page.visit() + self.certificates_section = self.instructor_dashboard_page.select_certificates() + disable_animations(self.certificates_section) + self.certificates_section.self_generation_certificate_enabled_button.click() + alert = get_modal_alert(self.certificates_section.browser) + alert.accept() + self.certificates_section.wait_for_ajax() + LogoutPage(self.browser).visit() From 59b0d80dce92274490e22f48ce8ab7038db6bbcc Mon Sep 17 00:00:00 2001 From: rabiaiftikhar Date: Tue, 30 May 2017 13:58:45 +0500 Subject: [PATCH 03/12] modify doc strings and method names. --- .../pages/lms/instructor_dashboard.py | 4 +-- .../tests/lms/test_certificate_web_view.py | 28 +++++++++---------- lms/djangoapps/certificates/models.py | 4 +++ .../certificates/tests/test_signals.py | 6 ++-- lms/djangoapps/courseware/tests/test_views.py | 3 +- 5 files changed, 26 insertions(+), 19 deletions(-) diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index 2e3d80e9f1..da24277a1b 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -1415,9 +1415,9 @@ class CertificatesPage(PageObject): self.get_selector('#invalidate-certificate').click() @property - def self_generation_certificate_enabled_button(self): + def self_generated_certificate_enabled_button(self): """ - Returns the "Self Generation Certificate Enable" button. + Returns the "Self Generated Certificate Enable" button. """ return self.get_selector('#enable-certificates-submit') diff --git a/common/test/acceptance/tests/lms/test_certificate_web_view.py b/common/test/acceptance/tests/lms/test_certificate_web_view.py index f34d8d4a83..5e47c441ed 100644 --- a/common/test/acceptance/tests/lms/test_certificate_web_view.py +++ b/common/test/acceptance/tests/lms/test_certificate_web_view.py @@ -1,13 +1,6 @@ """ Acceptance tests for the certificate web view feature. """ -from common.test.acceptance.tests.helpers import ( - UniqueCourseTest, - EventsTestMixin, - load_data_str, - get_element_padding, - get_modal_alert, -) from nose.plugins.attrib import attr from common.test.acceptance.fixtures.certificates import CertificateConfigFixture from common.test.acceptance.fixtures.course import CourseFixture, CourseUpdateDesc, XBlockFixtureDesc @@ -21,7 +14,13 @@ from common.test.acceptance.pages.lms.tab_nav import TabNavPage from common.test.acceptance.pages.lms.instructor_dashboard import InstructorDashboardPage from common.test.acceptance.tests.helpers import disable_animations from common.test.acceptance.pages.common.logout import LogoutPage - +from common.test.acceptance.tests.helpers import ( + UniqueCourseTest, + EventsTestMixin, + load_data_str, + get_element_padding, + get_modal_alert, +) @attr(shard=5) class CertificateWebViewTest(EventsTestMixin, UniqueCourseTest): @@ -183,7 +182,7 @@ class CertificateProgressPageTest(UniqueCourseTest): def log_in_as_staff(self): """ - Log in as a staff. + Log in as a staff user. """ AutoAuthPage( self.browser, @@ -209,7 +208,7 @@ class CertificateProgressPageTest(UniqueCourseTest): """ self.cert_fixture.install() - self.enable_self_generation_certificates() + self.enable_self_generated_certificates() self.log_in_as_unique_user() self.complete_course_problems() @@ -272,17 +271,18 @@ class CertificateProgressPageTest(UniqueCourseTest): self.courseware_page.q(css='button.submit').click() self.courseware_page.wait_for_ajax() - def enable_self_generation_certificates(self): + def enable_self_generated_certificates(self): """ - Enable self-generation certificates for instructor-paced courses. - By default, it is disabled. (EDUCATOR-394) + Self-generated certificates should be enabled for the presence of View + Certificate button on Course Progress menu. By default, it is disabled + for instructor-paced courses. (EDUCATOR-394) """ self.log_in_as_staff() self.instructor_dashboard_page.visit() self.certificates_section = self.instructor_dashboard_page.select_certificates() disable_animations(self.certificates_section) - self.certificates_section.self_generation_certificate_enabled_button.click() + self.certificates_section.self_generated_certificate_enabled_button.click() alert = get_modal_alert(self.certificates_section.browser) alert.accept() self.certificates_section.wait_for_ajax() diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index bbed49f87f..9a42fc1048 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -897,6 +897,10 @@ class CertificateGenerationCourseSetting(TimeStampedModel): with transaction.atomic(): certificate_generation_course_setting.save() except IntegrityError: + if is_enabled: + LOGGER.exception("Cannot enable self-generated certificates for course %s.", unicode(course_key)) + else: + LOGGER.exception("Cannot disable self-generated certificates for course %s.", unicode(course_key)) pass diff --git a/lms/djangoapps/certificates/tests/test_signals.py b/lms/djangoapps/certificates/tests/test_signals.py index 16106d4d2d..99e05f75a6 100644 --- a/lms/djangoapps/certificates/tests/test_signals.py +++ b/lms/djangoapps/certificates/tests/test_signals.py @@ -22,7 +22,8 @@ class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): CertificateGenerationConfiguration.objects.create(enabled=True) def test_cert_generation_enabled_for_self_paced(self): - """ Verify the signal enable the self-generated certificates for + """ + Verify the signal enables the self-generated certificates for self-paced courses. """ self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) @@ -31,7 +32,8 @@ class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): self.assertTrue(certs_api.cert_generation_enabled(self.course.id)) def test_cert_generation_disabled_for_instructor_paced(self): - """ Verify the signal disable the self-generated certificates for + """ + Verify the signal disables the self-generated certificates for instructor-paced courses. """ self.course.self_paced = False diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 8be980bd52..debda49780 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1349,7 +1349,8 @@ class ProgressPageTests(ModuleStoreTestCase): self.course.save() self.store.update_item(self.course, self.user.id) - # verify that certificate web view button disappears for disabled self-generation certificates + # verify that certificate web view button disappears if self-generated certificates + # are disabled resp = self._get_progress_page() self.assertNotContains(resp, u"View Certificate") From 755aad04f7b15b94ce9f7cde1f180242d485cd3a Mon Sep 17 00:00:00 2001 From: rabiaiftikhar Date: Tue, 30 May 2017 15:52:11 +0500 Subject: [PATCH 04/12] modify doc strings. --- .../tests/lms/test_certificate_web_view.py | 2 ++ lms/djangoapps/certificates/signals.py | 12 +++++++----- lms/djangoapps/certificates/tests/test_signals.py | 9 +++++---- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/common/test/acceptance/tests/lms/test_certificate_web_view.py b/common/test/acceptance/tests/lms/test_certificate_web_view.py index 5e47c441ed..2998385dea 100644 --- a/common/test/acceptance/tests/lms/test_certificate_web_view.py +++ b/common/test/acceptance/tests/lms/test_certificate_web_view.py @@ -2,6 +2,7 @@ Acceptance tests for the certificate web view feature. """ from nose.plugins.attrib import attr + from common.test.acceptance.fixtures.certificates import CertificateConfigFixture from common.test.acceptance.fixtures.course import CourseFixture, CourseUpdateDesc, XBlockFixtureDesc from common.test.acceptance.pages.lms.auto_auth import AutoAuthPage @@ -22,6 +23,7 @@ from common.test.acceptance.tests.helpers import ( get_modal_alert, ) + @attr(shard=5) class CertificateWebViewTest(EventsTestMixin, UniqueCourseTest): """ diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index e5b87df239..6097dfad8b 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -1,25 +1,27 @@ -""" Signal handler for enabling self-generated certificates for self-paced -courses and disabling for instructor-paced courses. +""" +Signal handler for enabling/disabling self-generated certificates based on the course-pacing. """ from celery.task import task from django.dispatch.dispatcher import receiver from certificates.models import CertificateGenerationCourseSetting from opaque_keys.edx.keys import CourseKey -from xmodule.modulestore.django import SignalHandler, modulestore +from xmodule.modulestore.django import modulestore, SignalHandler @receiver(SignalHandler.course_published) def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument """ Catches the signal that a course has been published in Studio and - enable/disable the self-generated certificates for courses according to pacing. + enable/disable the self-generated certificates according to course-pacing. """ set_self_generated_certs.delay(unicode(course_key)) @task() def set_self_generated_certs(course_key): - """Enable or disable self-generated certificates for a course according to pacing.""" + """ + Enable or disable self-generated certificates for a course according to pacing. + """ course_key = CourseKey.from_string(course_key) course = modulestore().get_course(course_key) CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course.self_paced) diff --git a/lms/djangoapps/certificates/tests/test_signals.py b/lms/djangoapps/certificates/tests/test_signals.py index 99e05f75a6..6ddcb98e3d 100644 --- a/lms/djangoapps/certificates/tests/test_signals.py +++ b/lms/djangoapps/certificates/tests/test_signals.py @@ -1,5 +1,6 @@ -""" Unit tests for enabling self-generated certificates for -self-paced courses and disabling for instructor-paced courses. +""" +Unit tests for enabling self-generated certificates for self-paced courses +and disabling for instructor-paced courses. """ from certificates import api as certs_api from certificates.models import CertificateGenerationConfiguration @@ -10,8 +11,8 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): - """ Tests for enabling/disabling self-generated certificates - according to pacing. + """ + Tests for enabling/disabling self-generated certificates according to course-pacing. """ def setUp(self): From 867bc9e3168e4e7772e6cb32b9c109de6210f94a Mon Sep 17 00:00:00 2001 From: rabiaiftikhar Date: Thu, 1 Jun 2017 15:04:42 +0500 Subject: [PATCH 05/12] get the course from CourseOverview table instead of Modulestore in certificates/signals.py. --- lms/djangoapps/certificates/signals.py | 8 ++++---- .../core/djangoapps/content/course_overviews/models.py | 8 ++++++++ .../core/djangoapps/content/course_overviews/signals.py | 6 ++++-- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 6097dfad8b..352df0a8ac 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -5,13 +5,13 @@ from celery.task import task from django.dispatch.dispatcher import receiver from certificates.models import CertificateGenerationCourseSetting +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview, COURSE_OVERVIEW_UPDATED from opaque_keys.edx.keys import CourseKey -from xmodule.modulestore.django import modulestore, SignalHandler -@receiver(SignalHandler.course_published) +@receiver(COURSE_OVERVIEW_UPDATED) def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument - """ Catches the signal that a course has been published in Studio and + """ Catches the signal that a course overview table has been updated in database and enable/disable the self-generated certificates according to course-pacing. """ set_self_generated_certs.delay(unicode(course_key)) @@ -23,5 +23,5 @@ def set_self_generated_certs(course_key): Enable or disable self-generated certificates for a course according to pacing. """ course_key = CourseKey.from_string(course_key) - course = modulestore().get_course(course_key) + course = CourseOverview.get_from_id(course_key) CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course.self_paced) diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index f61d994f3b..e2f96fc065 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -10,6 +10,7 @@ from django.db import models, transaction from django.db.models.fields import BooleanField, DateTimeField, DecimalField, TextField, FloatField, IntegerField from django.db.utils import IntegrityError from django.template import defaultfilters +from django.dispatch import Signal from ccx_keys.locator import CCXLocator from model_utils.models import TimeStampedModel @@ -26,6 +27,7 @@ from xmodule.modulestore.django import modulestore from openedx.core.djangoapps.xmodule_django.models import CourseKeyField, UsageKeyField from openedx.core.lib.xblock_fields.inherited_fields import DEFAULT_START_DATE +COURSE_OVERVIEW_UPDATED = Signal(providing_args=["course_key"]) log = logging.getLogger(__name__) @@ -646,6 +648,12 @@ class CourseOverview(TimeStampedModel): """Represent ourselves with the course key.""" return unicode(self.id) + def send_signal(self): + """ + Sends out the signal that course_overview table has been updated. + """ + COURSE_OVERVIEW_UPDATED.send(sender=None, course_key=self.id) + class CourseOverviewTab(models.Model): """ diff --git a/openedx/core/djangoapps/content/course_overviews/signals.py b/openedx/core/djangoapps/content/course_overviews/signals.py index f32cf24848..aab9bbafae 100644 --- a/openedx/core/djangoapps/content/course_overviews/signals.py +++ b/openedx/core/djangoapps/content/course_overviews/signals.py @@ -11,9 +11,11 @@ from xmodule.modulestore.django import SignalHandler def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument """ Catches the signal that a course has been published in Studio and - updates the corresponding CourseOverview cache entry. + updates the corresponding CourseOverview cache entry. Also sends out + the signal that course_overview table has been updated. """ - CourseOverview.load_from_module_store(course_key) + course_overview = CourseOverview.load_from_module_store(course_key) + course_overview.send_signal() @receiver(SignalHandler.course_deleted) From 955c64470ee9917bc776a86373f67a21f95ecba2 Mon Sep 17 00:00:00 2001 From: rabia23 Date: Sun, 11 Jun 2017 09:48:29 +0000 Subject: [PATCH 06/12] reset self-generation of certs according to course pacing --- lms/djangoapps/certificates/models.py | 16 ++++---------- lms/djangoapps/certificates/signals.py | 22 +++++++++---------- .../certificates/tests/test_signals.py | 4 ++-- .../content/course_overviews/models.py | 8 ------- .../content/course_overviews/signals.py | 6 ++--- .../core/djangoapps/models/course_details.py | 9 ++++++++ 6 files changed, 27 insertions(+), 38 deletions(-) diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 9a42fc1048..736a648959 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -54,7 +54,6 @@ from django.conf import settings from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.db import models, transaction -from django.db.utils import IntegrityError from django.db.models import Count from django.dispatch import receiver from django.utils.translation import ugettext_lazy as _ @@ -891,17 +890,10 @@ class CertificateGenerationCourseSetting(TimeStampedModel): is_enabled (boolean): Whether to enable or disable self-generated certificates. """ - certificate_generation_course_setting = CertificateGenerationCourseSetting(course_key=course_key, - enabled=is_enabled) - try: - with transaction.atomic(): - certificate_generation_course_setting.save() - except IntegrityError: - if is_enabled: - LOGGER.exception("Cannot enable self-generated certificates for course %s.", unicode(course_key)) - else: - LOGGER.exception("Cannot disable self-generated certificates for course %s.", unicode(course_key)) - pass + CertificateGenerationCourseSetting.objects.create( + course_key=course_key, + enabled=is_enabled + ) class CertificateGenerationConfiguration(ConfigurationModel): diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 352df0a8ac..7ffa066a41 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -2,26 +2,24 @@ Signal handler for enabling/disabling self-generated certificates based on the course-pacing. """ from celery.task import task -from django.dispatch.dispatcher import receiver +from django.dispatch import receiver from certificates.models import CertificateGenerationCourseSetting -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview, COURSE_OVERVIEW_UPDATED -from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.models.course_details import COURSE_PACING_CHANGE -@receiver(COURSE_OVERVIEW_UPDATED) -def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument - """ Catches the signal that a course overview table has been updated in database and - enable/disable the self-generated certificates according to course-pacing. +@receiver(COURSE_PACING_CHANGE, dispatch_uid="course_pacing_changed") +def _listen_for_course_publish(sender, course_key, course_self_paced, **kwargs): # pylint: disable=unused-argument """ - set_self_generated_certs.delay(unicode(course_key)) + Catches the signal that course pacing has changed and enable/disable + the self-generated certificates according to course-pacing. + """ + enable_self_generated_certs.delay(course_key, course_self_paced) @task() -def set_self_generated_certs(course_key): +def enable_self_generated_certs(course_key, course_self_paced): """ Enable or disable self-generated certificates for a course according to pacing. """ - course_key = CourseKey.from_string(course_key) - course = CourseOverview.get_from_id(course_key) - CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course.self_paced) + CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course_self_paced) diff --git a/lms/djangoapps/certificates/tests/test_signals.py b/lms/djangoapps/certificates/tests/test_signals.py index 6ddcb98e3d..b4ed25af89 100644 --- a/lms/djangoapps/certificates/tests/test_signals.py +++ b/lms/djangoapps/certificates/tests/test_signals.py @@ -29,7 +29,7 @@ class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): """ self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) - _listen_for_course_publish('store', self.course.id) + _listen_for_course_publish('store', self.course.id, self.course.self_paced) self.assertTrue(certs_api.cert_generation_enabled(self.course.id)) def test_cert_generation_disabled_for_instructor_paced(self): @@ -40,5 +40,5 @@ class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): self.course.self_paced = False self.store.update_item(self.course, self.user.id) - _listen_for_course_publish('store', self.course.id) + _listen_for_course_publish('store', self.course.id, self.course.self_paced) self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 168f190461..20330043ac 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -10,7 +10,6 @@ from django.db import models, transaction from django.db.models.fields import BooleanField, DateTimeField, DecimalField, TextField, FloatField, IntegerField from django.db.utils import IntegrityError from django.template import defaultfilters -from django.dispatch import Signal from ccx_keys.locator import CCXLocator from model_utils.models import TimeStampedModel @@ -26,7 +25,6 @@ from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore from openedx.core.djangoapps.xmodule_django.models import CourseKeyField, UsageKeyField -COURSE_OVERVIEW_UPDATED = Signal(providing_args=["course_key"]) log = logging.getLogger(__name__) @@ -647,12 +645,6 @@ class CourseOverview(TimeStampedModel): """Represent ourselves with the course key.""" return unicode(self.id) - def send_signal(self): - """ - Sends out the signal that course_overview table has been updated. - """ - COURSE_OVERVIEW_UPDATED.send(sender=None, course_key=self.id) - class CourseOverviewTab(models.Model): """ diff --git a/openedx/core/djangoapps/content/course_overviews/signals.py b/openedx/core/djangoapps/content/course_overviews/signals.py index aab9bbafae..f32cf24848 100644 --- a/openedx/core/djangoapps/content/course_overviews/signals.py +++ b/openedx/core/djangoapps/content/course_overviews/signals.py @@ -11,11 +11,9 @@ from xmodule.modulestore.django import SignalHandler def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument """ Catches the signal that a course has been published in Studio and - updates the corresponding CourseOverview cache entry. Also sends out - the signal that course_overview table has been updated. + updates the corresponding CourseOverview cache entry. """ - course_overview = CourseOverview.load_from_module_store(course_key) - course_overview.send_signal() + CourseOverview.load_from_module_store(course_key) @receiver(SignalHandler.course_deleted) diff --git a/openedx/core/djangoapps/models/course_details.py b/openedx/core/djangoapps/models/course_details.py index 6f0a2de250..1bbc39ac3d 100644 --- a/openedx/core/djangoapps/models/course_details.py +++ b/openedx/core/djangoapps/models/course_details.py @@ -5,6 +5,7 @@ import re import logging from django.conf import settings +from django.dispatch import Signal from xmodule.fields import Date from xmodule.modulestore.exceptions import ItemNotFoundError @@ -12,6 +13,8 @@ from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration from openedx.core.lib.courses import course_image_url from xmodule.modulestore.django import modulestore +COURSE_PACING_CHANGE = Signal(providing_args=["course_key", "course_self_paced"]) + # This list represents the attribute keys for a course's 'about' info. # Note: The 'video' attribute is intentionally excluded as it must be @@ -188,6 +191,7 @@ class CourseDetails(object): descriptor = module_store.get_course(course_key) dirty = False + is_pacing_changed = False # In the descriptor's setter, the date is converted to JSON # using Date's to_json method. Calling to_json on something that @@ -271,6 +275,7 @@ class CourseDetails(object): and jsondict['self_paced'] != descriptor.self_paced): descriptor.self_paced = jsondict['self_paced'] dirty = True + is_pacing_changed = True if dirty: module_store.update_item(descriptor, user.id) @@ -285,6 +290,10 @@ class CourseDetails(object): cls.update_about_video(descriptor, jsondict['intro_video'], user.id) + if is_pacing_changed: + # sends out the signal that course pacing has changed + COURSE_PACING_CHANGE.send(sender=None, course_key=course_key, course_self_paced=descriptor.self_paced) + # Could just return jsondict w/o doing any db reads, but I put # the reads in as a means to confirm it persisted correctly return CourseDetails.fetch(course_key) From b7b865615c2004f2ada0b853335221db47538784 Mon Sep 17 00:00:00 2001 From: rabia23 Date: Sun, 11 Jun 2017 11:20:23 +0000 Subject: [PATCH 07/12] fix unit and bokchoy tests --- .../pages/lms/instructor_dashboard.py | 7 --- .../tests/lms/test_certificate_web_view.py | 46 +------------------ lms/djangoapps/courseware/tests/test_views.py | 19 ++------ 3 files changed, 5 insertions(+), 67 deletions(-) diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index 627791f74f..6b59e6ac00 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -1456,13 +1456,6 @@ class CertificatesPage(PageObject): """ self.get_selector('#invalidate-certificate').click() - @property - def self_generated_certificate_enabled_button(self): - """ - Returns the "Self Generated Certificate Enable" button. - """ - return self.get_selector('#enable-certificates-submit') - @property def generate_certificates_button(self): """ diff --git a/common/test/acceptance/tests/lms/test_certificate_web_view.py b/common/test/acceptance/tests/lms/test_certificate_web_view.py index a4692d95e7..98911c6988 100644 --- a/common/test/acceptance/tests/lms/test_certificate_web_view.py +++ b/common/test/acceptance/tests/lms/test_certificate_web_view.py @@ -12,16 +12,7 @@ from common.test.acceptance.pages.lms.course_info import CourseInfoPage from common.test.acceptance.pages.lms.courseware import CoursewarePage from common.test.acceptance.pages.lms.progress import ProgressPage from common.test.acceptance.pages.lms.tab_nav import TabNavPage -from common.test.acceptance.pages.lms.instructor_dashboard import InstructorDashboardPage -from common.test.acceptance.tests.helpers import disable_animations -from common.test.acceptance.pages.common.logout import LogoutPage -from common.test.acceptance.tests.helpers import ( - UniqueCourseTest, - EventsTestMixin, - load_data_str, - get_element_padding, - get_modal_alert, -) +from common.test.acceptance.tests.helpers import EventsTestMixin, UniqueCourseTest, get_element_padding, load_data_str @attr(shard=5) @@ -168,7 +159,6 @@ class CertificateProgressPageTest(UniqueCourseTest): self.courseware_page = CoursewarePage(self.browser, self.course_id) self.course_home_page = CourseHomePage(self.browser, self.course_id) self.tab_nav = TabNavPage(self.browser) - self.instructor_dashboard_page = InstructorDashboardPage(self.browser, self.course_id) def log_in_as_unique_user(self): """ @@ -182,19 +172,6 @@ class CertificateProgressPageTest(UniqueCourseTest): course_id=self.course_id ).visit() - def log_in_as_staff(self): - """ - Log in as a staff user. - """ - AutoAuthPage( - self.browser, - username="teststaff", - email="test_staff@example.com", - password="teststaff", - course_id=self.course_id, - staff=True - ).visit() - def test_progress_page_has_view_certificate_button(self): """ Scenario: View Certificate option should be present on Course Progress menu if the user is @@ -209,10 +186,8 @@ class CertificateProgressPageTest(UniqueCourseTest): And their should be no padding around Certificate info box. """ self.cert_fixture.install() - - self.enable_self_generated_certificates() - self.log_in_as_unique_user() + self.complete_course_problems() self.course_info_page.visit() @@ -272,20 +247,3 @@ class CertificateProgressPageTest(UniqueCourseTest): # Submit the answer self.courseware_page.q(css='button.submit').click() self.courseware_page.wait_for_ajax() - - def enable_self_generated_certificates(self): - """ - Self-generated certificates should be enabled for the presence of View - Certificate button on Course Progress menu. By default, it is disabled - for instructor-paced courses. (EDUCATOR-394) - """ - self.log_in_as_staff() - - self.instructor_dashboard_page.visit() - self.certificates_section = self.instructor_dashboard_page.select_certificates() - disable_animations(self.certificates_section) - self.certificates_section.self_generated_certificate_enabled_button.click() - alert = get_modal_alert(self.certificates_section.browser) - alert.accept() - self.certificates_section.wait_for_ajax() - LogoutPage(self.browser).visit() diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index d5dff26344..1f9b1e7f35 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1361,6 +1361,9 @@ class ProgressPageTests(ProgressPageBaseTests): # Enable the feature, but do not enable it for this course CertificateGenerationConfiguration(enabled=True).save() + # Enable certificate generation for this course + certs_api.set_cert_generation_enabled(self.course.id, True) + # Course certificate configurations certificates = [ { @@ -1379,14 +1382,6 @@ class ProgressPageTests(ProgressPageBaseTests): self.course.save() self.store.update_item(self.course, self.user.id) - # verify that certificate web view button disappears if self-generated certificates - # are disabled - resp = self._get_progress_page() - self.assertNotContains(resp, u"View Certificate") - - # Enable certificate self-generation for this course - certs_api.set_cert_generation_enabled(self.course.id, True) - resp = self._get_progress_page() self.assertContains(resp, u"View Certificate") @@ -1398,11 +1393,6 @@ class ProgressPageTests(ProgressPageBaseTests): certificates[0]['is_active'] = False self.store.update_item(self.course, self.user.id) - # Re-enable certificate self-generation for this course because whenever - # course content changes then self-generation changes according to pacing - # (EDUCATOR-394) - certs_api.set_cert_generation_enabled(self.course.id, True) - resp = self._get_progress_page() self.assertNotContains(resp, u"View Your Certificate") self.assertNotContains(resp, u"You can now view your certificate") @@ -1533,9 +1523,6 @@ class ProgressPageTests(ProgressPageBaseTests): self.course.save() self.store.update_item(self.course, self.user.id) - # Enable certificate generation for this course (EDUCATOR-394) - certs_api.set_cert_generation_enabled(self.course.id, True) - resp = self._get_progress_page() self.assertContains(resp, u"View Certificate") self.assert_invalidate_certificate(generated_certificate) From b29d1e32a9aaeb97ee7ac264517a377000b97c6e Mon Sep 17 00:00:00 2001 From: rabiaiftikhar Date: Mon, 12 Jun 2017 11:47:02 +0500 Subject: [PATCH 08/12] Merge branch 'master' into ri/EDUCATOR-394-disable-self-generation-certificates --- lms/djangoapps/certificates/signals.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 7ffa066a41..219e11e129 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -22,4 +22,4 @@ def enable_self_generated_certs(course_key, course_self_paced): """ Enable or disable self-generated certificates for a course according to pacing. """ - CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course_self_paced) + CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course_self_paced) \ No newline at end of file From 648458010770da3ea7d9b239d7c490dfa5ceca08 Mon Sep 17 00:00:00 2001 From: rabia23 Date: Mon, 12 Jun 2017 11:19:17 +0000 Subject: [PATCH 09/12] fix coursekey error --- lms/djangoapps/certificates/signals.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 219e11e129..470dcc6415 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -3,6 +3,7 @@ Signal handler for enabling/disabling self-generated certificates based on the c """ from celery.task import task from django.dispatch import receiver +from opaque_keys.edx.keys import CourseKey from certificates.models import CertificateGenerationCourseSetting from openedx.core.djangoapps.models.course_details import COURSE_PACING_CHANGE @@ -14,7 +15,7 @@ def _listen_for_course_publish(sender, course_key, course_self_paced, **kwargs): Catches the signal that course pacing has changed and enable/disable the self-generated certificates according to course-pacing. """ - enable_self_generated_certs.delay(course_key, course_self_paced) + enable_self_generated_certs.delay(unicode(course_key), course_self_paced) @task() @@ -22,4 +23,5 @@ def enable_self_generated_certs(course_key, course_self_paced): """ Enable or disable self-generated certificates for a course according to pacing. """ - CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course_self_paced) \ No newline at end of file + course_key = CourseKey.from_string(course_key) + CertificateGenerationCourseSetting.set_enabled_for_course(course_key, course_self_paced) From a8913f9a6654db08cca33243e7589608f7a37edf Mon Sep 17 00:00:00 2001 From: rabia23 Date: Tue, 13 Jun 2017 06:51:38 +0000 Subject: [PATCH 10/12] Review Alex --- lms/djangoapps/certificates/signals.py | 4 ++-- openedx/core/djangoapps/models/course_details.py | 8 ++------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index 470dcc6415..abb9b5b4d0 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -15,11 +15,11 @@ def _listen_for_course_publish(sender, course_key, course_self_paced, **kwargs): Catches the signal that course pacing has changed and enable/disable the self-generated certificates according to course-pacing. """ - enable_self_generated_certs.delay(unicode(course_key), course_self_paced) + toggle_self_generated_certs.delay(unicode(course_key), course_self_paced) @task() -def enable_self_generated_certs(course_key, course_self_paced): +def toggle_self_generated_certs(course_key, course_self_paced): """ Enable or disable self-generated certificates for a course according to pacing. """ diff --git a/openedx/core/djangoapps/models/course_details.py b/openedx/core/djangoapps/models/course_details.py index 1bbc39ac3d..590a619cf8 100644 --- a/openedx/core/djangoapps/models/course_details.py +++ b/openedx/core/djangoapps/models/course_details.py @@ -191,7 +191,6 @@ class CourseDetails(object): descriptor = module_store.get_course(course_key) dirty = False - is_pacing_changed = False # In the descriptor's setter, the date is converted to JSON # using Date's to_json method. Calling to_json on something that @@ -275,7 +274,8 @@ class CourseDetails(object): and jsondict['self_paced'] != descriptor.self_paced): descriptor.self_paced = jsondict['self_paced'] dirty = True - is_pacing_changed = True + # sends out the signal that course pacing has changed + COURSE_PACING_CHANGE.send(sender=None, course_key=course_key, course_self_paced=descriptor.self_paced) if dirty: module_store.update_item(descriptor, user.id) @@ -290,10 +290,6 @@ class CourseDetails(object): cls.update_about_video(descriptor, jsondict['intro_video'], user.id) - if is_pacing_changed: - # sends out the signal that course pacing has changed - COURSE_PACING_CHANGE.send(sender=None, course_key=course_key, course_self_paced=descriptor.self_paced) - # Could just return jsondict w/o doing any db reads, but I put # the reads in as a means to confirm it persisted correctly return CourseDetails.fetch(course_key) From b84d4c0209fd885d8dd893507b2f589f1b94b812 Mon Sep 17 00:00:00 2001 From: rabiaiftikhar Date: Tue, 13 Jun 2017 15:43:36 +0500 Subject: [PATCH 11/12] Review Rehan --- lms/djangoapps/certificates/signals.py | 2 +- .../certificates/tests/test_signals.py | 18 +++++++----------- .../core/djangoapps/models/course_details.py | 8 ++++++-- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/certificates/signals.py b/lms/djangoapps/certificates/signals.py index abb9b5b4d0..387c63c73c 100644 --- a/lms/djangoapps/certificates/signals.py +++ b/lms/djangoapps/certificates/signals.py @@ -10,7 +10,7 @@ from openedx.core.djangoapps.models.course_details import COURSE_PACING_CHANGE @receiver(COURSE_PACING_CHANGE, dispatch_uid="course_pacing_changed") -def _listen_for_course_publish(sender, course_key, course_self_paced, **kwargs): # pylint: disable=unused-argument +def _listen_for_course_pacing_changed(sender, course_key, course_self_paced, **kwargs): # pylint: disable=unused-argument """ Catches the signal that course pacing has changed and enable/disable the self-generated certificates according to course-pacing. diff --git a/lms/djangoapps/certificates/tests/test_signals.py b/lms/djangoapps/certificates/tests/test_signals.py index b3a03f43eb..00374b080e 100644 --- a/lms/djangoapps/certificates/tests/test_signals.py +++ b/lms/djangoapps/certificates/tests/test_signals.py @@ -4,7 +4,7 @@ and disabling for instructor-paced courses. """ from certificates import api as certs_api from certificates.models import CertificateGenerationConfiguration -from certificates.signals import _listen_for_course_publish +from certificates.signals import _listen_for_course_pacing_changed from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -22,23 +22,19 @@ class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): # Enable the feature CertificateGenerationConfiguration.objects.create(enabled=True) - def test_cert_generation_enabled_for_self_paced(self): + + def test_cert_generation_flag_on_pacing_toggle(self): """ - Verify the signal enables the self-generated certificates for - self-paced courses. + Verify that signal enables or disables self-generated certificates + according to course-pacing. """ self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) - _listen_for_course_publish('store', self.course.id, self.course.self_paced) + _listen_for_course_pacing_changed('store', self.course.id, self.course.self_paced) self.assertTrue(certs_api.cert_generation_enabled(self.course.id)) - def test_cert_generation_disabled_for_instructor_paced(self): - """ - Verify the signal disables the self-generated certificates for - instructor-paced courses. - """ self.course.self_paced = False self.store.update_item(self.course, self.user.id) - _listen_for_course_publish('store', self.course.id, self.course.self_paced) + _listen_for_course_pacing_changed('store', self.course.id, self.course.self_paced) self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) diff --git a/openedx/core/djangoapps/models/course_details.py b/openedx/core/djangoapps/models/course_details.py index 590a619cf8..0765fcc518 100644 --- a/openedx/core/djangoapps/models/course_details.py +++ b/openedx/core/djangoapps/models/course_details.py @@ -191,6 +191,7 @@ class CourseDetails(object): descriptor = module_store.get_course(course_key) dirty = False + is_pacing_changed = False # In the descriptor's setter, the date is converted to JSON # using Date's to_json method. Calling to_json on something that @@ -274,8 +275,7 @@ class CourseDetails(object): and jsondict['self_paced'] != descriptor.self_paced): descriptor.self_paced = jsondict['self_paced'] dirty = True - # sends out the signal that course pacing has changed - COURSE_PACING_CHANGE.send(sender=None, course_key=course_key, course_self_paced=descriptor.self_paced) + is_pacing_changed = True if dirty: module_store.update_item(descriptor, user.id) @@ -290,6 +290,10 @@ class CourseDetails(object): cls.update_about_video(descriptor, jsondict['intro_video'], user.id) + # fires the signal that course pacing has changed after changes are reflected in db + if is_pacing_changed: + COURSE_PACING_CHANGE.send(sender=None, course_key=course_key, course_self_paced=descriptor.self_paced) + # Could just return jsondict w/o doing any db reads, but I put # the reads in as a means to confirm it persisted correctly return CourseDetails.fetch(course_key) From 22d3b028079893c59a6cdbf59e8d6525fd2d17dc Mon Sep 17 00:00:00 2001 From: rabiaiftikhar Date: Tue, 13 Jun 2017 16:29:17 +0500 Subject: [PATCH 12/12] respond to comments --- lms/djangoapps/certificates/tests/test_signals.py | 4 +++- openedx/core/djangoapps/models/course_details.py | 8 ++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/certificates/tests/test_signals.py b/lms/djangoapps/certificates/tests/test_signals.py index 00374b080e..1f90f8b492 100644 --- a/lms/djangoapps/certificates/tests/test_signals.py +++ b/lms/djangoapps/certificates/tests/test_signals.py @@ -22,19 +22,21 @@ class SelfGeneratedCertsSignalTest(ModuleStoreTestCase): # Enable the feature CertificateGenerationConfiguration.objects.create(enabled=True) - def test_cert_generation_flag_on_pacing_toggle(self): """ Verify that signal enables or disables self-generated certificates according to course-pacing. """ + #self-generation of cert disables by default self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) _listen_for_course_pacing_changed('store', self.course.id, self.course.self_paced) + #verify that self-generation of cert is enabled for self-paced course self.assertTrue(certs_api.cert_generation_enabled(self.course.id)) self.course.self_paced = False self.store.update_item(self.course, self.user.id) _listen_for_course_pacing_changed('store', self.course.id, self.course.self_paced) + # verify that self-generation of cert is disabled for instructor-paced course self.assertFalse(certs_api.cert_generation_enabled(self.course.id)) diff --git a/openedx/core/djangoapps/models/course_details.py b/openedx/core/djangoapps/models/course_details.py index 0765fcc518..298a72741f 100644 --- a/openedx/core/djangoapps/models/course_details.py +++ b/openedx/core/djangoapps/models/course_details.py @@ -280,6 +280,10 @@ class CourseDetails(object): if dirty: module_store.update_item(descriptor, user.id) + # fires a signal indicating that the course pacing has changed + if is_pacing_changed: + COURSE_PACING_CHANGE.send(sender=None, course_key=course_key, course_self_paced=descriptor.self_paced) + # NOTE: below auto writes to the db w/o verifying that any of # the fields actually changed to make faster, could compare # against db or could have client send over a list of which @@ -290,10 +294,6 @@ class CourseDetails(object): cls.update_about_video(descriptor, jsondict['intro_video'], user.id) - # fires the signal that course pacing has changed after changes are reflected in db - if is_pacing_changed: - COURSE_PACING_CHANGE.send(sender=None, course_key=course_key, course_self_paced=descriptor.self_paced) - # Could just return jsondict w/o doing any db reads, but I put # the reads in as a means to confirm it persisted correctly return CourseDetails.fetch(course_key)