Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix non uid lookup for pages #138

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions tests/settings.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
from django.core.signals import setting_changed

BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
from wagtail import VERSION as WAGTAIL_VERSION
Expand Down Expand Up @@ -150,6 +151,15 @@
'tests.category': ['name']
}

# some settings are modified in tests, in which case this caching interfers with the ability to test
# effectivley. Since settings are unlikely to change in the middle of usage in the real world,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: "interferes" and "effectively" typos

# it makes sense to clear these just inside our test settings
def clear_locator_cache(setting, value, **kwargs):
from wagtail_transfer.locators import get_locator_for_model
get_locator_for_model.cache_clear()

setting_changed.connect(clear_locator_cache)

# The default name for the Page -> Comment relation from Wagtail 2.15 onward. Setting this ensures that
# 2.13.x (from 2.13.5 onward) and 2.14.x (from 2.14.2 onward) adopt the 2.15 behaviour, allowing us to
# use the same test fixtures across all versions.
Expand Down
22 changes: 22 additions & 0 deletions tests/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,28 @@ def test_related_model_with_field_lookup(self):

# Category objects in the mappings section should be identified by name, not UUID
self.assertIn(['tests.category', 1, ['Cars']], mappings)

@override_settings(WAGTAILTRANSFER_LOOKUP_FIELDS = {'tests.category': ['name'],'wagtailcore.page': ['url_path'],})
def test_get_page_with_field_lookup(self):
response = self.get(5)
self.assertEqual(response.status_code, 200)
data = response.json()
mappings = data['mappings']

# Page objects in the mappings section should be identified by url_path
self.assertIn(['wagtailcore.page', 5, ['/home/oil-is-great/']], mappings)

@override_settings(WAGTAILTRANSFER_LOOKUP_FIELDS = {'tests.category': ['name'],'wagtailcore.page': ['slug', 'locale_id'],})
def test_get_page_with_field_lookup_multiple(self):
response = self.get(5)
self.assertEqual(response.status_code, 200)
data = response.json()
mappings = data['mappings']

# Page objects in the mappings section should be identified by url_path
self.assertIn(['wagtailcore.page', 5, ['oil-is-great', 1]], mappings)




class TestObjectsApi(TestCase):
Expand Down
132 changes: 132 additions & 0 deletions tests/tests/test_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,138 @@ def test_import_page_with_new_list_block_format(self):

self.assertEqual(imported_streamfield, [{'type': 'list_of_captioned_pages', 'value': [{'type': 'item', 'value': {'page': 1, 'text': 'a caption'}, 'id': '8c0d7de7-4f77-4477-be67-7d990d0bfb82'}], 'id': '21ffe52a-c0fc-4ecc-92f1-17b356c9cc94'}])

@override_settings(WAGTAILTRANSFER_LOOKUP_FIELDS = {'tests.category': ['name'],'wagtailcore.page': ['url_path'],})
def test_import_pages_via_lookup(self):
# make a draft edit to the homepage
home = SimplePage.objects.get(slug='home')
home.title = "Draft home"
home.save_revision()
data = """{
"ids_for_import": [
["wagtailcore.page", 12],
["wagtailcore.page", 15]
],
"mappings": [
["wagtailcore.page", 12, "/home/"],
["wagtailcore.page", 15, "/home/imported-child-page"]
],
"objects": [
{
"model": "tests.simplepage",
"pk": 15,
"parent_id": 12,
"fields": {
"title": "Imported child page",
"show_in_menus": false,
"live": true,
"slug": "imported-child-page",
"intro": "This page is imported from the source site",
"wagtail_admin_comments": []
}
},
{
"model": "tests.simplepage",
"pk": 12,
"parent_id": 1,
"fields": {
"title": "New home",
"show_in_menus": false,
"live": true,
"slug": "home",
"intro": "This is the updated homepage",
"wagtail_admin_comments": []
}
}
]
}"""

importer = ImportPlanner(root_page_source_pk=12, destination_parent_id=None)
importer.add_json(data)
importer.run()

updated_page = SimplePage.objects.get(url_path='/home/')
self.assertEqual(updated_page.intro, "This is the updated homepage")
self.assertEqual(updated_page.title, "New home")
self.assertEqual(updated_page.draft_title, "New home")

# get_latest_revision (as used in the edit-page view) should also reflect the imported content
updated_page_revision = updated_page.get_latest_revision_as_page()
self.assertEqual(updated_page_revision.intro, "This is the updated homepage")
self.assertEqual(updated_page_revision.title, "New home")

created_page = SimplePage.objects.get(url_path='/home/imported-child-page/')
self.assertEqual(created_page.intro, "This page is imported from the source site")
# An initial page revision should also be created
self.assertTrue(created_page.get_latest_revision())
created_page_revision = created_page.get_latest_revision_as_page()
self.assertEqual(created_page_revision.intro, "This page is imported from the source site")

@override_settings(WAGTAILTRANSFER_LOOKUP_FIELDS = {'tests.category': ['name'],'wagtailcore.page': ['slug', 'locale_id'],})
def test_import_pages_via_lookup_multiple(self):
# make a draft edit to the homepage
home = SimplePage.objects.get(slug='home')
home.title = "Draft home"
home.save_revision()
data = """{
"ids_for_import": [
["wagtailcore.page", 12],
["wagtailcore.page", 15]
],
"mappings": [
["wagtailcore.page", 12, ["home", 1]],
["wagtailcore.page", 15, ["imported-child-page",1]]
],
"objects": [
{
"model": "tests.simplepage",
"pk": 15,
"parent_id": 12,
"fields": {
"title": "Imported child page",
"show_in_menus": false,
"live": true,
"slug": "imported-child-page",
"intro": "This page is imported from the source site",
"wagtail_admin_comments": []
}
},
{
"model": "tests.simplepage",
"pk": 12,
"parent_id": 1,
"fields": {
"title": "New home",
"show_in_menus": false,
"live": true,
"slug": "home",
"intro": "This is the updated homepage",
"wagtail_admin_comments": []
}
}
]
}"""

importer = ImportPlanner(root_page_source_pk=12, destination_parent_id=None)
importer.add_json(data)
importer.run()

updated_page = SimplePage.objects.get(url_path='/home/')
self.assertEqual(updated_page.intro, "This is the updated homepage")
self.assertEqual(updated_page.title, "New home")
self.assertEqual(updated_page.draft_title, "New home")

# get_latest_revision (as used in the edit-page view) should also reflect the imported content
updated_page_revision = updated_page.get_latest_revision_as_page()
self.assertEqual(updated_page_revision.intro, "This is the updated homepage")
self.assertEqual(updated_page_revision.title, "New home")

created_page = SimplePage.objects.get(url_path='/home/imported-child-page/')
self.assertEqual(created_page.intro, "This page is imported from the source site")
# An initial page revision should also be created
self.assertTrue(created_page.get_latest_revision())
created_page_revision = created_page.get_latest_revision_as_page()
self.assertEqual(created_page_revision.intro, "This page is imported from the source site")

@mock.patch('requests.get')
def test_import_image_with_file(self, get):
get.return_value.status_code = 200
Expand Down
12 changes: 12 additions & 0 deletions tests/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ def test_get(self):
self.assertEqual(response.status_code, 200)
self.assertContains(response, 'data-wagtail-component="content-import-form"')

class TestCheckUIDView(TestCase):
fixtures = ['test.json']

def setUp(self):
self.client.login(username='admin', password='password')

def test_get(self):
with self.settings(WAGTAILTRANSFER_LOOKUP_FIELDS = {'wagtailcore.page': ['slug', 'locale_id'],}):
# the view should parse comma-seperated params correctly
response = self.client.get('/admin/wagtail-transfer/api/check_uid/?uid=home,1')
self.assertEqual(response.status_code, 200)

@mock.patch('requests.post')
@mock.patch('requests.get')
Expand Down Expand Up @@ -383,6 +394,7 @@ def _test_view(self, method, url, data=None, success_url=None):
with self.subTest(user=user):
self.client.login(username=user.username, password="password")
request = getattr(self.client, method)
breakpoint()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an accidental addition?

response = request(url, data)
if success_url:
self.assertRedirects(response, success_url)
Expand Down
21 changes: 16 additions & 5 deletions wagtail_transfer/locators.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,12 @@
'taggit.tag': ['slug'], # sensible default for taggit; can still be overridden
'wagtailcore.locale': ["language_code"]
}
for model_label, fields in getattr(settings, 'WAGTAILTRANSFER_LOOKUP_FIELDS', {}).items():
LOOKUP_FIELDS[model_label.lower()] = fields

def get_lookup_fields():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth caching this as well? We'll work it out once per locator we look up, which feels a little redundant

""" get all fields for lookup, including those declared in settings """
lookup_fields = LOOKUP_FIELDS.copy()
for model_label, fields in getattr(settings, 'WAGTAILTRANSFER_LOOKUP_FIELDS', {}).items():
lookup_fields[model_label.lower()] = fields
return lookup_fields

class IDMappingLocator:
def __init__(self, model):
Expand Down Expand Up @@ -122,12 +125,19 @@ def uid_from_json(self, json_uid):
# A UID coming from JSON data will arrive as a list (because JSON has no tuple type),
# but we need a tuple because the importer logic expects a hashable type that we can use
# in sets and dict keys
if type(json_uid) is str:
json_uid = [json_uid]
return tuple(json_uid)

def find(self, uid):
# pair up field names with their respective items in the UID tuple, to form a filter dict
# that we can use for an ORM lookup
filters = dict(zip(self.fields, uid))
if type(uid) == tuple:
filters = dict(zip(self.fields, uid))
elif type(uid) == str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the last PR, I'm not sure whether this is the right approach - it feels like this method should accept a single type, and the caller (or another method, like we use when we get it from JSON with uid_from_json) should take care of conversion into a tuple. My thought is that currently, it looks like we put the uid into a single GET parameter here:

`${localCheckUIDUrl}?uid=${sourcePage.meta.uid}`
. That's why my previous request.GET.getlist() suggestion failed - we're not using the typical param=a&param=b GET syntax for passing a list. We could fix that and use getlist in the view, or else we could do the conversion to tuple in the view/in another method like uid_from_json. But just like the JSON case isn't in the main function, I don't think the list case should be either.

# if lookup fields are configured for wagtailcore.page, then in the admin view those
# fields get passed along as a string
filters = dict(zip(self.fields, uid.split(",")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the lookup field value contains a ","?


try:
return self.model.objects.get(**filters)
Expand All @@ -138,9 +148,10 @@ def find(self, uid):
@lru_cache(maxsize=None)
def get_locator_for_model(model):
base_model = get_base_model(model)
lookup_fields = get_lookup_fields()
try:
# Use FieldLocator if an entry exists in LOOKUP_FIELDS
fields = LOOKUP_FIELDS[base_model._meta.label_lower]
fields = lookup_fields[base_model._meta.label_lower]
return FieldLocator(base_model, fields)
except KeyError:
# Fall back on IDMappingLocator
Expand Down