Bug 29116 - request.pl re-invents Koha::Patron::is_expired accessor
Summary: request.pl re-invents Koha::Patron::is_expired accessor
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Joonas Kylmälä
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-26 16:09 UTC by Joonas Kylmälä
Modified: 2021-10-22 13:55 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 29116: Use a pre-existing accessor instead of reimplementing it (3.39 KB, patch)
2021-09-26 16:15 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29116: Use a pre-existing accessor instead of reimplementing it (3.45 KB, patch)
2021-09-28 07:22 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29116: Use a pre-existing accessor instead of reimplementing it (3.54 KB, patch)
2021-10-07 06:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-09-26 16:09:07 UTC
reserve/request.pl duplicates the same code as what Koha::Patron::is_expired does, the code should be deleted.
Comment 1 Joonas Kylmälä 2021-09-26 16:15:57 UTC
Created attachment 125341 [details] [review]
Bug 29116: Use a pre-existing accessor instead of reimplementing it

We can use the Koha::Patron::is_expired accessor instead of re-writing
it in request.pl.

To test:
 1) Make that a warning is being shown with and without this patch
    when placing a hold to an expired patron account
Comment 2 Fridolin Somers 2021-09-28 07:22:11 UTC
Created attachment 125391 [details] [review]
Bug 29116: Use a pre-existing accessor instead of reimplementing it

We can use the Koha::Patron::is_expired accessor instead of re-writing
it in request.pl.

To test:
 1) Make that a warning is being shown with and without this patch
    when placing a hold to an expired patron account

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 3 Fridolin Somers 2021-09-28 08:08:44 UTC
Always better not to re-invent the wheel :D
Comment 4 Marcel de Rooy 2021-10-07 06:54:10 UTC
Created attachment 125860 [details] [review]
Bug 29116: Use a pre-existing accessor instead of reimplementing it

We can use the Koha::Patron::is_expired accessor instead of re-writing
it in request.pl.

To test:
 1) Make that a warning is being shown with and without this patch
    when placing a hold to an expired patron account

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Jonathan Druart 2021-10-07 09:06:44 UTC
Same file:

266         my $expiry_date = $enrollment->patron->dateexpiry;
267         $member->{expiry} = 0; # flag set if patron account has expired
268         if ($expiry_date and
269             Date_to_Days(split /-/,$date) > Date_to_Days(split /-/,$expiry_date)) {
270             $member->{expiry} = 1;
271         }

Can we fix that one as well please?
Comment 6 Joonas Kylmälä 2021-10-10 08:48:57 UTC
(In reply to Jonathan Druart from comment #5)
> Same file:
> 
> 266         my $expiry_date = $enrollment->patron->dateexpiry;
> 267         $member->{expiry} = 0; # flag set if patron account has expired
> 268         if ($expiry_date and
> 269             Date_to_Days(split /-/,$date) > Date_to_Days(split
> /-/,$expiry_date)) {
> 270             $member->{expiry} = 1;
> 271         }
> 
> Can we fix that one as well please?

There's actually more to it, I had opened earlier early a separate bug report for that with a patch: bug 29115. I'm putting this back to PQA.
Comment 7 Jonathan Druart 2021-10-12 09:18:59 UTC
Pushed to master for 21.11, thanks to everybody involved!