Bug 26053 - Add styling to show expired patron restrictions as inactive
Summary: Add styling to show expired patron restrictions as inactive
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Fridolin Somers
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-23 17:42 UTC by Kelly McElligott
Modified: 2023-12-12 22:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/784
Text to go in the release notes:
This enhancement highlights that a patron restriction is expired in the patron restriction table (Patrons > ([patron] > Check out or Details tab > Restrictions): - adds the text "(expired)" after the expiration date, and - changes the text color for the line to a light gray.
Version(s) released in:
23.11.00


Attachments
member entry page - restriction (47.19 KB, image/png)
2020-07-29 12:05 UTC, Kelly McElligott
Details
Bug 26053: Distinguish expired patron restrictions (3.31 KB, patch)
2023-09-21 22:24 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.39 KB, patch)
2023-09-22 13:53 UTC, Caroline Cyr La Rose
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.43 KB, patch)
2023-09-22 15:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.34 KB, patch)
2023-09-29 19:29 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Add unit test for Koha::Patron::Restriction (2.76 KB, patch)
2023-09-29 19:29 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.34 KB, patch)
2023-09-29 19:35 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Add unit test for Koha::Patron::Restriction (2.76 KB, patch)
2023-09-29 19:36 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.28 KB, patch)
2023-09-29 20:04 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Add unit test for Koha::Patron::Restriction (2.76 KB, patch)
2023-09-29 20:04 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.38 KB, patch)
2023-10-09 09:49 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Add unit test for Koha::Patron::Restriction (2.76 KB, patch)
2023-10-09 09:49 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.38 KB, patch)
2023-10-09 09:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Add unit test for Koha::Patron::Restriction (2.76 KB, patch)
2023-10-09 09:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.43 KB, patch)
2023-10-09 19:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 26053: Add unit test for Koha::Patron::Restriction (2.80 KB, patch)
2023-10-09 19:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 26053: Distinguish expired patron restrictions (3.49 KB, patch)
2023-10-09 19:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26053: Add unit test for Koha::Patron::Restriction (2.86 KB, patch)
2023-10-09 19:49 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2020-07-23 17:42:40 UTC
A library can add a manual restriction to a patrons account with a date. This is the date the restriction will be lifted.  It would be nice to add to the manual that this is controlled by a cron and would need to be activated for this process to work.

--restrictions DAYS   purge patrons restrictions expired since more than DAYS days.
                         Defaults to 30 days if no days specified.
Comment 1 Katrin Fischer 2020-07-24 12:43:30 UTC
Hm, I think the "lifting" works without the cronjob, just on the date. The restriction loses its effect. - The purge is for deleting them so they no longer show up in the account.
Comment 2 Kelly McElligott 2020-07-24 14:31:21 UTC
Katrin, Interesting, I had examples from a library that the restrictions weren't being removed on the date.
Comment 3 Katrin Fischer 2020-07-24 15:13:03 UTC
I think that's where we should probably look then - might be somewhere the check is not working right. But we need more details then.
Comment 4 Katrin Fischer 2020-07-24 15:13:24 UTC
To be clear: they still show, but you should be able to checkout etc.
Comment 5 Kelly McElligott 2020-07-29 12:04:06 UTC
I would propose something to be added to the restriction on the patron detail (member entry) page, like the restriction table to show that the restriction has expired?  Currently, yes after the date, the patron can checkout- the restriction no longer appears on the checkout page.  However it does show on the patron account screen (member entry) and it appears to be active.  A library may want to see expired restrictions, but it would be nice to have this show that it has indeed expired.
Comment 6 Kelly McElligott 2020-07-29 12:05:21 UTC
Created attachment 107488 [details]
member entry page - restriction
Comment 7 Katrin Fischer 2020-07-29 18:50:59 UTC
Hm, maybe it could be greyed out or striked through?
Comment 8 Kelly McElligott 2020-07-30 17:45:15 UTC
Yes - greyed out would be good!
Comment 9 Caroline Cyr La Rose 2023-05-18 18:37:53 UTC
I think this has moved from documentation to enhancement request?
Comment 10 Caroline Cyr La Rose 2023-09-21 18:31:57 UTC
Moving this to Templates component as it is more of a styling thing now.
Comment 11 Fridolin Somers 2023-09-21 22:24:14 UTC
Created attachment 156026 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey
Comment 12 Fridolin Somers 2023-09-21 22:25:22 UTC
Great idea.

