Bug 25813 - Enhance patron expiration in SIP display
Summary: Enhance patron expiration in SIP display
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matthias Meusburger
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-19 09:07 UTC by Matthias Meusburger
Modified: 2024-03-22 14:09 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 25813: Enhance patron expiration in SIP display (3.33 KB, patch)
2020-06-19 09:08 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 25813: Enhance patron expiration in SIP display (5.65 KB, patch)
2023-08-22 13:59 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 25813: Enhance patron expiration in SIP display (5.83 KB, patch)
2024-01-10 08:06 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 25813: Enhance patron expiration in SIP display (5.88 KB, patch)
2024-01-10 23:54 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 25813: Enhance patron expiration in SIP display (5.95 KB, patch)
2024-01-17 09:53 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2020-06-19 09:07:06 UTC
Currently, the patron information returned by SIP only shows "PATRON EXPIRED" when the patron card has expired.
    
This patch makes the display more consistant with the Opac display and also complies with the NotifyBorrowerDeparture system preference.
Comment 1 Matthias Meusburger 2020-06-19 09:08:06 UTC
Created attachment 106057 [details] [review]
Bug 25813: Enhance patron expiration in SIP display

 Currently, the patron information returned by SIP only shows
 "PATRON EXPIRED" when the patron card has expired.

 This patch makes the display more consistant with the Opac display
 and also complies with the NotifyBorrowerDeparture system preference.

 Test plan:

  - apply the patch

  - set NotifyBorrowerDeparture to 0

  - check that nothing is ever displayed about the card expiration

  - set NotifyBorrowerDeparture to a value greater than 0

  - check that the following message will be displayed for a card that will
    expire within NotifyBorrowerDeparture days:
    "Your card will expire on {correctly formatted date}"

  - check that the following message will be displayed for a card that has
    expired:
    "Your account has expired as of {correctly formatted date}"

 You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.
Comment 2 Matthias Meusburger 2023-08-22 13:59:15 UTC
Created attachment 154697 [details] [review]
Bug 25813: Enhance patron expiration in SIP display

Currently, the patron information returned by SIP only shows
 "PATRON EXPIRED" when the patron card has expired.

 This patch makes the display more consistant with the Opac display
 and also complies with the NotifyBorrowerDeparture system preference.

 Test plan:

  - apply the patch

  - set NotifyBorrowerDeparture to 0

  - check that nothing is ever displayed about the card expiration

  - set NotifyBorrowerDeparture to a value greater than 0

  - check that the following message will be displayed for a card that will
    expire within NotifyBorrowerDeparture days:
    "Your card will expire on {correctly formatted date}"

  - check that the following message will be displayed for a card that has
    expired:
    "Your account has expired as of {correctly formatted date}"

 You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.
Comment 3 Matthias Meusburger 2023-08-22 13:59:46 UTC
The patch has been rebased, and unit tests have been added.
Comment 4 Matthias Le Gac 2024-01-09 21:01:03 UTC
I can't apply le patch 

Applying: Bug 25813: Enhance patron expiration in SIP display
Using index info to reconstruct a base tree...
M	C4/SIP/ILS/Patron.pm
M	t/db_dependent/SIP/Patron.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/SIP/Patron.t
Auto-merging C4/SIP/ILS/Patron.pm
CONFLICT (content): Merge conflict in C4/SIP/ILS/Patron.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 25813: Enhance patron expiration in SIP display
Comment 5 Matthias Meusburger 2024-01-10 08:06:39 UTC
Created attachment 160730 [details] [review]
Bug 25813: Enhance patron expiration in SIP display

Currently, the patron information returned by SIP only shows
 "PATRON EXPIRED" when the patron card has expired.

 This patch makes the display more consistant with the Opac display
 and also complies with the NotifyBorrowerDeparture system preference.

 Test plan:

  - apply the patch

  - set NotifyBorrowerDeparture to 0

  - check that nothing is ever displayed about the card expiration

  - set NotifyBorrowerDeparture to a value greater than 0

  - check that the following message will be displayed for a card that will
    expire within NotifyBorrowerDeparture days:
    "Your card will expire on {correctly formatted date}"

  - check that the following message will be displayed for a card that has
    expired:
    "Your account has expired as of {correctly formatted date}"

 You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.
Comment 6 Matthias Meusburger 2024-01-10 08:07:41 UTC
The patch has been rebased.
Comment 7 Lucas Gass 2024-01-10 23:53:45 UTC
Works well, using sip_cli_emulator.pl to test.
Comment 8 Lucas Gass 2024-01-10 23:54:17 UTC
Created attachment 160794 [details] [review]
Bug 25813: Enhance patron expiration in SIP display

Currently, the patron information returned by SIP only shows
 "PATRON EXPIRED" when the patron card has expired.

 This patch makes the display more consistant with the Opac display
 and also complies with the NotifyBorrowerDeparture system preference.

 Test plan:

  - apply the patch

  - set NotifyBorrowerDeparture to 0

  - check that nothing is ever displayed about the card expiration

  - set NotifyBorrowerDeparture to a value greater than 0

  - check that the following message will be displayed for a card that will
    expire within NotifyBorrowerDeparture days:
    "Your card will expire on {correctly formatted date}"

  - check that the following message will be displayed for a card that has
    expired:
    "Your account has expired as of {correctly formatted date}"

 You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 9 Martin Renvoize 2024-01-17 09:53:47 UTC
Created attachment 161084 [details] [review]
Bug 25813: Enhance patron expiration in SIP display

 Currently, the patron information returned by SIP only shows
 "PATRON EXPIRED" when the patron card has expired.

 This patch makes the display more consistant with the Opac display
 and also complies with the NotifyBorrowerDeparture system preference.

 Test plan:

  - apply the patch

  - set NotifyBorrowerDeparture to 0

  - check that nothing is ever displayed about the card expiration

  - set NotifyBorrowerDeparture to a value greater than 0

  - check that the following message will be displayed for a card that will
    expire within NotifyBorrowerDeparture days:
    "Your card will expire on {correctly formatted date}"

  - check that the following message will be displayed for a card that has
    expired:
    "Your account has expired as of {correctly formatted date}"

 You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2024-01-17 09:54:19 UTC
All working as described, code is tidy, covered by tests and passes QA scripts.

Passing QA
Comment 11 Katrin Fischer 2024-03-22 13:18:56 UTC
I'll push this, but I'd love a follow-up doing some terminology clean-up:

* Debarred = Restricted
* Borrower = Patron

I know we have tons of each, but we decided to move one direction, so let's not move backwards :)
Comment 12 Matthias Meusburger 2024-03-22 13:51:37 UTC
Hi Katrin,

I agree with you about having consistent terminology.

However, NotifyBorrowerDeparture is a pre-existing system preference and is used in other places in Koha. Renaming it to NotifyPatronDeparture should be done in a separate BZ in my opinion.

As for Debarred/Restricted, it is only code comments and a log line, it is never displayed to the user. I tried to be consistent with the Patron object method, which is $patron->is_debarred (and not $patron->is_restricted).

That being said, I would happily change code comments (borrower -> patron, debarred -> restricted) and the log line if you think it's better this way.
Comment 13 Katrin Fischer 2024-03-22 14:01:38 UTC
Maybe I am going a bit far today, but not as far as asking you to rename the pref at least :)
I mostly meant the code comments as it also can get confusing to new devs with our all-over mix of things.
Comment 14 Katrin Fischer 2024-03-22 14:09:06 UTC
Pushed for 24.05!

Well done everyone, thank you!