From cfe5ad4b89509c01e55bb57188145af49c889ab6 Mon Sep 17 00:00:00 2001 From: Simon Chen Date: Tue, 6 Sep 2016 16:16:12 -0400 Subject: [PATCH] ECOM-5402 Delete the drupal node when program is hard deleted --- course_discovery/apps/course_metadata/apps.py | 5 ++ .../apps/course_metadata/publishers.py | 72 +++++++++++-------- .../apps/course_metadata/signals.py | 14 ++++ .../apps/course_metadata/tests/mixins.py | 15 +++- .../apps/course_metadata/tests/test_models.py | 36 ++++++++-- .../course_metadata/tests/test_publishers.py | 52 ++++++++++---- .../course_metadata/tests/test_signals.py | 26 +++++++ 7 files changed, 170 insertions(+), 50 deletions(-) create mode 100644 course_discovery/apps/course_metadata/signals.py create mode 100644 course_discovery/apps/course_metadata/tests/test_signals.py diff --git a/course_discovery/apps/course_metadata/apps.py b/course_discovery/apps/course_metadata/apps.py index d8d4d81e9c..9b79cd41a1 100644 --- a/course_discovery/apps/course_metadata/apps.py +++ b/course_discovery/apps/course_metadata/apps.py @@ -4,3 +4,8 @@ class CourseMetadataConfig(AppConfig): name = 'course_discovery.apps.course_metadata' verbose_name = 'Course Metadata' + + def ready(self): + super(CourseMetadataConfig, self).ready() + # noinspection PyUnresolvedReferences + import course_discovery.apps.course_metadata.signals # pylint: disable=unused-variable diff --git a/course_discovery/apps/course_metadata/publishers.py b/course_discovery/apps/course_metadata/publishers.py index 580a1ed5d7..001eaae8b8 100644 --- a/course_discovery/apps/course_metadata/publishers.py +++ b/course_discovery/apps/course_metadata/publishers.py @@ -89,6 +89,27 @@ def __init__(self, program_before=None): 'title': program_before.title, } + def _get_api_client(self, program): + if not program.partner.has_marketing_site: + return + + if not (program.partner.marketing_site_api_username and program.partner.marketing_site_api_password): + msg = 'Marketing Site API credentials are not properly configured for Partner [{partner}]!'.format( + partner=program.partner.short_code) + raise ProgramPublisherException(msg) + + if self.data_before and \ + all(self.data_before[key] == getattr(program, key) for key in ['title', 'status', 'type']): + # We don't need to publish to marketing site because + # nothing we care about has changed. This would save at least 4 network calls + return + + return MarketingSiteAPIClient( + program.partner.marketing_site_api_username, + program.partner.marketing_site_api_password, + program.partner.marketing_site_url_root + ) + def _get_node_data(self, program, user_id): return { 'type': str(program.type).lower(), @@ -128,34 +149,25 @@ def _create_node(self, api_client, node_data): else: raise ProgramPublisherException("Marketing site page creation failed!") - def publish_program(self, program): - if not program.partner.has_marketing_site: - return - - if not (program.partner.marketing_site_api_username and program.partner.marketing_site_api_password): - msg = 'Marketing Site API credentials are not properly configured for Partner [{partner}]!'.format( - partner=program.partner.short_code) - raise ProgramPublisherException(msg) - - if self.data_before and \ - self.data_before.get('title') == program.title and \ - self.data_before.get('status') == program.status and \ - self.data_before.get('type') == program.type: - # We don't need to publish to marketing site because - # nothing we care about has changed. This would save at least 4 network calls - return - - api_client = MarketingSiteAPIClient( - program.partner.marketing_site_api_username, - program.partner.marketing_site_api_password, - program.partner.marketing_site_url_root - ) + def _delete_node(self, api_client, nid): + node_url = '{root}/node.json/{nid}'.format(root=api_client.api_url, nid=nid) + api_client.api_session.delete(node_url) - node_data = self._get_node_data(program, api_client.user_id) - nid = self._get_node_id(api_client, program.uuid) - if nid: - # We would like to edit the existing node - self._edit_node(api_client, nid, node_data) - else: - # We should create a new node - self._create_node(api_client, node_data) + def publish_program(self, program): + api_client = self._get_api_client(program) + if api_client: + node_data = self._get_node_data(program, api_client.user_id) + nid = self._get_node_id(api_client, program.uuid) + if nid: + # We would like to edit the existing node + self._edit_node(api_client, nid, node_data) + else: + # We should create a new node + self._create_node(api_client, node_data) + + def delete_program(self, program): + api_client = self._get_api_client(program) + if api_client: + nid = self._get_node_id(api_client, program.uuid) + if nid: + self._delete_node(api_client, nid) diff --git a/course_discovery/apps/course_metadata/signals.py b/course_discovery/apps/course_metadata/signals.py new file mode 100644 index 0000000000..f63f14edb9 --- /dev/null +++ b/course_discovery/apps/course_metadata/signals.py @@ -0,0 +1,14 @@ +from django.db.models.signals import pre_delete +from django.dispatch import receiver +import waffle + +from course_discovery.apps.course_metadata.models import Program +from course_discovery.apps.course_metadata.publishers import MarketingSitePublisher + + +@receiver(pre_delete, sender=Program) +def delete_program(sender, instance, **kwargs): # pylint: disable=unused-argument + if waffle.switch_is_active('publish_program_to_marketing_site') and \ + instance.partner.has_marketing_site: + publisher = MarketingSitePublisher() + publisher.delete_program(instance) diff --git a/course_discovery/apps/course_metadata/tests/mixins.py b/course_discovery/apps/course_metadata/tests/mixins.py index 598526e76c..34ff5d0395 100644 --- a/course_discovery/apps/course_metadata/tests/mixins.py +++ b/course_discovery/apps/course_metadata/tests/mixins.py @@ -1,12 +1,13 @@ import json +from django.test import TestCase from factory.fuzzy import FuzzyText, FuzzyInteger import responses from course_discovery.apps.core.tests.utils import FuzzyUrlRoot -class MarketingSiteAPIClientTestMixin(object): +class MarketingSiteAPIClientTestMixin(TestCase): """ The mixing to help mock the responses for marketing site API Client """ @@ -70,6 +71,9 @@ def mock_user_id_response(self, status): match_querystring=True ) + def assert_responses_call_count(self, count): + self.assertEqual(len(responses.calls), count) + class MarketingSitePublisherTestMixin(MarketingSiteAPIClientTestMixin): """ @@ -121,3 +125,12 @@ def mock_node_create(self, response_data, status): content_type='application/json', status=status ) + + def mock_node_delete(self, status): + responses.add( + responses.DELETE, + '{root}/node.json/{nid}'.format(root=self.api_root, nid=self.nid), + body='', + content_type='text/html', + status=status + ) diff --git a/course_discovery/apps/course_metadata/tests/test_models.py b/course_discovery/apps/course_metadata/tests/test_models.py index 081d6e0f6e..f3b3093c5b 100644 --- a/course_discovery/apps/course_metadata/tests/test_models.py +++ b/course_discovery/apps/course_metadata/tests/test_models.py @@ -268,7 +268,7 @@ class TestAbstractValueModel(AbstractValueModel): @ddt.ddt -class ProgramTests(MarketingSitePublisherTestMixin, TestCase): +class ProgramTests(MarketingSitePublisherTestMixin): """Tests of the Program model.""" def setUp(self): @@ -395,7 +395,12 @@ def test_is_active(self, status): def test_save_without_publish(self): self.program.title = FuzzyText().fuzz() self.program.save() - self.assertEqual(len(responses.calls), 0) + self.assert_responses_call_count(0) + + @responses.activate + def test_delete_without_publish(self): + self.program.delete() + self.assert_responses_call_count(0) @responses.activate def test_save_and_publish_success(self): @@ -409,8 +414,7 @@ def test_save_and_publish_success(self): toggle_switch('publish_program_to_marketing_site', True) self.program.title = FuzzyText().fuzz() self.program.save() - self.assertEqual(len(responses.calls), 6) - toggle_switch('publish_program_to_marketing_site', False) + self.assert_responses_call_count(6) @responses.activate def test_save_and_no_marketing_site(self): @@ -419,8 +423,28 @@ def test_save_and_no_marketing_site(self): toggle_switch('publish_program_to_marketing_site', True) self.program.title = FuzzyText().fuzz() self.program.save() - self.assertEqual(len(responses.calls), 0) - toggle_switch('publish_program_to_marketing_site', False) + self.assert_responses_call_count(0) + + @responses.activate + def test_delete_and_publish_success(self): + self.program.partner.marketing_site_url_root = self.api_root + self.program.partner.marketing_site_api_username = self.username + self.program.partner.marketing_site_api_password = self.password + self.program.save() + self.mock_api_client(200) + self.mock_node_retrieval(self.program.uuid) + self.mock_node_delete(204) + toggle_switch('publish_program_to_marketing_site', True) + self.program.delete() + self.assert_responses_call_count(5) + + @responses.activate + def test_delete_and_no_marketing_site(self): + self.program.partner.marketing_site_url_root = None + self.program.save() + toggle_switch('publish_program_to_marketing_site', True) + self.program.delete() + self.assert_responses_call_count(0) def test_course_update_caught_exception(self): """ Test that the index update process failing will not cause the program save to error """ diff --git a/course_discovery/apps/course_metadata/tests/test_publishers.py b/course_discovery/apps/course_metadata/tests/test_publishers.py index 195db0d55a..7802f0af90 100644 --- a/course_discovery/apps/course_metadata/tests/test_publishers.py +++ b/course_discovery/apps/course_metadata/tests/test_publishers.py @@ -1,4 +1,3 @@ -from django.test import TestCase import responses from course_discovery.apps.course_metadata.publishers import ( @@ -14,7 +13,7 @@ from course_discovery.apps.course_metadata.models import Program -class MarketingSiteAPIClientTests(MarketingSiteAPIClientTestMixin, TestCase): +class MarketingSiteAPIClientTests(MarketingSiteAPIClientTestMixin): """ Unit test cases for MarketinSiteAPIClient """ @@ -30,7 +29,7 @@ def setUp(self): def test_init_session(self): self.mock_login_response(200) session = self.api_client.init_session - self.assertEqual(len(responses.calls), 2) + self.assert_responses_call_count(2) self.assertIsNotNone(session) @responses.activate @@ -44,7 +43,7 @@ def test_csrf_token(self): self.mock_login_response(200) self.mock_csrf_token_response(200) csrf_token = self.api_client.csrf_token - self.assertEqual(len(responses.calls), 3) + self.assert_responses_call_count(3) self.assertEqual(self.csrf_token, csrf_token) @responses.activate @@ -59,7 +58,7 @@ def test_user_id(self): self.mock_login_response(200) self.mock_user_id_response(200) user_id = self.api_client.user_id - self.assertEqual(len(responses.calls), 3) + self.assert_responses_call_count(3) self.assertEqual(self.user_id, user_id) @responses.activate @@ -74,7 +73,7 @@ def test_api_session(self): self.mock_login_response(200) self.mock_csrf_token_response(200) api_session = self.api_client.api_session - self.assertEqual(len(responses.calls), 3) + self.assert_responses_call_count(3) self.assertIsNotNone(api_session) self.assertEqual(api_session.headers.get('Content-Type'), 'application/json') self.assertEqual(api_session.headers.get('X-CSRF-Token'), self.csrf_token) @@ -87,7 +86,7 @@ def test_api_session_failed(self): self.api_client.api_session # pylint: disable=pointless-statement -class MarketingSitePublisherTests(MarketingSitePublisherTestMixin, TestCase): +class MarketingSitePublisherTests(MarketingSitePublisherTestMixin): """ Unit test cases for the MarketingSitePublisher """ @@ -125,7 +124,7 @@ def test_get_node_id(self): self.mock_node_retrieval(self.program.uuid) publisher = MarketingSitePublisher() node_id = publisher._get_node_id(self.api_client, self.program.uuid) # pylint: disable=protected-access - self.assertEqual(len(responses.calls), 4) + self.assert_responses_call_count(4) self.assertEqual(node_id, self.nid) @responses.activate @@ -143,7 +142,7 @@ def test_edit_node(self): publisher = MarketingSitePublisher() publish_data = publisher._get_node_data(self.program, self.user_id) # pylint: disable=protected-access publisher._edit_node(self.api_client, self.nid, publish_data) # pylint: disable=protected-access - self.assertEqual(len(responses.calls), 4) + self.assert_responses_call_count(4) @responses.activate def test_edit_node_failed(self): @@ -189,7 +188,7 @@ def test_publish_program_create(self): self.mock_node_create(expected, 201) publisher = MarketingSitePublisher() publisher.publish_program(self.program) - self.assertEqual(len(responses.calls), 6) + self.assert_responses_call_count(6) @responses.activate def test_publish_program_edit(self): @@ -198,7 +197,7 @@ def test_publish_program_edit(self): self.mock_node_edit(200) publisher = MarketingSitePublisher() publisher.publish_program(self.program) - self.assertEqual(len(responses.calls), 6) + self.assert_responses_call_count(6) @responses.activate def test_publish_modified_program(self): @@ -208,11 +207,38 @@ def test_publish_modified_program(self): program_before = ProgramFactory() publisher = MarketingSitePublisher(program_before) publisher.publish_program(self.program) - self.assertEqual(len(responses.calls), 6) + self.assert_responses_call_count(6) @responses.activate def test_publish_unmodified_program(self): self.mock_api_client(200) publisher = MarketingSitePublisher(self.program) publisher.publish_program(self.program) - self.assertEqual(len(responses.calls), 0) + self.assert_responses_call_count(0) + + @responses.activate + def test_publish_program_no_credential(self): + self.program.partner.marketing_site_api_password = None + self.program.partner.marketing_site_api_username = None + self.program.save() # pylint: disable=no-member + publisher = MarketingSitePublisher() + with self.assertRaises(ProgramPublisherException): + publisher.publish_program(self.program) + self.assert_responses_call_count(0) + + @responses.activate + def test_publish_delete_program(self): + self.mock_api_client(200) + self.mock_node_retrieval(self.program.uuid) + self.mock_node_delete(204) + publisher = MarketingSitePublisher() + publisher.delete_program(self.program) + self.assert_responses_call_count(5) + + @responses.activate + def test_publish_delete_non_existent_program(self): + self.mock_api_client(200) + self.mock_node_retrieval(self.program.uuid, exists=False) + publisher = MarketingSitePublisher() + publisher.delete_program(self.program) + self.assert_responses_call_count(4) diff --git a/course_discovery/apps/course_metadata/tests/test_signals.py b/course_discovery/apps/course_metadata/tests/test_signals.py new file mode 100644 index 0000000000..334b24e865 --- /dev/null +++ b/course_discovery/apps/course_metadata/tests/test_signals.py @@ -0,0 +1,26 @@ +# pylint: disable=no-member +from unittest.mock import patch + +from django.test import TestCase + +from course_discovery.apps.course_metadata.tests import factories, toggle_switch + + +MARKETING_SITE_PUBLISHERS_MODULE = 'course_discovery.apps.course_metadata.publishers.MarketingSitePublisher' + + +@patch(MARKETING_SITE_PUBLISHERS_MODULE + '.delete_program') +class SignalsTest(TestCase): + def setUp(self): + super(SignalsTest, self).setUp() + self.program = factories.ProgramFactory() + + def test_delete_program_signal_no_publish(self, delete_program_mock): + toggle_switch('publish_program_to_marketing_site', False) + self.program.delete() + self.assertFalse(delete_program_mock.called) + + def test_delete_program_signal_with_publish(self, delete_program_mock): + toggle_switch('publish_program_to_marketing_site', True) + self.program.delete() + delete_program_mock.assert_called_once_with(self.program)