Lorem ipsum dolor sit amet, consectetur adipisicing elit. Veniam laboriosam consequatur saepe. Repellat itaque dolores neque, impedit reprehenderit eum culpa voluptates harum sapiente nesciunt ratione.
", - "points": 95, - "slug": "red-its-new-blue", - }, - } + + def get_fn(record_id): + if record_id == "recNewRecordId": + return { + "id": "recNewRecordId", + "fields": { + "title": "Red! It's the new blue!", + "description": "Red is a scientifically proven color that moves faster than all other colors.", + "external_link": "https://example.com/", + "is_active": True, + "rating": "1.5", + "long_description": "Lorem ipsum dolor sit amet, consectetur adipisicing elit. Veniam laboriosam consequatur saepe. Repellat itaque dolores neque, impedit reprehenderit eum culpa voluptates harum sapiente nesciunt ratione.
", + "points": 95, + "slug": "red-its-new-blue", + }, + } + else: + raise HTTPError("404 Client Error: Not Found") + + MockAirtable.get.side_effect = get_fn MockAirtable.insert = mock.MagicMock("insert") + MockAirtable.insert.return_value = { "id": "recNewRecordId", "fields": { @@ -62,34 +71,67 @@ def get_iter(self): MockAirtable.delete.return_value = {"deleted": True, "record": "recNewRecordId"} MockAirtable.search = mock.MagicMock("search") - MockAirtable.search.return_value = [ - { - "id": "recNewRecordId", - "fields": { - "title": "Red! It's the new blue!", - "description": "Red is a scientifically proven color that moves faster than all other colors.", - "external_link": "https://example.com/", - "is_active": True, - "rating": "1.5", - "long_description": "Lorem ipsum dolor sit amet, consectetur adipisicing elit. Veniam laboriosam consequatur saepe. Repellat itaque dolores neque, impedit reprehenderit eum culpa voluptates harum sapiente nesciunt ratione.
", - "points": 95, - "slug": "red-its-new-blue", - }, - }, - { - "id": "Different record", - "fields": { - "title": "Not the used record.", - "description": "This is only used for multiple responses from MockAirtable", - "external_link": "https://example.com/", - "is_active": False, - "rating": "5.5", - "long_description": "", - "points": 1, - "slug": "not-the-used-record", - }, - }, - ] + def search_fn(field, value): + if field == "slug" and value == "red-its-new-blue": + return [ + { + "id": "recNewRecordId", + "fields": { + "title": "Red! It's the new blue!", + "description": "Red is a scientifically proven color that moves faster than all other colors.", + "external_link": "https://example.com/", + "is_active": True, + "rating": "1.5", + "long_description": "Lorem ipsum dolor sit amet, consectetur adipisicing elit. Veniam laboriosam consequatur saepe. Repellat itaque dolores neque, impedit reprehenderit eum culpa voluptates harum sapiente nesciunt ratione.
", + "points": 95, + "slug": "red-its-new-blue", + }, + }, + { + "id": "Different record", + "fields": { + "title": "Not the used record.", + "description": "This is only used for multiple responses from MockAirtable", + "external_link": "https://example.com/", + "is_active": False, + "rating": "5.5", + "long_description": "", + "points": 1, + "slug": "not-the-used-record", + }, + }, + ] + elif field == "slug" and value == "a-matching-slug": + return [ + { + "id": "recMatchedRecordId", + "fields": { + "title": "Red! It's the new blue!", + "description": "Red is a scientifically proven color that moves faster than all other colors.", + "external_link": "https://example.com/", + "is_active": True, + "rating": "1.5", + "long_description": "Lorem ipsum dolor sit amet, consectetur adipisicing elit. Veniam laboriosam consequatur saepe. Repellat itaque dolores neque, impedit reprehenderit eum culpa voluptates harum sapiente nesciunt ratione.
", + "points": 95, + "slug": "a-matching-slug", + }, + }, + ] + elif field == "Page Slug" and value == "home": + return [ + { + "id": "recHomePageId", + "fields": { + "title": "Home", + "Page Slug": "home", + "intro": "This is the home page.", + }, + }, + ] + else: + return [] + + MockAirtable.search.side_effect = search_fn MockAirtable.get_all = mock.MagicMock("get_all") MockAirtable.get_all.return_value = [ diff --git a/tests/models.py b/tests/models.py index b1e4826..45fa804 100644 --- a/tests/models.py +++ b/tests/models.py @@ -18,7 +18,7 @@ class SimplePage(AirtableMixin, Page): def map_import_fields(cls): mappings = { "title": "title", - "slug": "slug", + "Page Slug": "slug", "intro": "intro", } return mappings @@ -26,7 +26,7 @@ def map_import_fields(cls): def get_export_fields(self): return { "title": self.title, - "slug": self.slug, + "Page Slug": self.slug, "intro": self.intro, } diff --git a/tests/settings.py b/tests/settings.py index 72f4e8b..d42f452 100644 --- a/tests/settings.py +++ b/tests/settings.py @@ -130,7 +130,7 @@ 'tests.SimplePage': { 'AIRTABLE_BASE_KEY': 'xxx', 'AIRTABLE_TABLE_NAME': 'xxx', - 'AIRTABLE_UNIQUE_IDENTIFIER': 'slug', + 'AIRTABLE_UNIQUE_IDENTIFIER': {'Page Slug': 'slug'}, 'AIRTABLE_SERIALIZER': 'tests.serializers.SimplePageSerializer', 'PARENT_PAGE_ID': 'tests.models.get_import_parent_page', }, diff --git a/tests/test_import.py b/tests/test_import.py index 35d05b2..1026702 100644 --- a/tests/test_import.py +++ b/tests/test_import.py @@ -200,7 +200,7 @@ def test_create_page(self, mixin_airtable): "id": "test-created-page-id", "fields": { "title": "A simple page", - "slug": "a-simple-page", + "Page Slug": "a-simple-page", "intro": "How much more simple can it get? And the answer is none. None more simple.", }, }] @@ -230,7 +230,7 @@ def test_create_and_publish_page(self, mixin_airtable): "id": "test-created-page-id", "fields": { "title": "A simple page", - "slug": "a-simple-page", + "Page Slug": "a-simple-page", "intro": "How much more simple can it get? And the answer is none. None more simple.", }, }] @@ -265,7 +265,7 @@ def test_update_page(self, mixin_airtable): "id": "test-created-page-id", "fields": { "title": "A simple page", - "slug": "a-simple-page", + "Page Slug": "a-simple-page", "intro": "How much more simple can it get? Oh, actually it can get more simple.", }, }] @@ -299,7 +299,7 @@ def test_skip_update_page_if_unchanged(self, mixin_airtable): "id": "test-created-page-id", "fields": { "title": "A simple page", - "slug": "a-simple-page", + "Page Slug": "a-simple-page", "intro": "How much more simple can it get? And the answer is none. None more simple.", }, }] @@ -330,7 +330,7 @@ def test_skip_update_page_if_locked(self, mixin_airtable): "id": "test-created-page-id", "fields": { "title": "A simple page", - "slug": "a-simple-page", + "Page Slug": "a-simple-page", "intro": "How much more simple can it get? Oh, actually it can get more simple.", }, }] diff --git a/tests/test_models.py b/tests/test_models.py index 654e72c..9a962d1 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -2,9 +2,9 @@ from django.test import TestCase -from tests.models import Advert +from tests.models import Advert, SimplePage from wagtail_airtable.mixins import AirtableMixin -from unittest.mock import patch +from unittest.mock import ANY, patch from .mock_airtable import get_mock_airtable @@ -75,33 +75,97 @@ def test_create_object_from_url(self): self.assertTrue(advert._push_to_airtable) self.assertTrue(hasattr(advert, 'airtable_client')) - def test_edit_object(self): + def test_create_object_with_existing_airtable_record_id(self): advert = Advert.objects.create( - title='Throw away advert', + title='Testing creation', description='Lorem ipsum dolor sit amet, consectetur adipisicing elit.', rating="2.5", - slug='edit-advert', - airtable_record_id='recCustomEditId', + slug='testing-creation', + airtable_record_id='recNewRecordId', ) - # Get the new advert without the instantiated airtable properties and api client - advert = Advert.objects.get(slug='edit-advert') - self.assertFalse(advert._ran_airtable_setup) - self.assertFalse(advert._is_enabled) - self.assertFalse(advert._push_to_airtable) + # save_to_airtable will confirm that a record with the given ID exists + # and update that record + advert.airtable_client.get.assert_called_once_with('recNewRecordId') + advert.airtable_client.update.assert_called_once_with('recNewRecordId', ANY) + call_args = advert.airtable_client.update.call_args.args + self.assertEqual(call_args[1]['title'], 'Testing creation') + advert.airtable_client.insert.assert_not_called() + + def test_create_object_with_missing_id_and_matching_airtable_record(self): + advert = Advert.objects.create( + title='Testing creation', + description='Lorem ipsum dolor sit amet, consectetur adipisicing elit.', + rating="2.5", + slug='a-matching-slug', + airtable_record_id='recMissingRecordId', + ) + # save_to_airtable will find that a record with the given ID does not exist, + # but one matching the slug does, and update that record + advert.airtable_client.get.assert_called_once_with('recMissingRecordId') + advert.airtable_client.search.assert_called_once_with('slug', 'a-matching-slug') + advert.airtable_client.update.assert_called_once_with('recMatchedRecordId', ANY) + call_args = advert.airtable_client.update.call_args.args + self.assertEqual(call_args[1]['title'], 'Testing creation') + advert.airtable_client.insert.assert_not_called() + advert.refresh_from_db() + self.assertEqual(advert.airtable_record_id, 'recMatchedRecordId') + + def test_create_object_with_no_id_and_matching_airtable_record(self): + advert = Advert.objects.create( + title='Testing creation', + description='Lorem ipsum dolor sit amet, consectetur adipisicing elit.', + rating="2.5", + slug='a-matching-slug', + ) + # save_to_airtable will skip the lookup by ID, but find a record matching the slug, + # and update that record + advert.airtable_client.get.assert_not_called() + advert.airtable_client.search.assert_called_once_with('slug', 'a-matching-slug') + advert.airtable_client.update.assert_called_once_with('recMatchedRecordId', ANY) + call_args = advert.airtable_client.update.call_args.args + self.assertEqual(call_args[1]['title'], 'Testing creation') + advert.airtable_client.insert.assert_not_called() + advert.refresh_from_db() + self.assertEqual(advert.airtable_record_id, 'recMatchedRecordId') + + def test_create_object_with_missing_id_and_non_matching_airtable_record(self): + advert = Advert.objects.create( + title='Testing creation', + description='Lorem ipsum dolor sit amet, consectetur adipisicing elit.', + rating="2.5", + slug='a-non-matching-slug', + airtable_record_id='recMissingRecordId', + ) + # save_to_airtable will find that a record with the given ID does not exist, + # and neither does one matching the slug - so it will create a new one + # and update the model with the new record ID + advert.airtable_client.get.assert_called_once_with('recMissingRecordId') + advert.airtable_client.search.assert_called_once_with('slug', 'a-non-matching-slug') + advert.airtable_client.insert.assert_called_once() + call_args = advert.airtable_client.insert.call_args.args + self.assertEqual(call_args[0]['title'], 'Testing creation') + advert.airtable_client.update.assert_not_called() + advert.refresh_from_db() + self.assertEqual(advert.airtable_record_id, 'recNewRecordId') + + def test_edit_object(self): + advert = Advert.objects.get(airtable_record_id='recNewRecordId') advert.title = "Edited title" advert.description = "Edited description" advert.save() - advert.airtable_client.update.assert_called() - self.assertTrue(advert._ran_airtable_setup) - self.assertTrue(advert._is_enabled) - self.assertTrue(advert._push_to_airtable) + # save_to_airtable will confirm that a record with the given ID exists and update it + advert.airtable_client.get.assert_called_once_with('recNewRecordId') + advert.airtable_client.update.assert_called_once_with('recNewRecordId', ANY) + call_args = advert.airtable_client.update.call_args.args + advert.airtable_client.insert.assert_not_called() + self.assertEqual(call_args[1]['title'], 'Edited title') self.assertEqual(advert.title, "Edited title") def test_delete_object(self): advert = Advert.objects.get(slug='delete-me') - # If we werent mocking the Airtable.update() method, we'd assert advert.airtable_client.insert self.assertEqual(advert.airtable_record_id, 'recNewRecordId') advert.delete() + advert.airtable_client.delete.assert_called_once_with('recNewRecordId') find_deleted_advert = Advert.objects.filter(slug='delete-me').count() self.assertEqual(find_deleted_advert, 0) @@ -114,13 +178,11 @@ def setUp(self): self.mock_airtable = airtable_patcher.start() self.addCleanup(airtable_patcher.stop) - self.advert = Advert.objects.first() - def test_setup_airtable(self): - advert = copy(self.advert) - self.assertEqual(advert._ran_airtable_setup, False) - self.assertEqual(advert._is_enabled, False) - self.assertEqual(advert._push_to_airtable, False) + advert = copy(Advert.objects.first()) + self.assertFalse(advert._ran_airtable_setup) + self.assertFalse(advert._is_enabled) + self.assertFalse(advert._push_to_airtable) self.assertFalse(hasattr(advert, 'airtable_client')) advert.setup_airtable() @@ -128,38 +190,17 @@ def test_setup_airtable(self): self.assertEqual(advert.AIRTABLE_BASE_KEY, 'app_airtable_advert_base_key') self.assertEqual(advert.AIRTABLE_TABLE_NAME, 'Advert Table Name') self.assertEqual(advert.AIRTABLE_UNIQUE_IDENTIFIER, 'slug') - self.assertEqual(advert._ran_airtable_setup, True) - self.assertEqual(advert._is_enabled, True) - self.assertEqual(advert._push_to_airtable, True) + self.assertTrue(advert._ran_airtable_setup) + self.assertTrue(advert._is_enabled) + self.assertTrue(advert._push_to_airtable) self.assertTrue(hasattr(advert, 'airtable_client')) - def test_create_and_attach_airtable_record(self): - advert = copy(self.advert) - self.assertEqual(advert.airtable_record_id, '') - - advert.setup_airtable() - advert.save() - - advert.airtable_client.update.assert_called() - advert.airtable_client.insert.assert_not_called() - self.assertEqual(advert.airtable_record_id, 'recNewRecordId') - - def test_update_record(self): - advert = Advert.objects.first() - advert.setup_airtable() - self.assertEqual(advert.airtable_record_id, '') - record = advert.update_record('fake record id') - advert.airtable_client.update.assert_called() - advert.airtable_client.insert.assert_not_called() - self.assertEqual(record['id'], 'recNewRecordId') - self.assertEqual(advert.airtable_record_id, 'recNewRecordId') - def test_delete_record(self): - advert = copy(self.advert) + advert = Advert.objects.get(airtable_record_id='recNewRecordId') advert.setup_airtable() deleted = advert.delete_record() self.assertTrue(deleted) - advert.airtable_client.delete.assert_called() + advert.airtable_client.delete.assert_called_once_with("recNewRecordId") def test_parse_request_error(self): error_401 = "401 Client Error: Unauthorized for url: https://api.airtable.com/v0/appYourAppId/Your%20Table?filterByFormula=.... [Error: {'type': 'AUTHENTICATION_REQUIRED', 'message': 'Authentication required'}]" @@ -181,50 +222,22 @@ def test_parse_request_error(self): self.assertEqual(parsed_error['message'], 'Could not find table table_name in appxxxxx') def test_match_record(self): - advert = copy(self.advert) + advert = Advert.objects.get(slug='red-its-new-blue') advert.setup_airtable() record_id = advert.match_record() self.assertEqual(record_id, 'recNewRecordId') - advert.airtable_client.search.assert_called() + advert.airtable_client.search.assert_called_once_with('slug', 'red-its-new-blue') + + def test_match_record_with_dict_identifier(self): + page = SimplePage.objects.get(slug='home') + page.setup_airtable() + record_id = page.match_record() + self.assertEqual(record_id, 'recHomePageId') + page.airtable_client.search.assert_called_once_with('Page Slug', 'home') def test_check_record_exists(self): - advert = copy(self.advert) + advert = Advert.objects.get(airtable_record_id='recNewRecordId') advert.setup_airtable() record_exists = advert.check_record_exists('recNewRecordId') self.assertTrue(record_exists) - advert.airtable_client.get.assert_called() - - def test_is_airtable_enabled(self): - advert = copy(self.advert) - self.assertFalse(advert._ran_airtable_setup) - self.assertFalse(advert._is_enabled) - advert.setup_airtable() - self.assertTrue(advert._ran_airtable_setup) - self.assertTrue(advert._is_enabled) - - def test_save(self): - advert = copy(self.advert) - self.assertFalse(advert._ran_airtable_setup) - self.assertFalse(advert._push_to_airtable) - self.assertFalse(advert._is_enabled) - self.assertEqual(advert.airtable_record_id, '') - - advert.setup_airtable() - self.assertTrue(advert._ran_airtable_setup) - self.assertTrue(advert._push_to_airtable) - self.assertTrue(advert._is_enabled) - - self.assertEqual(advert.airtable_record_id, '') - - advert.save() - self.assertEqual(advert.airtable_record_id, 'recNewRecordId') - advert.airtable_client.update.assert_called() - advert.airtable_client.insert.assert_not_called() - - def test_delete(self): - advert = copy(self.advert) - advert.setup_airtable() - - deleted = advert.delete_record() - self.assertTrue(deleted) - advert.airtable_client.delete.assert_called() + advert.airtable_client.get.assert_called_once_with('recNewRecordId') diff --git a/wagtail_airtable/mixins.py b/wagtail_airtable/mixins.py index 82a603b..dd2a913 100644 --- a/wagtail_airtable/mixins.py +++ b/wagtail_airtable/mixins.py @@ -132,30 +132,6 @@ def get_export_fields(self): def mapped_export_fields(self): return self.get_export_fields() - def create_record(self) -> dict: - """ - Create or update a record. - - The create_record() method will look for an Airtable match before trying - to create a new Airtable record (that comes with a new airtable_record_id). - - This function needs to check for a matched record in Airtable first just in case - some data became out of sync, or one person worked in Airtable and one worked in - Wagtail. The idea here is to marry those records whenever possible instead of - duplicating Airtable records. - - If a record in Airtable exists, update this object with the found record_id. (Prevent record duplication) - But if a record is NOT found in Airtable, create a new record. - """ - matched_record = self.match_record() - if matched_record: - record = self.update_record(matched_record) - else: - record = self.airtable_client.insert(self.mapped_export_fields) - - self.airtable_record_id = record["id"] - return record - def check_record_exists(self, airtable_record_id) -> bool: """ Check if a record exists in an Airtable by its exact Airtable Record ID. @@ -169,28 +145,6 @@ def check_record_exists(self, airtable_record_id) -> bool: record = {} return bool(record) - def update_record(self, airtable_record_id=None): - """ - Update a record. - - Before updating a record this will check to see if a record even exists - in Airtable. If a record is not found using its Airtable record_id it cannot - be updated and may throw an unexpected error. - - If a record DOES exist based on its Airtable record_id, we can update that particular row. - If a record does NOT exist in Airtable, a new record will need to be created. - """ - airtable_record_id = airtable_record_id or self.airtable_record_id - if self.check_record_exists(airtable_record_id): - # Record exists in Airtable - record = self.airtable_client.update(airtable_record_id, self.mapped_export_fields) - else: - # No record exists in Airtable. Create a new record now. - record = self.create_record() - - self.airtable_record_id = record["id"] - return record - def delete_record(self) -> bool: """ Deletes a record from Airtable, but does not delete the object from Django. @@ -294,54 +248,67 @@ def parse_request_error(cls, error): "message": error_info["message"], } - def save_to_airtable(self, *args, **kwargs): + def _update_record(self, record_id, fields): + try: + self.airtable_client.update(record_id, fields) + except HTTPError as e: + error = self.parse_request_error(e.args[0]) + message = ( + f"Could not update Airtable record. Reason: {error['message']}" + ) + logger.warning(message) + # Used in the `after_edit_page` hook. If it exists, an error message will be displayed. + self._airtable_update_error = message + return False + return True + + def _create_record(self, fields): + try: + record = self.airtable_client.insert(fields) + except HTTPError as e: + error = self.parse_request_error(e.args[0]) + message = ( + f"Could not create Airtable record. Reason: {error['message']}" + ) + logger.warning(message) + # Used in the `after_edit_page` hook. If it exists, an error message will be displayed. + self._airtable_update_error = message + return None + return record["id"] + + def save_to_airtable(self): """ If there's an existing airtable record id, update the row. Otherwise attempt to create a new record. """ - - # Save to database first so we get pk, in case it's used for uniqueness - saved_model = super().save(*args, **kwargs) self.setup_airtable() if self._push_to_airtable and self.push_to_airtable: # Every airtable model needs mapped fields. # mapped_export_fields is a cached property. Delete the cached prop and get new values upon save. self.refresh_mapped_export_fields() - if self.airtable_record_id: + if self.airtable_record_id and self.check_record_exists(self.airtable_record_id): # If this model has an airtable_record_id, attempt to update the record. - try: - self.update_record() - except HTTPError as e: - error = self.parse_request_error(e.args[0]) - message = ( - f"Could not update Airtable record. Reason: {error['message']}" - ) - logger.warning(message) - # Used in the `after_edit_page` hook. If it exists, an error message will be displayed. - self._airtable_update_error = message + self._update_record(self.airtable_record_id, self.mapped_export_fields) else: - # Creating a record will also search for an existing field match - # ie. Looks for a matching `slug` in Airtable and Wagtail/Django - try: - self.create_record() - # Save once more so the airtable_record_id is stored. - super().save(*args, **kwargs) - except HTTPError as e: - error = self.parse_request_error(e.args[0]) - message = ( - f"Could not create Airtable record. Reason: {error['message']}" - ) - logger.warning(message) - # Used in the `after_edit_page` hook. If it exists, an error message will be displayed. - self._airtable_update_error = message - return saved_model + record_id = self.match_record() + if record_id: + # A match was found by unique identifier. Update the record. + success = self._update_record(record_id, self.mapped_export_fields) + else: + record_id = self._create_record(self.mapped_export_fields) + success = bool(record_id) + + if success: + self.airtable_record_id = record_id + super().save(update_fields=["airtable_record_id"]) def save(self, *args, **kwargs): + # Save to database first so we get pk, in case it's used for uniqueness + super().save(*args, **kwargs) + if getattr(settings, "WAGTAIL_AIRTABLE_SAVE_SYNC", True): # If WAGTAIL_AIRTABLE_SAVE_SYNC is set to True we do it the synchronous way - return self.save_to_airtable(*args, **kwargs) - - return super().save(*args, **kwargs) + self.save_to_airtable() def delete(self, *args, **kwargs): self.setup_airtable()