Once validated, I'll create a unit test for new method 'is_expired'
Comment 13 Caroline Cyr La Rose 2023-09-22 13:53:13 UTC
Created attachment 156095 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 14 Caroline Cyr La Rose 2023-09-22 13:53:28 UTC
Testing comments

- I wasn't sure if I had to rebuild yarn after applying the patch. I did it anyway since it doesn't hurt anything if it wasn't necessary

- To edit the restriction in the database, it's in the borrower_debarments tables (I couldn't remember and had to search for it).

I did 

select * from borrower_debarments where borrowernumber = 49;

Then chose one of the id's to edit

update borrower_debarments set expiration = '2023-08-23', created = '2023-08-12 09:46:37' where borrower_debarment_id = 25;
Comment 15 David Nind 2023-09-22 15:41:44 UTC
Created attachment 156109 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2023-09-22 15:59:09 UTC
One thing I noticed was that when clicking on the 'View restrictions' button (on either the check out or details tab):

- It opens the Restrictions tab
- But doesn't move you down the page to the actual tab

It's not related to this bug, so I can create another bug for this if this is happening for others as well.
Comment 17 David Nind 2023-09-22 16:28:14 UTC
For some reason, I didn't notice that it had already been signed off!

Bug 34891 created.
Comment 18 Caroline Cyr La Rose 2023-09-22 18:54:29 UTC
(In reply to David Nind from comment #17)
> For some reason, I didn't notice that it had already been signed off!
> 
> Bug 34891 created.

No worries, two pairs of eyes is always better than one!
Comment 19 Emily Lamancusa 2023-09-29 13:39:20 UTC
Nice clarity improvement! is_expired looks good and works as expected - it just needs the unit test.

Also, there's a typo in patron-restrictions-tab.inc:
37	[% IF restriction.is_expired %]<span>(expired)<span>[% END %]
^ The / is missing from the closing </span> tag
Comment 20 Fridolin Somers 2023-09-29 19:29:28 UTC
Created attachment 156423 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey
Comment 21 Fridolin Somers 2023-09-29 19:29:42 UTC
Created attachment 156424 [details] [review]
Bug 26053: Add unit test for Koha::Patron::Restriction

Run:
prove t/db_dependent/Koha/Patron/Restriction.t
Comment 22 Fridolin Somers 2023-09-29 19:35:28 UTC
Created attachment 156425 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey
Comment 23 Fridolin Somers 2023-09-29 19:36:08 UTC
Created attachment 156426 [details] [review]
Bug 26053: Add unit test for Koha::Patron::Restriction

Run:
prove t/db_dependent/Koha/Patron/Restriction.t
Comment 24 Fridolin Somers 2023-09-29 19:38:38 UTC
(In reply to Emily Lamancusa from comment #19)
> Nice clarity improvement! is_expired looks good and works as expected - it
> just needs the unit test.
Here it is :)

> 
> Also, there's a typo in patron-restrictions-tab.inc:
> 37	[% IF restriction.is_expired %]<span>(expired)<span>[% END %]
> ^ The / is missing from the closing </span> tag
Good catch, yayk ;)

Also UT helped me to find an issue with expiration = today, one must use day truncation like in Koha::Patron :
  dt_from_string->truncate( to => 'day' );
Comment 25 Fridolin Somers 2023-09-29 20:02:21 UTC
Ah I found there is a "expired" CSS class already used by additional contents :
https://git.koha-community.org/Koha-community/Koha/src/commit/d9849aaa3aa777072c846526c772a896b5424cfb/koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt#L367

So I change to use it.
Comment 26 Fridolin Somers 2023-09-29 20:04:00 UTC
Also move parenthesis outside of span like in additional contents :
  [% IF restriction.is_expired %](<span>expired</span>)[% END %]
Comment 27 Fridolin Somers 2023-09-29 20:04:44 UTC
Created attachment 156427 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey
Comment 28 Fridolin Somers 2023-09-29 20:04:56 UTC
Created attachment 156428 [details] [review]
Bug 26053: Add unit test for Koha::Patron::Restriction

Run:
prove t/db_dependent/Koha/Patron/Restriction.t
Comment 29 Emily Lamancusa 2023-09-29 21:09:36 UTC
> Also UT helped me to find an issue with expiration = today, one must use day
> truncation like in Koha::Patron :
>   dt_from_string->truncate( to => 'day' );

Hmmm, it looks like Koha::Patron is inconsistent with itself in that.

$patron->is_expired uses:
return 1 if dt_from_string( $self->dateexpiry ) < dt_from_string->truncate( to => 'day' );


