Ensure related posts are of the same locale #8125
Conversation
Before, it could happen that related pages would be returned that had a different locale than the reference page. If a reference page would have a fairly unique combination of tags, it could happen that all related pages would be different locales of the reference page. This is now prevented by filtering related pages to be of the same locale as the reference page.
The `.locale` property is a foreign key relation ship from the translateable page to the `Locale` model. It should be avoided that this look up is repeated on every loop over the different page types.
Before, the locale property was access with no regard to the page type. Not all page types have to be translatable and have the property. The lookup is now protected and only used when present.
Much simpler... 🤦
|
The code looks good, but visual regression doesn't ssem to like this change, so we'll want to do some debugging to find out why. In order to run the visual regression tests locally, run |
|
Looking at the regression logs, I see: so I guess we'll have to go with |
|
Also, since the visual tests caught this, but the regular tests didn't, let's also add a small test in |
There was a problem hiding this comment.
apparently page.locale can throw, which is... not so great. Let's revert to getattr, write a small test for this codepath, and let's also file an issue with wagtail to report that this property can throw and that it probably should instead universally report None if there is no localization, or the default locale if localization is active.
I am actually confused as to why |
|
Re: exception vs. https://github.com/wagtail/wagtail/blob/main/wagtail/core/models/i18n.py#L89 |
|
It appears the issue is not the visual test itself, but the loading of the fake data: https://github.com/mozilla/foundation.mozilla.org/runs/4874848441?check_suite_focus=true#step:10:620 |
|
Ok, I am able to recreate locally with |
|
@Pomax I think I finally got it. The locale is only set when the page is first saved (cleaned really). But, the blog post save is overridden to add the related posts. Adding the related posts requires the locale. This is why we get the Only including the locale in the relation when it exists does not quite work, because all three spots will be filled with related posts potentially of other locales. This is the same problem that is being reported. The It also appears that the factory it not expecting for the save method to add the related posts, because the method is explicitly called before the save method is called. for post in BlogPage.objects.all():
post.ensure_related_posts()
post.save()I have just tested it locally and that seems to work. |
Before, when creating the blog post with the factory, there would be the problem that ensuring the related blog posts required the locale to be set. Wagtail only sets the local during the full_clean step for a page. The factory does not use the full_clean method, but does call the save method. This means when we try to ensure the related posts, the local access would fail, because this non-nullable field would be null, raising an exception. To fix this issue, I moved the ensure_related_posts call from the save method to the clean method. This still works as expected when creating pages through the Wagtail admin, because it runs the clean method before saving the page. This now also allows the factory to successfully create the page. On first save, the related posts will not be set. We need to make sure that the factory ensures that the related posts have been set. This is not a problem, because that is how the blog factory generate function already worked. We could potentially optimize this with the post-generation hooks that factory boy provides: https://factoryboy.readthedocs.io/en/stable/reference.html#post-generation-hooks
Before, it could happen that related pages would be returned that had a
different locale than the reference page.
If a reference page would have a fairly unique combination of tags, it could
happen that all related pages would be different locales of the
reference page.
This is now prevented by filtering related pages to be of the same
locale as the reference page.
Closes #7991
Related PRs/issues #
Link to sample test page:
Checklist
Remove unnecessary checks
Tests
Changes in Models:
Documentation: