wagtail-nest / wagtail-bakery

A set of helpers for baking your Django Wagtail site out as flat files.
MIT License
182 stars 40 forks source link

[REVIEW] Fix/root page issue #51

Open rosscdh opened 4 years ago

rosscdh commented 4 years ago

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

rosscdh commented 4 years ago

Added basic test

rosscdh commented 4 years ago

ping

tomdyson commented 4 years ago

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

zerolab commented 4 years ago

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)