$patron->password_expired uses:
return 1 if dt_from_string( $self->password_expiration_date ) <= dt_from_string->truncate( to => 'day' );


$patron->is_debarred uses:
return $self->debarred
      if $self->debarred =~ '^9999'
      or dt_from_string( $self->debarred ) > dt_from_string;


We should probably standardize those on another bug...

I don't think we should be truncating just one, though. Then if the expiration date == today, we're comparing, for instance, '2023-09-29 00:00:00' to '2023-09-29', which should be equal, but won't evaluate to equal. We should truncate either both or neither.


In any case, in this particular situation, the behavior should match the behavior of $patron->is_debarred, since we're looking at restrictions. If the restriction's expiration date is today, it would compare:
'2023-09-29 00:00:00' > '2023-09-29 09:31:23' (or whatever the current time is), which would return false (the patron is not debarred), which means the restriction IS expired. I don't see a coding guideline about whether to truncate or not, so I think that's up to you. To make the behavior consistent with is_debarred, it would need to be either:

dt_from_string( $self->expiration ) < dt_from_string

or

if dt_from_string( $self->expiration )->truncate( to => 'day' ) <= dt_from_string->truncate( to => 'day' )
Comment 30 Fridolin Somers 2023-10-09 09:39:45 UTC
Great thanks for your help

>  In any case, in this particular situation, the behavior should match the behavior of $patron->is_debarred, since we're looking at restrictions.

Sure, makes sens.

Seems Koha::Patron->is_debarred should have a unit test covering yesterday/today/tomorrow.

I fix patches.
Comment 31 Fridolin Somers 2023-10-09 09:49:10 UTC
Created attachment 156725 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey
Comment 32 Fridolin Somers 2023-10-09 09:49:21 UTC
Created attachment 156726 [details] [review]
Bug 26053: Add unit test for Koha::Patron::Restriction

Run:
prove t/db_dependent/Koha/Patron/Restriction.t
Comment 33 Fridolin Somers 2023-10-09 09:53:36 UTC
Created attachment 156727 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey
Comment 34 Fridolin Somers 2023-10-09 09:53:50 UTC
Created attachment 156728 [details] [review]
Bug 26053: Add unit test for Koha::Patron::Restriction

Run:
prove t/db_dependent/Koha/Patron/Restriction.t
Comment 35 Fridolin Somers 2023-10-09 09:56:35 UTC
To be super clear I added a comment and variable :

+    # This condition must be consistent with Koha::Patron->is_debarred
+    my $makes_patron_debarred = dt_from_string( $self->expiration ) > dt_from_string;
+    return 1 unless $makes_patron_debarred;
Comment 36 David Nind 2023-10-09 19:18:10 UTC
Created attachment 156757 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey

Signed-off-by: David Nind <david@davidnind.com>
Comment 37 David Nind 2023-10-09 19:18:13 UTC
Created attachment 156758 [details] [review]
Bug 26053: Add unit test for Koha::Patron::Restriction

Run:
prove t/db_dependent/Koha/Patron/Restriction.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 38 David Nind 2023-10-09 19:31:44 UTC
Note for documentation updates:

1. The change in this bug needs to be incorporated.

2. Add a note (if it doesn't exist already) about the cron job (misc/cronjobs/cleanup_database.pl) that needs to be run to remove expired restrictions. (see #c1)

3. Update the cron job (misc/cronjobs/cleanup_database.pl) information if any updates are required.
Comment 39 Katrin Fischer 2023-10-09 19:49:14 UTC
Created attachment 156762 [details] [review]
Bug 26053: Distinguish expired patron restrictions

On the patron detail page, in restrictions table to show that the restriction has expired.
With text and a grey color line.
Uses 'text-muted' boostrap class (already used in OPAC).

Test plan :
1) Go to a patron details page cgi-bin/koha/members/moremember.pl
2) Create 2 restrictions in the future
3) Edit in database to se the first restriction into the past
4) Create a retriction without date
5) Check you see on expired line text : (expired)
6) Check line is grey

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 Katrin Fischer 2023-10-09 19:49:17 UTC
Created attachment 156763 [details] [review]
Bug 26053: Add unit test for Koha::Patron::Restriction

Run:
prove t/db_dependent/Koha/Patron/Restriction.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 41 Tomás Cohen Arazi 2023-10-10 13:02:15 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 42 Fridolin Somers 2023-10-12 20:52:01 UTC
Enhancement not pushed to 23.05.x
Comment 43 Caroline Cyr La Rose 2023-12-12 22:24:12 UTC
Added details to the manual as per David's comment 38.