fix: pack weapons not showing on fighter cards#1541
Conversation
ContentManager.get_queryset() excludes pack content by default. This filtered out pack weapon profiles in two places: - The M2M weapon_profiles_field prefetch used the default manager, so pack profiles were invisible on fighter cards after assignment. - The form validation for weapon_profiles_field and upgrades_field rejected pack content, silently failing to create assignments. Fixes #1540
There was a problem hiding this comment.
Pull request overview
Fixes a regression where weapon profiles originating from content packs were being filtered out during fighter card rendering, and adjusts equipment assignment form querysets to accept pack-scoped selections.
Changes:
- Prefetch
weapon_profiles_fieldusingContentWeaponProfile.objects.all_content()so pack weapon profiles render on fighter cards. - Set
weapon_profiles_fieldandupgrades_fieldform querysets towith_packs(packs)during equipment assignment POST validation. - Update the performance view query snapshot fixture to reflect the new prefetch SQL.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
gyrinx/core/views/fighter/equipment.py |
Broadens POST-time form field querysets to include pack-scoped weapon profiles and upgrades. |
gyrinx/core/models/list.py |
Updates fighter with_related_data() prefetch so assigned pack weapon profiles are included in card rendering. |
gyrinx/core/tests/fixtures/performance_view_queries.json |
Refreshes expected SQL snapshot for the performance view after the prefetch change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| form.fields["content_equipment"].queryset = ContentEquipment.objects.with_packs( | ||
| packs | ||
| ) | ||
| form.fields[ | ||
| "weapon_profiles_field" | ||
| ].queryset = ContentWeaponProfile.objects.with_packs(packs) | ||
| form.fields[ | ||
| "upgrades_field" | ||
| ].queryset = ContentEquipmentUpgrade.objects.with_packs(packs) |
There was a problem hiding this comment.
The weapon_profiles_field and upgrades_field querysets are widened to with_packs(packs) but not constrained to the selected content_equipment. This means a crafted POST can associate profiles/upgrades that belong to a different piece of equipment (the form will consider them valid as long as they’re in the queryset). Consider narrowing each queryset based on the posted/cleaned content_equipment (e.g., filter by equipment=...) before calling is_valid() so server-side validation enforces consistency.
| form.fields["content_equipment"].queryset = ContentEquipment.objects.with_packs( | |
| packs | |
| ) | |
| form.fields[ | |
| "weapon_profiles_field" | |
| ].queryset = ContentWeaponProfile.objects.with_packs(packs) | |
| form.fields[ | |
| "upgrades_field" | |
| ].queryset = ContentEquipmentUpgrade.objects.with_packs(packs) | |
| equipment_qs = ContentEquipment.objects.with_packs(packs) | |
| form.fields["content_equipment"].queryset = equipment_qs | |
| # Start with pack-filtered querysets for related profiles and upgrades | |
| weapon_qs = ContentWeaponProfile.objects.with_packs(packs) | |
| upgrade_qs = ContentEquipmentUpgrade.objects.with_packs(packs) | |
| # Further constrain the querysets based on the posted content_equipment, | |
| # so that only profiles/upgrades belonging to the selected equipment are valid. | |
| equipment_id = request.POST.get("content_equipment") | |
| if equipment_id: | |
| equipment_filter = {} | |
| if is_int(equipment_id): | |
| equipment_filter["pk"] = int(equipment_id) | |
| elif is_valid_uuid(equipment_id): | |
| equipment_filter["pk"] = equipment_id | |
| if equipment_filter: | |
| selected_equipment = equipment_qs.filter(**equipment_filter).first() | |
| if selected_equipment is not None: | |
| weapon_qs = weapon_qs.filter(equipment=selected_equipment) | |
| upgrade_qs = upgrade_qs.filter(equipment=selected_equipment) | |
| else: | |
| # Invalid or unauthorized equipment id: disallow any related selections | |
| weapon_qs = ContentWeaponProfile.objects.none() | |
| upgrade_qs = ContentEquipmentUpgrade.objects.none() | |
| else: | |
| # Malformed equipment id: disallow any related selections | |
| weapon_qs = ContentWeaponProfile.objects.none() | |
| upgrade_qs = ContentEquipmentUpgrade.objects.none() | |
| form.fields["weapon_profiles_field"].queryset = weapon_qs | |
| form.fields["upgrades_field"].queryset = upgrade_qs |
| "listfighterequipmentassignment_set__weapon_accessories_field__modifiers", | ||
| "listfighterequipmentassignment_set__content_equipment__modifiers", | ||
| "listfighterequipmentassignment_set__upgrades_field__modifiers", |
There was a problem hiding this comment.
weapon_profiles_field is now prefetched with all_content(), but upgrades_field (and weapon_accessories_field) are still reached via the default content manager through __upgrades_field__modifiers / __weapon_accessories_field__modifiers, which excludes pack items. If an assignment has a pack upgrade/accessory, it will still be invisible in templates and helpers like active_upgrade() (because assignment.upgrades_field won’t return pack rows). Consider adding explicit Prefetch entries for listfighterequipmentassignment_set__upgrades_field (and likely __weapon_accessories_field) with all_content() (and prefetch their modifiers) similar to weapon_profiles_field.
| "listfighterequipmentassignment_set__weapon_accessories_field__modifiers", | |
| "listfighterequipmentassignment_set__content_equipment__modifiers", | |
| "listfighterequipmentassignment_set__upgrades_field__modifiers", | |
| Prefetch( | |
| "listfighterequipmentassignment_set__weapon_accessories_field", | |
| queryset=ContentWeaponAccessory.objects.all_content().prefetch_related( | |
| "modifiers" | |
| ), | |
| ), | |
| "listfighterequipmentassignment_set__content_equipment__modifiers", | |
| Prefetch( | |
| "listfighterequipmentassignment_set__upgrades_field", | |
| queryset=ContentEquipmentUpgrade.objects.all_content().prefetch_related( | |
| "modifiers" | |
| ), | |
| ), |
| Prefetch( | ||
| "listfighterequipmentassignment_set__weapon_profiles_field", | ||
| queryset=ContentWeaponProfile.objects.all_content(), | ||
| ), |
There was a problem hiding this comment.
This change fixes a regression path (pack weapon profiles missing from fighter card rendering) but there doesn’t appear to be an automated regression test covering it. Adding a view-level test that creates a list subscribed to a pack, assigns a pack weapon profile to a fighter, and asserts the rendered fighter card (e.g., list/print/embed output) contains the weapon/profile name would help prevent this from regressing again.
Summary
all_content()in theweapon_profiles_fieldprefetchwith_packs()querysets onweapon_profiles_fieldandupgrades_fieldTest plan
Fixes #1540
🤖 Generated with Claude Code