[Bug] Hold list permission model: inconsistent ownership, lock, and affordance checks

[andrewyager]

Summary

An audit of the hold list permission logic reveals a patchwork of inconsistent checks across views, services, and templates. Ownership, lock status, and role gates are applied differently depending on which view handles the request, and several views that exist have no corresponding UI affordance.

Current state

View	Role gate	Ownership check	Lock check	Template affordance
holdlist_list	None	—	—	N/A
holdlist_detail	None (computes flags)	—	—	N/A
holdlist_create	_require_holdlist_write	N/A	N/A	Link in list view
holdlist_edit	Inline	Creator OR admin/mgr	Yes	Edit button (gated)
holdlist_delete	Inline	Creator OR admin/mgr	None	No button
holdlist_add_item	_require_holdlist_write	None	Service layer	Add form (gated)
holdlist_remove_item	_require_holdlist_write	None	Service layer	Remove button (gated)
holdlist_edit_item	_require_holdlist_write	None	None	No button
holdlist_update_pull_status	_require_holdlist_write	None	None	Pull buttons (shown on locked lists)
holdlist_lock	_require_holdlist_write	None	—	No button
holdlist_unlock	_require_holdlist_write	None	—	No button
holdlist_fulfil	_require_holdlist_write	None	None	No button
holdlist_pick_sheet	None	None	—	PDF link (ungated)

Problems

1. Inconsistent ownership checks

holdlist_edit and holdlist_delete require the user to be the creator (or admin/department_manager). But holdlist_lock, holdlist_unlock, holdlist_fulfil, and all item-mutation views only check the role — any member can lock, unlock, fulfil, or mutate items on any hold list, even ones they didn't create.

Decision needed: Should members be able to collaborate on any hold list (current behaviour for items), or should destructive/state-changing operations (lock, unlock, fulfil, delete) require ownership like edit does?

2. Missing lock checks

• holdlist_edit_item — can edit item quantity/notes on a locked list
• holdlist_update_pull_status — can change pull status on a locked list
• holdlist_delete — can delete a locked hold list
• holdlist_fulfil — can bulk-checkout from a locked list

add_item and remove_item correctly delegate lock enforcement to the service layer, but the other views don't check at all — neither in the view nor in any service call.

3. Missing template affordances

These views exist and are routed but have no corresponding button, link, or form in holdlist_detail.html:

• Lock / Unlock — no toggle button
• Delete — no delete button
• Fulfil — no fulfil/checkout button
• Edit Item (quantity/notes) — no inline edit control

Users can only reach these via direct URL, which means they're effectively invisible.

4. Pull status buttons shown on locked lists

The template renders "Mark Pulled" / "Mark Unavailable" / "Reset" buttons gated by can_write_holdlist but not by hold_list.is_locked. A member sees actionable buttons on a locked list, and the view will happily process the POST.

5. _require_holdlist_write is too thin

The helper only checks role membership. Every view that needs ownership or lock enforcement does it inline (or doesn't). This leads to the inconsistencies above.

Proposal: Extend the helper or create a richer permission function, e.g.:

def check_holdlist_permission(user, hold_list=None, *, require_owner=False, require_unlocked=False):
    role = get_user_role(user)
    if role not in HOLD_LIST_WRITE_ROLES:
        raise PermissionDenied
    if require_owner and hold_list:
        if hold_list.created_by != user and role not in ("system_admin", "department_manager"):
            raise PermissionDenied
    if require_unlocked and hold_list and hold_list.is_locked:
        if role not in ("system_admin", "department_manager"):
            raise PermissionDenied

Then each view calls it with the flags it needs, making the policy explicit and auditable.

Suggested fix order

1. Add missing lock checks to edit_item, update_pull_status, delete, and fulfil
2. Add ownership checks to lock, unlock, and fulfil (matching edit/delete)
3. Gate pull status buttons behind not hold_list.is_locked in the template
4. Add Lock/Unlock/Delete/Fulfil buttons to holdlist_detail.html with appropriate permission gating
5. Consolidate permission logic into a single helper
6. Add tests for each combination