Conversation
There was a problem hiding this comment.
Pull request overview
Updates petition detail rendering to better integrate linked Hansards and improve petition status display/progress UI.
Changes:
- Add “Hansard override” logic in
petition_detailto merge manual petition events of typehansardwith linked plenary (Hansard) events by date. - Show the petition’s current status name on the petition detail page.
- Adjust progress-bar styles for stage 4 and add an admin event type option for “Hansard”.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pmg/views.py |
Adds date-based matching to override linked Hansard titles with manual petition event titles. |
pmg/templates/petitions/detail.html |
Displays petition.status.name beneath the progress indicator. |
pmg/static/resources/css/bill-progress.scss |
Tweaks stage-4 tick styling and hides the process dot for petitions at stage 4. |
pmg/admin/__init__.py |
Adds hansard as a selectable PetitionEvent type in the admin inline form. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| linked_hansards_by_date = { | ||
| _event_date(e): e | ||
| for e in linked_events | ||
| if getattr(e, 'type', None) == 'plenary' | ||
| } | ||
|
|
||
| overridden_hansard_dates = set() | ||
| hansard_overrides = [] | ||
| for pe in hansard_petition_events: | ||
| pe_date = pe.date if not isinstance(pe.date, datetime) else pe.date.date() | ||
| matched = linked_hansards_by_date.get(pe_date) | ||
| if matched: | ||
| hansard_overrides.append(_HansardOverride(matched, pe)) | ||
| overridden_hansard_dates.add(pe_date) | ||
| # If no matched hansard, the petition event is silently dropped | ||
|
|
||
| # Exclude linked hansards that have been overridden | ||
| remaining_linked = [ | ||
| e for e in linked_events | ||
| if not (getattr(e, 'type', None) == 'plenary' and _event_date(e) in overridden_hansard_dates) |
There was a problem hiding this comment.
linked_hansards_by_date is keyed only by the date portion of Event.date, so if there are multiple linked Hansards on the same calendar day the dict will keep only one and effectively lose the others. This also interacts with overridden_hansard_dates/remaining_linked (date-based filtering) and can remove multiple Hansards while creating only a single override. Consider keying by a stable identifier (e.g., event id) or mapping each date to a list of hansards and overriding/removing only the specific matched instance(s).
| linked_hansards_by_date = { | |
| _event_date(e): e | |
| for e in linked_events | |
| if getattr(e, 'type', None) == 'plenary' | |
| } | |
| overridden_hansard_dates = set() | |
| hansard_overrides = [] | |
| for pe in hansard_petition_events: | |
| pe_date = pe.date if not isinstance(pe.date, datetime) else pe.date.date() | |
| matched = linked_hansards_by_date.get(pe_date) | |
| if matched: | |
| hansard_overrides.append(_HansardOverride(matched, pe)) | |
| overridden_hansard_dates.add(pe_date) | |
| # If no matched hansard, the petition event is silently dropped | |
| # Exclude linked hansards that have been overridden | |
| remaining_linked = [ | |
| e for e in linked_events | |
| if not (getattr(e, 'type', None) == 'plenary' and _event_date(e) in overridden_hansard_dates) | |
| # Map each date to a list of plenary events to avoid discarding multiple | |
| # hansards that occur on the same calendar day. | |
| linked_hansards_by_date = {} | |
| for e in linked_events: | |
| if getattr(e, 'type', None) == 'plenary': | |
| d = _event_date(e) | |
| linked_hansards_by_date.setdefault(d, []).append(e) | |
| # Track which specific linked hansards have been overridden, by id | |
| overridden_hansard_ids = set() | |
| hansard_overrides = [] | |
| for pe in hansard_petition_events: | |
| pe_date = pe.date if not isinstance(pe.date, datetime) else pe.date.date() | |
| candidates = linked_hansards_by_date.get(pe_date) or [] | |
| matched = None | |
| if candidates: | |
| # Use and consume one specific linked hansard for this petition event | |
| matched = candidates.pop(0) | |
| if matched: | |
| hansard_overrides.append(_HansardOverride(matched, pe)) | |
| overridden_hansard_ids.add(getattr(matched, 'id', None)) | |
| # If no matched hansard, the petition event is silently dropped | |
| # Exclude linked hansards that have been overridden | |
| remaining_linked = [ | |
| e for e in linked_events | |
| if not ( | |
| getattr(e, 'type', None) == 'plenary' | |
| and getattr(e, 'id', None) in overridden_hansard_ids | |
| ) |
| for pe in hansard_petition_events: | ||
| pe_date = pe.date if not isinstance(pe.date, datetime) else pe.date.date() | ||
| matched = linked_hansards_by_date.get(pe_date) | ||
| if matched: | ||
| hansard_overrides.append(_HansardOverride(matched, pe)) | ||
| overridden_hansard_dates.add(pe_date) | ||
| # If no matched hansard, the petition event is silently dropped | ||
|
|
There was a problem hiding this comment.
These hansard PetitionEvents are silently dropped when no matching linked Hansard exists on the same date. That means an editor can add a Hansard-type event in admin and it will never appear in the petition history, which is hard to diagnose and can look like data loss. Consider rendering unmatched hansard petition events as a non-linked history item (or surfacing a visible warning/validation in admin when saving).
| ('document', 'Document'), | ||
| ('report', 'Report'), | ||
| ('finalised', 'Finalised'), | ||
| ('hansard', 'Hansard'), |
There was a problem hiding this comment.
A new PetitionEvent type value hansard is exposed in the admin choices, but the petition detail template’s icon rendering uses /static/resources/images/petitions/{{ first_event.type }}.png for non-meeting types; there is no hansard.png in that directory (only plenary.png). If hansard events are ever meant to be displayed, add the missing icon and update the template logic to link to the relevant hansard; otherwise consider renaming this choice to reflect that it’s only an override (and/or ensuring it never reaches the template).
| ('hansard', 'Hansard'), |
| # Handle "hansard" petition events: these provide a custom title for a linked hansard. | ||
| # Match by date. If a linked hansard exists on the same date, combine them (custom title | ||
| # from the petition event, link/id from the hansard). If no matching hansard exists, | ||
| # skip the petition event entirely. | ||
| class _HansardOverride: | ||
| """Wraps a linked Hansard but uses a custom title from a PetitionEvent.""" | ||
| def __init__(self, hansard, petition_event): | ||
| self.id = hansard.id | ||
| self.type = 'plenary' | ||
| self.title = petition_event.title | ||
| self.date = hansard.date | ||
| self.description = petition_event.description | ||
| self.committee = getattr(petition_event, 'committee', None) | ||
|
|
||
| linked_events = list(petition.linked_events) | ||
| hansard_petition_events = [e for e in manual_events if e.type == 'hansard'] | ||
| other_manual_events = [e for e in manual_events if e.type != 'hansard'] | ||
|
|
There was a problem hiding this comment.
This change introduces non-trivial new behavior in petition history composition (date-matching and overriding linked hansards). There currently don’t appear to be any view tests covering /petitions/<id>/, so regressions here would be hard to catch. Adding a focused test for petition detail that asserts (1) override title is shown when a linked plenary exists on the same date and (2) other linked events are preserved would help protect this logic.
1 participant
No description provided.