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. Katrin, Interesting, I had examples from a library that the restrictions weren't being removed on the date. 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. To be clear: they still show, but you should be able to checkout etc. 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. Created attachment 107488 [details]
member entry page - restriction
Hm, maybe it could be greyed out or striked through? Yes - greyed out would be good! I think this has moved from documentation to enhancement request? Moving this to Templates component as it is more of a styling thing now. 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 Great idea. Once validated, I'll create a unit test for new method 'is_expired' 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> 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; 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> 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. For some reason, I didn't notice that it had already been signed off! Bug 34891 created. (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! 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 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 Created attachment 156424 [details] [review] Bug 26053: Add unit test for Koha::Patron::Restriction Run: prove t/db_dependent/Koha/Patron/Restriction.t 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 Created attachment 156426 [details] [review] Bug 26053: Add unit test for Koha::Patron::Restriction Run: prove t/db_dependent/Koha/Patron/Restriction.t (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' ); 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. Also move parenthesis outside of span like in additional contents : [% IF restriction.is_expired %](<span>expired</span>)[% END %] 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 Created attachment 156428 [details] [review] Bug 26053: Add unit test for Koha::Patron::Restriction Run: prove t/db_dependent/Koha/Patron/Restriction.t > 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' )
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.
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 Created attachment 156726 [details] [review] Bug 26053: Add unit test for Koha::Patron::Restriction Run: prove t/db_dependent/Koha/Patron/Restriction.t 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 Created attachment 156728 [details] [review] Bug 26053: Add unit test for Koha::Patron::Restriction Run: prove t/db_dependent/Koha/Patron/Restriction.t 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; 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> 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> 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. 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> 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> Pushed to master for 23.11. Nice work everyone, thanks! Enhancement not pushed to 23.05.x Added details to the manual as per David's comment 38. |
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.