Bug 39498 - Correct display of patron restriction comments
Summary: Correct display of patron restriction comments
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Philip Orr
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 31097
Blocks:
  Show dependency treegraph
 
Reported: 2025-03-31 13:31 UTC by Philip Orr
Modified: 2025-05-15 15:35 UTC (History)
7 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 39498: correct patron restrictions displays (9.09 KB, patch)
2025-04-01 10:07 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 39498: correct patron restrictions displays (9.16 KB, patch)
2025-04-02 09:22 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
Bug 39498: correct patron restrictions displays (9.20 KB, patch)
2025-05-12 17:05 UTC, Philip Orr
Details | Diff | Splinter Review
Screenshots (46.32 KB, image/png)
2025-05-14 06:05 UTC, Martin Renvoize (ashimema)
Details
Bug 39498: correct patron restrictions displays (11.04 KB, patch)
2025-05-14 06:06 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Another proposal (131.43 KB, image/png)
2025-05-14 11:10 UTC, Owen Leonard
Details
Bug 39498: correct patron restrictions displays (18.55 KB, patch)
2025-05-15 15:35 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Orr 2025-03-31 13:31:47 UTC
In Bug 31097, we introduced the possibility to show display of patron restriction types and expiry dates directly in the patron's account in checkouts and in details. (And to the display in the OPAC)
The way it is currently, if a patron has many restrictions but only on one of them they have a comment on the restriction, the text "with the explanation:" is shown for each restriction, even those with no comments. This looks not right.
The text should only be shown for the one restriction that has a comment and not for the others.
This is because we are checking if "debarredcomment" exists. But "debarredcomment" is filled as soon as any one restriction has a comment. So it will be true even for other restrictions that have no comment as long as they are on the same patron account as one that has a comment.
Instead we should be checking inside the FOREACH loop for every restriction, if it has restriction.comment filled, and only display the text if that is true for that specific restriction.

Also, if you add a restriction with a comment containing the text "OVERDUES_PROCESS" (or more likely if such a restriction is set automatically during overdues process), all other restrictions are no longer displayed.
This is because the overdues check again happens before the FOREACH loop. Again it should be inside the loop so it only displays for the Overdues restrictions and does not block other restrictions from also being shown.
Comment 1 Philip Orr 2025-03-31 13:32:25 UTC
I will write a patch to correct this and add it here.
Comment 2 Philip Orr 2025-04-01 09:57:14 UTC
Test plan:
1. Before applying the patch, check how the display is currently. First set syspref PatronRestrictionTypes to "Allow"
2. Add two different new restriction types, fill out both "Code" and "Label"
3. Go to a patron's checkout page and add one of each of these restrictions, but do not fill out the comment field when adding them!
4. Observe that in the checkouts page and details page of the patron, it just says "Restricted since..." and does not show the restriction types.
5. Add another restriction, this time fill out the comment field
6. Observe that now in checkout and detail pages, the labels from the restriction types are listed and the comment shown.
7. Apply patch
8. Go to patron's details and checkout pages and check the restrictions are now displayed correctly, that is the text "with the explanation" is only shown for the restrictions that actually have a comment
8b. Log in to the OPAC with that patron and check they are displayed correctly in their summary page and when the patron is trying to place a hold.
9. Go to a different patron's account and add a restriction without a comment
10. Check it is now displayed correctly
11. Add more restrictions and check they are displayed correctly. Also add an OVERDUES restriction:
a. find a patron with an overdue item checked out (or create an overdue checkout on a patron's account)
b. go to Tools -> Overdue notice/status triggers
c. set up a rule for that patron's category, e.g. "First" -> Delay: 1, make sure to mark the check box for "Restrict" and "Print"
d. run misc/cronjobs/overdue_notices.pl
e. that patron should now also have an OVERDUES restriction
12. Turn PatronRestrictionTypes off again and check the patrons if everything is still displayed correctly
13. Go to another different patron and add a restriction with no comment, check if it is displayed correctly
14. Add another restriction with a comment and check it is displayed correctly
15. Log in to the OPAC with the patron that had OVERDUES restriction and check they are displayed correctly in their summary page and when the patron is trying to place a hold.
16. Same thing for the other patron without the OVERDUES restriction.
Comment 3 Philip Orr 2025-04-01 10:07:43 UTC Comment hidden (obsolete)
Comment 4 claire.hernandez@biblibre.com 2025-04-02 09:17:22 UTC
c. set up a rule for that patron's category, e.g. "First" -> Delay: 1, make sure to mark the check box for "Restrict" and "Print"

You can add Letter = ODUE
Comment 5 claire.hernandez@biblibre.com 2025-04-02 09:22:33 UTC Comment hidden (obsolete)
Comment 6 Philip Orr 2025-05-12 17:05:12 UTC Comment hidden (obsolete)
Comment 7 David Nind 2025-05-12 20:49:38 UTC
Hi Claire, it looks like you have signed off this bug (which is great, thanks!).

To get the credit on the dashboard, please change the status to Signed Off.

David
Comment 8 Philip Orr 2025-05-13 08:11:29 UTC
Claire had set it to signed off, I set the status back to "Needs Signoff" by accident after I rebased this one - sorry about that - setting back to "Signed Off" :)
Comment 9 Philip Orr 2025-05-13 08:12:13 UTC
Oh and I guess I should mention, it's rebased now.
Comment 10 David Nind 2025-05-13 11:14:07 UTC
Excellent!. Thanks Philip.
Comment 11 Martin Renvoize (ashimema) 2025-05-14 06:05:27 UTC
Created attachment 182425 [details]
Screenshots

