wagtail-bakery icon indicating copy to clipboard operation
wagtail-bakery copied to clipboard

[REVIEW] Fix/root page issue

Open rosscdh opened this issue 5 years ago • 4 comments
trafficstars

Kept getting 404 on multisite true. Turns out, this was caused by the "root" page was being included in the result set.

rosscdh avatar Jan 02 '20 13:01 rosscdh

Added basic test

rosscdh avatar Jan 02 '20 15:01 rosscdh

ping

rosscdh avatar Jan 13 '20 12:01 rosscdh

Thanks @rosscdh ! @zerolab do you have any time to review this?

tomdyson avatar Jan 13 '20 13:01 tomdyson

Thank you for the tweaks @rosscdh. It is great to have a test.

I went on to replicate the issue locally using the examples/multisite project provided with wagtail-bakery, with

BAKERY_VIEWS = (
    'wagtailbakery.api_views.PagesAPIDetailView',
    'wagtailbakery.api_views.PagesAPIListingView',
    'wagtailbakery.api_views.TypedPagesAPIListingView',
)

in settings.py.

It still fails with

  File "wagtail-bakery/src/wagtailbakery/api_views.py", line 91, in build_queryset
    [self.build_object(o) for o in self.get_queryset()]
  File "wagtail-bakery/src/wagtailbakery/api_views.py", line 91, in <listcomp>
    [self.build_object(o) for o in self.get_queryset()]
  File "wagtail-bakery/src/wagtailbakery/api_views.py", line 85, in build_object
    page_content = self.get_content(obj)
  File "wagtail-bakery/src/wagtailbakery/api_views.py", line 114, in get_content
    handle_api_error(response)
  File "wagtail-bakery/src/wagtailbakery/api_views.py", line 25, in handle_api_error
    raise APIResponseError("Unexpected status code returned from API: %d" % response.status_code)
wagtailbakery.api_views.APIResponseError: Unexpected status code returned from API: 404

Digging a bit deeper, the issue seem to be related to https://github.com/wagtail/wagtail-bakery/blob/28ae53756c7a0da3106411d969036fb2e3968b5b/src/wagtailbakery/api_views.py#L105 -- the Wagtail API will limit the returned pages to the requesting site. So even though we're passing a list of pages from all both sites, it will successfully render the default site ones, then fail with a 404 for the first page in a non-default site

changing that line to request.site = obj.get_site() fixes the issue.

This would need a test too.

Last, but not least, the reason the tests were failing with "limit cannot be higher than 20" API error is because of https://github.com/wagtail/wagtail/blob/master/wagtail/api/v2/pagination.py#L31, i.e. if BAKERY_RESULTS_PER_PAGE > WAGTAILAPI_LIMIT_MAX (ref) it should explode. Interestingly, locally it worked without complaints 🤔

Sounds nitpicky, but the PR introduces a new setting which remains undocumented. My preference with the setting, to be on the safe side would be to wrap it in a method that will be defensive about BAKERY_RESULTS_PER_PAGE <= WAGTAILAPI_LIMIT_MAX (if set)

zerolab avatar Jan 13 '20 15:01 zerolab