-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement board member details as snippet #1189
base: main
Are you sure you want to change the base?
Conversation
from lametro.models import LAMetroPerson, BoardMemberDetails | ||
|
||
|
||
@receiver(post_save, sender=LAMetroPerson) |
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.
Will handle instantiating details for new members moving forward. To backfill, we simply need to call save on all existing board members.
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.
The review app didn't have any board members listed in the cms to start, but saving Najarian in the shell got him listed, just like you're describing!
Would it make sense to perform this save for all existing members in the new migration in this pr? That way it wouldn't have to be a manual step for staging and production when this gets pushed
_revisions = GenericRelation( | ||
"wagtailcore.Revision", related_query_name="member_details" | ||
) | ||
|
||
@property | ||
def revisions(self): | ||
return self._revisions |
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.
council_post = self.person.latest_council_membership.post | ||
context["qualifying_post"] = council_post.acting_label | ||
|
||
context["preview"] = True |
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.
The map and committee/board report modules require a bunch of extra context, so I use this to purposefully omit them from page previews. (I also added a note to the admin interface.)
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.
The map currently isn't showing up on the non-preview version of the person detail page. This might be because it's on a review app that doesn't have access to map_geojson
but wanted to make sure!
@@ -0,0 +1,4 @@ | |||
{% load wagtailadmin_tags %} |
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.
More or less stolen from the Wagtail source code, to make the status icon in the board member details snippet list link to the person detail page.
return user.has_perm(self._get_permission_name(action)) | ||
|
||
|
||
class BoardMemberFilterSet(django_filters.FilterSet): |
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 a django-filter
filter set, which I'm using to add custom filters for whether a person is a current board member and what body they belong to, on the board member detail snippet listing page.
HelpPanel( | ||
content=( | ||
"<p>On this page, you can manage a board member's headshot and " | ||
"bio. All other details and relationships, e.g., memberships, " | ||
"should be managed in Legistar.</p>" | ||
"<p><strong>Note:</strong> The page preview excludes " | ||
"the map and committee and board report modules displayed " | ||
"on the live page.</p>" | ||
) | ||
), |
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.
🙃
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 makes sense. And omitting those extra bits of context should help keep the preview lightweight which is a good call!
register_snippet(BoardMemberDetailsViewSet) | ||
|
||
|
||
class UserBarLink: |
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.
I pulled some reusable things out of the model admin link so I could also use them for the snippet edit link.
finder = AdminURLFinder() | ||
return finder.get_edit_url(snippet) |
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.
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.
Convenient!
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 looks really cool so far! Got a few a questions and comments below, but the status links and "edit this page" links work exactly as expected, and the whole editing process is smooth.
loaded_obj = ( | ||
type(self) | ||
.objects.select_related("person") | ||
.prefetch_related("person__memberships") | ||
.get(id=self.id) | ||
) | ||
return f"{loaded_obj.person.name}{' (current)' if loaded_obj.person.current_memberships.exists() else ''}" |
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.
Ah TIL, this is cool! A neat way to reduce the number of queries.
When do you personally like to use these select_related
and prefetch_related
? Because I imagine this would be useful in most cases where we know we want to grab some related objects, but I'd also think it'd be more nuanced than that.
HelpPanel( | ||
content=( | ||
"<p>On this page, you can manage a board member's headshot and " | ||
"bio. All other details and relationships, e.g., memberships, " | ||
"should be managed in Legistar.</p>" | ||
"<p><strong>Note:</strong> The page preview excludes " | ||
"the map and committee and board report modules displayed " | ||
"on the live page.</p>" | ||
) | ||
), |
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 makes sense. And omitting those extra bits of context should help keep the preview lightweight which is a good call!
finder = AdminURLFinder() | ||
return finder.get_edit_url(snippet) |
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.
Convenient!
from lametro.models import LAMetroPerson, BoardMemberDetails | ||
|
||
|
||
@receiver(post_save, sender=LAMetroPerson) |
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.
The review app didn't have any board members listed in the cms to start, but saving Najarian in the shell got him listed, just like you're describing!
Would it make sense to perform this save for all existing members in the new migration in this pr? That way it wouldn't have to be a manual step for staging and production when this gets pushed
council_post = self.person.latest_council_membership.post | ||
context["qualifying_post"] = council_post.acting_label | ||
|
||
context["preview"] = True |
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.
The map currently isn't showing up on the non-preview version of the person detail page. This might be because it's on a review app that doesn't have access to map_geojson
but wanted to make sure!
Overview
This PR adds a Django model for board member headshots and bios and registers as a snippet for editing in the CMS. It also deprecates the
headshot_url
property, instead creating a shared template block that renders the right thing given a person instance and, optionally, a way to request the original aspect ratio, instead of a square thumbnail.Connects #1176
Demo
Notes
I decided to go ahead and implement this as a snippet, since the
modeladmin
module is deprecated in the next version of Wagtail, so using snippets now makes our migration path easier moving forward. I'm wondering if we might want to do this in #1185, too. https://docs.wagtail.org/en/v5.2.1/reference/contrib/modeladmin/migrating_to_snippets.htmlTesting Instructions