I'm not sure I like this change to be honest.. I find it less clear..

I'd be interested in others opinions.. I'd be tempted to just drop the 'with explanation' text from the top and display the original list as a list as before.. I likes that the dates/indefinite lined up the the beginning, followed by restriction type, followed by the optional comment.

Your change, to me, makes it harder to parse at a glance to me... seeking others opinions here.
Comment 12 Martin Renvoize (ashimema) 2025-05-14 06:06:18 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize (ashimema) 2025-05-14 06:07:18 UTC
To get this in a state ready for QA I had to rework it very slightly as we had invalid HTML getting generated and thus the tidy scripts weren't applying.
Comment 14 Katrin Fischer 2025-05-14 07:15:33 UTC
I have to admit that the first version seems easier to scan at a busy circulation desk.
Comment 15 Philip Orr 2025-05-14 08:00:07 UTC
I'll see if I can find time next week to try and rework the display.
Comment 16 Philip Orr 2025-05-14 08:00:37 UTC
.. and thanks Martin for the QA rework! :)
Comment 17 Owen Leonard 2025-05-14 11:10:49 UTC
Created attachment 182437 [details]
Another proposal

I've been tinkering with this too, trying to make it clearer.

I notice it displays expired restrictions. Has it always been that way?
Comment 18 Aude Charillon 2025-05-14 11:30:29 UTC
Still haven't had a proper look but quick reply here...

(In reply to Owen Leonard from comment #17)
> I notice it displays expired restrictions. Has it always been that way?

I believe so. The Manual says: "Expired restrictions are marked as such as of Koha version 23.11. In prior versions, if a restriction was expired, it looked the same as an active restriction, but it stopped blocking circulation nonetheless." https://koha-community.org/manual/latest/en/html/patrons.html#restrictions

Some libraries use cleanup_database.pl to delete old expired restrictions.
Comment 19 Philip Orr 2025-05-14 15:57:08 UTC
Thanks Owen, that looks great to me - reads easier to me at a glance than the way it was before!
And definitely better than my own attempt.
Comment 20 Owen Leonard 2025-05-15 15:35:40 UTC
Created attachment 182491 [details] [review]
Bug 39498: correct patron restrictions displays

This patch updates the way patron restrictions are displayed in the OPAC
and staff interface, with the intention of making the details of each
restriction more clear. In the OPAC, a new include file is created to be
reused on the holds page and the user summary page. opac-user.pl is
updated to allow a parameter to be passed to automatically open the
"Overdues" tab.

Test plan (copied from the original patch):

1. Before applying the patch, check how the display is currently. First
   set syspref PatronRestrictionTypes to "Allow"
2. Add two different new restriction types, fill out both "Code" and
   "Label"
3. Go to a patron's checkout page and add one of each of these
   restrictions, but do not fill out the comment field when adding them!
4. Observe that in the checkouts page and details page of the patron, it
   just says "Restricted since..." and does not show the restriction
   types.
5. Add another restriction, this time fill out the comment field
6. Observe that now in checkout and detail pages, the labels from the
   restriction types are listed and the comment shown.
7. Apply patch
8. Go to patron's details and checkout pages and check the restrictions
   are now displayed correctly, that is the text "with the explanation"
   is only shown for the restrictions that actually have a comment
   8b. Log in to the OPAC with that patron and check they are displayed
       correctly in their summary page and when the patron is trying to
       place a hold.
9. Go to a different patron's account and add a restriction without a
   comment
10. Check it is now displayed correctly
11. Add more restrictions and check they are displayed correctly. Also
    add an OVERDUES restriction:
    11a. Find a patron with an overdue item checked out (or create an
         overdue checkout on a patron's account)
    11b. Go to Tools -> Overdue notice/status triggers
    11c. Set up a rule for that patron's category, e.g. "First" ->
         Delay: 1, make sure to mark the check box for "Restrict" and
         "Print"
    11d. Run misc/cronjobs/overdue_notices.pl
    11e. That patron should now also have an OVERDUES restriction
12. Turn PatronRestrictionTypes off again and check the patrons if
    everything is still displayed correctly
13. Go to another different patron and add a restriction with no
    comment, check if it is displayed correctly
14. Add another restriction with a comment and check it is displayed
    correctly
15. Log in to the OPAC with the patron that had OVERDUES restriction and
    check they are displayed correctly in their summary page and when
    the patron is trying to place a hold.
16. Same thing for the other patron without the OVERDUES restriction.