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

Adding `'wagtailcore.page' to WAGTAILTRANSFER_LOOKUP_FIELDS causes error trying to match in admin #133

Open
easherma-truth opened this issue Dec 20, 2022 · 5 comments · May be fixed by #138
Labels
bug Something isn't working

Comments

@easherma-truth
Copy link

easherma-truth commented Dec 20, 2022

I don’t think there was ever an expectation that WAGTAILTRANSFER_LOOKUP_FIELDS would be used for looking up the page model, but nothing prevents that from happening either.

However, with this setting on import the admin will fail to find the correct matching page.

#132 proposes a change to allow this, though please consider it just a starting place for discussion as it may not be the best place to make the change (and probably merits some test coverage, ideally)

To replicate

  1. Have an instance set up with the above setting included in WAGTAILTRANSFER_LOOKUP_FIELDS
  2. Open the admin view and go to import
  3. Notice you'll get an error like ValueError: Field 'id' expected a number but got 'e'. when trying to choose the page to import
  4. This is because in that case uid is getting passed along as a string instead of a tuple, so it can't match based on that because it gets parsed incorrectly
@Scotchester
Copy link

Hey Eric,

We ran into this issue as well because we wanted to use url_path to look up pages:

WAGTAILTRANSFER_LOOKUP_FIELDS = {
    …
    "wagtailcore.Page": ["url_path"],
}

As we were on a tight deadline to make use of this, we implemented a very similar fix in a fork, intending to PR it back of course, but just never got around to it.

Would love to work together and settle on the best approach. /cc @addisonhardy

Also interested in input from the wagtail-transfer creators on why this sort of approach wasn't taken in the first place, given that maintaining links to / choices of pages on transfer seems pretty highly desirable.

@easherma-truth
Copy link
Author

Looks like at least some related discussion and context in the commit messages on this branch: #15

@easherma-truth
Copy link
Author

@Scotchester one idea I'm also considering that I wonder if ya'll considered:

def check_page_existence_for_uid(request):
    """
    Check whether a page with the specified UID exists - used for checking whether a page has already been imported
    to the destination site
    """
    uid = request.GET.get('uid', '').split(',')

this seems to work without changes to locators.py that we both jumped to

@Scotchester
Copy link

But that only works if the page has already been imported? The reason we went down this road was to handle links/choosers to non-imported pages that already exist.

@easherma-truth
Copy link
Author

@Scotchester right, thanks for the reminder! :-D

@easherma easherma linked a pull request Jan 12, 2023 that will close this issue
@thibaudcolas thibaudcolas added the bug Something isn't working label Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants