-
Notifications
You must be signed in to change notification settings - Fork 153
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
Moved IndexPage into its own file #4289
Conversation
@@ -82,58 +66,6 @@ def titlecase(s): | |||
] if field is not None] | |||
|
|||
|
|||
# Override the MetadataPageMixin to allow for a default | |||
# description and image in page metadata for all Pages on the site | |||
class FoundationMetadataPageMixin(MetadataPageMixin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class got moved to wagtailpages/pagemodels/mixin/foundation_metadata.py
@@ -530,282 +462,6 @@ def get_context(self, request): | |||
return get_page_tree_information(self, context) | |||
|
|||
|
|||
class IndexPage(FoundationMetadataPageMixin, RoutablePageMixin, Page): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class got moved to wagtailpages/pagemodels/index_page.py
@@ -39,17 +34,6 @@ | |||
from .donation_modal import DonationModals # noqa: F401 | |||
|
|||
|
|||
# See https://docs.python.org/3.7/library/stdtypes.html#str.title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function got moved into wagtailpages/utils.py
…ozilla.org into index-page-refactor
# helper function to resolve category slugs to actual objects | ||
def get_category_object_for_slug(self, category_slug): | ||
# FIXME: this should not need to be an app consult | ||
BlogPageCategory = apps.get_model('wagtailpages', 'BlogPageCategory') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only line of "new" code, because we can't have from ..models import BlogPageCategory
at the top of this file, since this file itself gets imported by wagtailpages.models
, so that would lead to a circular import dependency.
Instead we get that model "whenever this def gets called" by asking the Django app for it by name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! So much cleaner! ✨
Wondering if we should update this line so FoundationMetadataPageMixin
is imported from the new file?
yep, good call. |
Hm, I had a closer look at how that code works, and it feels like it's better to keep the import from wagtailpages.models rather than its refactored location, since the mozfest app "extends" wagtailpages, and shouldn't need to know anything about the directory structure inside that app. It should be able to trust that it can import from (so if the files move around inside wagtailpages, keeping it the way it is means that mozfest files won't need to be updated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
Closes #4288
inv docker-test-python
should pass as usual.