Fix non uid lookup for pages #138
Conversation
This also wraps the LOOKUP_FIELD initialization in a function, so that settings can be initialized correctly
|
See #132 for original discussion |
| @@ -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() | |||
There was a problem hiding this comment.
This looks like an accidental addition?
| elif type(uid) == str: | ||
| # 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(","))) |
There was a problem hiding this comment.
What if the lookup field value contains a ","?
| @@ -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, | |||
There was a problem hiding this comment.
Nitpick: "interferes" and "effectively" typos
| filters = dict(zip(self.fields, uid)) | ||
| if type(uid) == tuple: | ||
| filters = dict(zip(self.fields, uid)) | ||
| elif type(uid) == str: |
There was a problem hiding this comment.
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:
request.GET.getlist() suggestion failed - we're not using the typical param=a¶m=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.
| for model_label, fields in getattr(settings, 'WAGTAILTRANSFER_LOOKUP_FIELDS', {}).items(): | ||
| LOOKUP_FIELDS[model_label.lower()] = fields | ||
|
|
||
| def get_lookup_fields(): |
There was a problem hiding this comment.
Worth caching this as well? We'll work it out once per locator we look up, which feels a little redundant
Fixes #133 and adds some test coverage, as well as some tweaks to the way some settings are initialized so that those tests can work