Bug 29889 - Incorrect library check in patron message deletion logic
Summary: Incorrect library check in patron message deletion logic
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on: 21757
Blocks:
  Show dependency treegraph
 
Reported: 2022-01-14 19:33 UTC by Owen Leonard
Modified: 2022-12-12 21:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the logic controlling whether a patron message on the circulation or patron details page has a "Delete" link. An error in the logic prevented messages from being removed by staff who should have been authorized to do so.
Version(s) released in:
22.05.00,21.11.03
Circulation function:


Attachments
Bug 29889: Incorrect library check in patron message deletion logic (2.28 KB, patch)
2022-01-14 19:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29889: Incorrect library check in patron message deletion logic (2.36 KB, patch)
2022-01-19 10:18 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2022-01-14 19:33:31 UTC
If you DON'T have AllowAllMessageDeletion enabled, message deletion is supposed to be limited by library: Staff from the same library can delete each other's messages, but not messages left by other libraries.

At some point the logic around the display of the "Delete" link got changed, I think by Bug 21757. Now the check compares the branchcode of the staff member who left the message with the branchcode of the patron:

patron_message.branchcode == patron.branchcode

I think this is meant to compare patron_message.branchcode to the logged-in library.
Comment 1 Owen Leonard 2022-01-14 19:48:09 UTC
Created attachment 129514 [details] [review]
Bug 29889: Incorrect library check in patron message deletion logic

This patch corrects the logic controlling whether a patron message on
the circulation or patron details page has a "Delete" link. An error in
the logic prevented messages from being removed by staff who should have
been authorized to do so.

To reproduce the bug, check that your AllowAllMessageDeletion preference
is disabled.

- In the staff client, check out to a patron whose home library doesn't
  match the library you're logged in at.
- Add a message to the patron's account.
- You should see no "Delete" link next to the newly-added message.
  - If you edit the patron so that their home library matches the
    library you're logged in at the delete link will appear.

To test, apply the patch and follow the steps above. The delete link
should aways appear if the message was left by someone logged in at the
same library.
Comment 2 Jonathan Druart 2022-01-19 10:18:26 UTC
Created attachment 129592 [details] [review]
Bug 29889: Incorrect library check in patron message deletion logic

This patch corrects the logic controlling whether a patron message on
the circulation or patron details page has a "Delete" link. An error in
the logic prevented messages from being removed by staff who should have
been authorized to do so.

To reproduce the bug, check that your AllowAllMessageDeletion preference
is disabled.

- In the staff client, check out to a patron whose home library doesn't
  match the library you're logged in at.
- Add a message to the patron's account.
- You should see no "Delete" link next to the newly-added message.
  - If you edit the patron so that their home library matches the
    library you're logged in at the delete link will appear.

To test, apply the patch and follow the steps above. The delete link
should aways appear if the message was left by someone logged in at the
same library.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 3 Jonathan Druart 2022-01-19 10:21:24 UTC
Trivial, skipping QA.
Comment 4 Fridolin Somers 2022-01-19 21:48:38 UTC
I'm about to push master

I was puzzled by test plan until I understood it is without patch, then with patch :)

BTW : 
We may allow superlibrarian to delete any message even when system preference is disabled.
Comment 5 Fridolin Somers 2022-01-19 22:16:30 UTC
Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑
Comment 6 Kyle M Hall (khall) 2022-02-07 11:35:28 UTC
Pushed to 21.11.x for 21.11.03
Comment 7 Andrew Fuerste-Henry 2022-02-16 17:00:35 UTC
Missing dependency, not backported to 21.05