You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Character.can_link (character/models/character.py) is a single stored BooleanField that's written by several independent code paths, each representing a different reason a character should or shouldn't be linkable:
character/signals.py — recomputes it based on whether the character has an active PlayerCharacterLink.
locations/management/commands/spawn_characters.py — sets it once at creation based on age (can_link = age_days >= int(15 * 365.25), i.e. underage characters aren't linkable).
character/admin.py — mark_as_npc / mark_as_canlink admin actions toggle it by hand (e.g. to manually reserve a character for a future storyline).
character/services/link_services.py — player_link_assign_character sets it False on link, player_link_deactivate_active_links sets it True on unlink.
Because these all write to the same flat bit, they can clobber each other with no way to recover the reason afterward. For example: an admin manually reserves an NPC for a storyline (can_link = False), but the next active-link change or a population-centre unlock could flip it back to True with no record that it was ever reserved. There's currently no way to tell why a given character has can_link=False — active link, underage, manually reserved, or centre locked are all indistinguishable.
Proposal
Keep explicit, independent fields/state for each underlying reason, and derive can_link (or an equivalently-named property/queryset filter) from combining them, rather than one flag multiple call sites mutate directly:
Active link → already derivable via character.links.filter(is_active=True).exists() (see Character.is_npc).
Underage → derivable from age/birth date (whatever field spawn_characters.py currently computes age_days from).
Manually reserved (e.g. for a storyline) → would need a new explicit field, e.g. is_reserved (BooleanField(default=False)), since there's currently no field capturing "held back on purpose" separately from "not currently linkable." This should surface as its own "Reserved" checkbox on CharacterAdmin (character/admin.py), replacing the current mark_as_canlink/mark_as_npc actions as the way to manually hold a character back — an admin ticks "Reserved" on the character directly rather than toggling can_link.
can_link would then be not is_reserved and not underage and not has_active_link and (population_centre is None or population_centre.characters_can_link).
Implementation considerations
can_link is used today as a DB-level filter in several places (users/utils.py::assign_character_to_player, character/services/character_services.py::character_has_available, character/filters.py::CharacterFilter, character/serializers.py). If it becomes a pure Python property, these need a queryset-level equivalent (e.g. a manager method or annotate) rather than a plain .filter(can_link=True) lookup.
Need to decide whether can_link stays as a real column that's recomputed and saved whenever an underlying reason changes (simplest migration path, keeps existing .filter(can_link=True) call sites working), versus becoming fully computed/annotated with no stored column (more correct, more invasive).
Every current writer of can_link (listed above) needs to move to setting the underlying reason instead of the flag directly — including replacing the mark_as_canlink/mark_as_npc admin actions with the is_reserved checkbox described above.
Related
Split out from #681 (population centre lock/unlock for character linking), which would otherwise become one more ad-hoc writer of can_link.
Problem
Character.can_link(character/models/character.py) is a single storedBooleanFieldthat's written by several independent code paths, each representing a different reason a character should or shouldn't be linkable:character/signals.py— recomputes it based on whether the character has an activePlayerCharacterLink.locations/management/commands/spawn_characters.py— sets it once at creation based on age (can_link = age_days >= int(15 * 365.25), i.e. underage characters aren't linkable).character/admin.py—mark_as_npc/mark_as_canlinkadmin actions toggle it by hand (e.g. to manually reserve a character for a future storyline).character/services/link_services.py—player_link_assign_charactersets itFalseon link,player_link_deactivate_active_linkssets itTrueon unlink.PopulationCentre.characters_can_linkflag would also need to gate linkability.Because these all write to the same flat bit, they can clobber each other with no way to recover the reason afterward. For example: an admin manually reserves an NPC for a storyline (
can_link = False), but the next active-link change or a population-centre unlock could flip it back toTruewith no record that it was ever reserved. There's currently no way to tell why a given character hascan_link=False— active link, underage, manually reserved, or centre locked are all indistinguishable.Proposal
Keep explicit, independent fields/state for each underlying reason, and derive
can_link(or an equivalently-named property/queryset filter) from combining them, rather than one flag multiple call sites mutate directly:character.links.filter(is_active=True).exists()(seeCharacter.is_npc).spawn_characters.pycurrently computesage_daysfrom).is_reserved(BooleanField(default=False)), since there's currently no field capturing "held back on purpose" separately from "not currently linkable." This should surface as its own "Reserved" checkbox onCharacterAdmin(character/admin.py), replacing the currentmark_as_canlink/mark_as_npcactions as the way to manually hold a character back — an admin ticks "Reserved" on the character directly rather than togglingcan_link.character.population_centre.characters_can_link(Add command to lock/unlock a PopulationCentre's characters for linking #681).can_linkwould then benot is_reserved and not underage and not has_active_link and (population_centre is None or population_centre.characters_can_link).Implementation considerations
can_linkis used today as a DB-level filter in several places (users/utils.py::assign_character_to_player,character/services/character_services.py::character_has_available,character/filters.py::CharacterFilter,character/serializers.py). If it becomes a pure Python property, these need a queryset-level equivalent (e.g. a manager method orannotate) rather than a plain.filter(can_link=True)lookup.can_linkstays as a real column that's recomputed and saved whenever an underlying reason changes (simplest migration path, keeps existing.filter(can_link=True)call sites working), versus becoming fully computed/annotated with no stored column (more correct, more invasive).can_link(listed above) needs to move to setting the underlying reason instead of the flag directly — including replacing themark_as_canlink/mark_as_npcadmin actions with theis_reservedcheckbox described above.Related
Split out from #681 (population centre lock/unlock for character linking), which would otherwise become one more ad-hoc writer of
can_link.