Creating an ILL request, when staff adds a patron, there is no indication that the patron is expired, if said patron is indeed expired. Part of the problem tackling this, I believe, is that the 'create form' is ILL backend specific, so this should be tackled at each ILL backend level. Having said that, with bug 35570 in the pipeline, we could perhaps consider adding this little enhancement as a follow-up to that.
Created attachment 171516 [details] [review] Bug 36454: Add 'expired' information on patron auto complete results Test plan: 1) Alter the 'Expiry date' of any patron. 2) Search for that patron on any search input that provides auto complete results (e.g. top 'Search patrons' input) 3) Notice the expired patron has a 'expired' indication. Those who aren't do not.
Widening the scope here to provide 'expired' information on the patron search results including, but not limited to, when placing an ILL request. Do others agree with this?
Created attachment 171630 [details] Add restricted status too?
Created attachment 171631 [details] [review] Bug 36454: Add 'expired' information on patron auto complete results Test plan: 1) Alter the 'Expiry date' of any patron. 2) Search for that patron on any search input that provides auto complete results (e.g. top 'Search patrons' input) 3) Notice the expired patron has a 'expired' indication. Those who aren't do not. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 171632 [details] [review] Bug 36454: (follow-up) Tweak CSS and add restricted status output This patch makes some tweaks to the style of the new information: We can use the Bootstrap 5 "badge" class and "warning" style to get the appearance we want. This patch also adds output of the patron's restricted status if present. To test, apply the patch and rebuild the staff interface CSS. Perform a patron search (from the checkout header search form for instance) and confirm that a "restricted" badge appears alongside the branch and expired badge.
Created attachment 171633 [details] [review] Bug 36454: (follow-up) Tweak CSS and add restricted status output This patch makes some tweaks to the style of the new information: We can use the Bootstrap 5 "badge" class and "warning" style to get the appearance we want. This patch also adds output of the patron's restricted status if present. To test, apply the patch and rebuild the staff interface CSS. Perform a patron search (from the checkout header search form for instance) and confirm that a "restricted" badge appears alongside the branch and expired badge. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 171839 [details] [review] Bug 36454: Add 'expired' information on patron auto complete results Test plan: 1) Alter the 'Expiry date' of any patron. 2) Search for that patron on any search input that provides auto complete results (e.g. top 'Search patrons' input) 3) Notice the expired patron has a 'expired' indication. Those who aren't do not. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 171840 [details] [review] Bug 36454: (follow-up) Tweak CSS and add restricted status output This patch makes some tweaks to the style of the new information: We can use the Bootstrap 5 "badge" class and "warning" style to get the appearance we want. This patch also adds output of the patron's restricted status if present. To test, apply the patch and rebuild the staff interface CSS. Perform a patron search (from the checkout header search form for instance) and confirm that a "restricted" badge appears alongside the branch and expired badge. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Pushed for 24.11! Well done everyone, thank you!
Nice idea! I wonder if we could think of a way to display this information in patron search as well. For example we could turn the expiration date red if it was in the past. As color alone is not accessible, another visible indicator like an icon might be needed.
(In reply to Katrin Fischer from comment #10) > Nice idea! > > I wonder if we could think of a way to display this information in patron > search as well. For example we could turn the expiration date red if it was > in the past. As color alone is not accessible, another visible indicator > like an icon might be needed. The patrons table list results, correct? I think that's a nice idea, we should log a bug to spark discussion / verify interest.
(In reply to Pedro Amorim from comment #11) > (In reply to Katrin Fischer from comment #10) > > Nice idea! > > > > I wonder if we could think of a way to display this information in patron > > search as well. For example we could turn the expiration date red if it was > > in the past. As color alone is not accessible, another visible indicator > > like an icon might be needed. > > The patrons table list results, correct? I think that's a nice idea, we > should log a bug to spark discussion / verify interest. Yep. Just hard to keep track of all lose ends so wanted to store in a comment at least.
This patch breaks curbside pickup, please have a look at bug 38465.
A couple of remarks/questions: 1. If we have a Koha::Object's method to calculate the "is expired" status, we should not have a JS counter part (at least in this situation). What if the calculation change? 2. Date.parse($date_to_rfc3339($date(new_date.toString()))); This sounds way too complicated, why is that needed?
(In reply to Jonathan Druart from comment #13) > This patch breaks curbside pickup, please have a look at bug 38465. Added a patch there.
(In reply to Jonathan Druart from comment #14) > A couple of remarks/questions: > > 1. If we have a Koha::Object's method to calculate the "is expired" status, > we should not have a JS counter part (at least in this situation). What if > the calculation change? > > 2. Date.parse($date_to_rfc3339($date(new_date.toString()))); > This sounds way too complicated, why is that needed? 3. From Koha::Patron->is_expired:897 return 0 unless $self->dateexpiry; This is not taken into account in the JS code. What happens if dateexpiry is NULL in DB?
(In reply to Jonathan Druart from comment #16) > (In reply to Jonathan Druart from comment #14) > > A couple of remarks/questions: > > > > 1. If we have a Koha::Object's method to calculate the "is expired" status, > > we should not have a JS counter part (at least in this situation). What if > > the calculation change? > > > > 2. Date.parse($date_to_rfc3339($date(new_date.toString()))); > > This sounds way too complicated, why is that needed? > > 3. From Koha::Patron->is_expired:897 return 0 unless $self->dateexpiry; > This is not taken into account in the JS code. What happens if dateexpiry is > NULL in DB? The search breaks, see bug 38254
Created attachment 174671 [details] [review] Bug 36454: (QA follow-up) Use native is_expired instead of redundant JS calculation Test plan: 1) $ yarn api:bundle 2) $ koha-plack --restart kohadev 3) Make a patron expired, search for them. Verify the 'expired' badge still shows
(In reply to Jonathan Druart from comment #16) I agree with everything you said. Code was not great. I've submitted a follow-up.
Created attachment 174682 [details] [review] Bug 36454: (QA follow-up): Selenium - Add regression tests Test plan: 1) $ ktd --selenium up 2) prove t/db_dependent/selenium/patrons_search_badges.t Ensure these tests pass before and after the previous 'Use native is_expired instead of redundant JS calculation' patch.
Created attachment 174684 [details] [review] Bug 36454: (QA follow-up) Use native is_expired instead of redundant JS calculation Test plan: 1) $ yarn api:bundle 2) $ koha-plack --restart kohadev 3) Make a patron expired, search for them. Verify the 'expired' badge still shows Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 174685 [details] [review] Bug 36454: (QA follow-up): Selenium - Add regression tests Test plan: 1) $ ktd --selenium up 2) prove t/db_dependent/selenium/patrons_search_badges.t Ensure these tests pass before and after the previous 'Use native is_expired instead of redundant JS calculation' patch. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 174686 [details] [review] Bug 36454: Fix and add API tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 174687 [details] [review] Bug 36454: Selenium - test patrons must start with 'test_' I usually > delete from borrowers where surname like "test_" So I do not need to reset_all Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I think "is_expired" would have been better than "expired". But we already have "restricted", so better to keep the consistency I guess... Could adjust both later if needed.
Picked 4 follow-up patches starting with "Use native is_expired..." for push to main.