Bug 30847

Summary: Cleanup opac-reserve.pl
Product: Koha Reporter: Nick Clemens <nick>
Component: OPACAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: normal    
Priority: P5 - low CC: caroline.cyr-la-rose, jonathan.druart, joonas.kylmala, lucas, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on: 31314    
Bug Blocks:    
Attachments: Bug 30847: Cleanup calculation of holds and rank
Bug 30847: Remove unused noreserve parameter
Bug 30847: Remove unused OPACShowHoldQueueDetails
Bug 30847: Don't fetch patron twice
Bug 30847: Remove %itemInfoHash
Bug 30847: Avoid fetching biblio object
Bug 30847: Don't declrae undef variable for assignment
Bug 30847: Don't get biblio/item info when placing holds
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot
Bug 30847: Cleanup calculation of holds and rank
Bug 30847: Remove unused noreserve parameter
Bug 30847: Remove unused OPACShowHoldQueueDetails
Bug 30847: Don't fetch patron twice
Bug 30847: Avoid fetching biblio object
Bug 30847: Don't declrae undef variable for assignment
Bug 30847: Don't get biblio/item info when placing holds
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot
Bug 30847: Don't get biblio/item info when placing holds
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot
Bug 30847: Cleanup calculation of holds and rank
Bug 30847: Remove unused noreserve parameter
Bug 30847: Remove unused OPACShowHoldQueueDetails
Bug 30847: Don't fetch patron twice
Bug 30847: Avoid fetching biblio object
Bug 30847: Don't declare undef variable for assignment
Bug 30847: Don't get biblio/item info when placing holds
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot
Bug 30847: Cleanup calculation of holds and rank
Bug 30847: Remove unused noreserve parameter
Bug 30847: Remove unused OPACShowHoldQueueDetails
Bug 30847: Don't fetch patron twice
Bug 30847: Avoid fetching biblio object
Bug 30847: Don't declare undef variable for assignment
Bug 30847: Don't get biblio/item info when placing holds
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot
Bug 30847: Cleanup calculation of holds and rank
Bug 30847: Remove unused noreserve parameter
Bug 30847: Remove unused OPACShowHoldQueueDetails
Bug 30847: Don't fetch patron twice
Bug 30847: Avoid fetching biblio object
Bug 30847: Don't declare undef variable for assignment
Bug 30847: Don't get biblio/item info when placing holds
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot
Bug 30847: Remove unused variable

Description Nick Clemens 2022-05-25 13:23:25 UTC

    
Comment 1 Nick Clemens 2022-05-25 15:09:43 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2022-05-25 15:09:46 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2022-05-25 15:09:50 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2022-05-25 15:09:53 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2022-05-25 15:09:56 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2022-05-25 15:10:00 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2022-05-25 15:10:04 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2022-05-25 15:10:08 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2022-05-25 15:10:11 UTC Comment hidden (obsolete)
Comment 10 Nick Clemens 2022-05-25 15:12:53 UTC
Largely these changes should be small enough to read code for approval

To test:
1 - Apply patch
2 - Confirm placing holds on OPAC works
3 - Confirm patron blocked when:
    marked 'gone no address'
    marked 'lost'
    has a restriction
    is expired and expired patrons are blocked
    has outstanding fines above maxoutstanding
    has too many holds (more than maxreserves)
Comment 11 Jonathan Druart 2022-06-08 12:32:45 UTC
Nick, I am cleaning a lot on bug 27272 already!
Comment 12 Caroline Cyr La Rose 2022-06-30 15:46:49 UTC
Is this still to be tested or is it a duplicate or bug 27272?
Comment 13 Caroline Cyr La Rose 2022-06-30 15:47:10 UTC
(In reply to Caroline Cyr La Rose from comment #12)
> Is this still to be tested or is it a duplicate or bug 27272?

*of
Comment 14 Jonathan Druart 2022-07-13 06:40:54 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2022-07-13 06:40:59 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2022-07-13 06:41:03 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2022-07-13 06:41:07 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2022-07-13 06:41:11 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2022-07-13 06:41:16 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2022-07-13 06:41:20 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2022-07-13 06:41:24 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2022-07-13 06:42:04 UTC
Hi Nick, I have rebased your patches on top of bug 27272, hope you don't mind!
I can also mark myself as assignee of the patches and continue their integration if you like.
Comment 23 Nick Clemens 2022-07-14 11:18:50 UTC
(In reply to Jonathan Druart from comment #22)
> Hi Nick, I have rebased your patches on top of bug 27272, hope you don't
> mind!
> I can also mark myself as assignee of the patches and continue their
> integration if you like.

Thanks! I do not mind at all ;-)
Comment 24 Victor Grousset/tuxayo 2022-08-05 02:32:50 UTC
Patch doesn't apply and doesn't even make conflicts, just fails. Why does applying patches regularly does this?

Applying: Bug 30847: Don't get biblio/item info when placing holds
error: sha1 information is lacking or useless (opac/opac-reserve.pl).
error: could not build fake ancestor
Patch failed at 0001 Bug 30847: Don't get biblio/item info when placing holds

note: dependency bug 27272 is applied


patch -p1 < /tmp/Bug-30847-Dont-get-biblioitem-info-when-placing-ho-giEvEm.patch 
patching file opac/opac-reserve.pl
Hunk #1 FAILED at 130.
1 out of 5 hunks FAILED -- saving rejects to file opac/opac-reserve.pl.rej

I never did this but it seems theoretically possible to manually apply(copy paste lines!!!) the failed hunk but it would be quite tedious and unreliable.

Jonathan: if you don't have time to rebase I can do it, just push your local version to a branch so I can use that and get conflict instead of this terrible error.
Comment 25 Jonathan Druart 2022-08-05 07:30:42 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2022-08-05 07:30:47 UTC Comment hidden (obsolete)
Comment 27 Owen Leonard 2022-08-09 17:32:32 UTC
Created attachment 138934 [details] [review]
Bug 30847: Cleanup calculation of holds and rank

Currently we loop over all the holds on the record to get a count of the reserves not
waiting and a rank for the upcoming hold

We can simply count the number of nont waiting holds and add 1

Note: Before this patch the count of holds included the new hold we were going to place
which meant rank and count were the same. I believe countaing existing holds and our
upcoming rank is the correct behavior

To test:
 1 - Set OPACShowHoldQueueDetails system preference to 'Show holds and their priorty level'
 2 - Place a few holds on a bib on the staff side
 3 - Mark 1 waiting
 4 - Attempt to place hold on the bib on the opac
 5 - Note 'Your priority' and 'Number of holds' are the same number
 6 - Place another hold, confirm both increase
 7 - Set another hold waiting, confirm both decrease
 8 - Apply patch
 9 - Confirm 'Your priority' is now 1 greater than Number of holds
10 - Place a hold, set a hold waiting, confirm numbers adjust as appropriate

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 28 Owen Leonard 2022-08-09 17:32:36 UTC
Created attachment 138935 [details] [review]
Bug 30847: Remove unused noreserve parameter

This is defined but never used

To test:
1 - git grep noreserve
2 - only one ocurrence
3 - Apply patch
4 - repeat, no ocurrence
5 - Note the plural has a few uses

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 29 Owen Leonard 2022-08-09 17:32:41 UTC
Created attachment 138936 [details] [review]
Bug 30847: Remove unused OPACShowHoldQueueDetails

Search for the variables, they aren't used!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 30 Owen Leonard 2022-08-09 17:32:45 UTC
Created attachment 138937 [details] [review]
Bug 30847: Don't fetch patron twice

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 31 Owen Leonard 2022-08-09 17:32:48 UTC
Created attachment 138938 [details] [review]
Bug 30847: Avoid fetching biblio object

Here the items biblionumber is either equal to the biblionumber passed
in or it is not and should be - we can just assign directly from the
item in all cases

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 32 Owen Leonard 2022-08-09 17:32:52 UTC
Created attachment 138939 [details] [review]
Bug 30847: Don't declare undef variable for assignment

New holds have found set to undef - we can do this directly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 33 Owen Leonard 2022-08-09 17:32:57 UTC
Created attachment 138940 [details] [review]
Bug 30847: Don't get biblio/item info when placing holds

The only bit we get form the hash is the rank and title
We can simply fetch the biblio once to calculate
no need to fetch all the info as we don't display or use it

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 34 Owen Leonard 2022-08-09 17:33:00 UTC
Created attachment 138941 [details] [review]
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot

This will save us fetching more info if we are denying the holds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 35 Joonas Kylmälä 2022-08-21 09:08:53 UTC
Needs a rebase due to the changes introduced in the dependency bug 31314 after these patches were made.
Comment 36 Victor Grousset/tuxayo 2022-08-23 00:17:43 UTC
Created attachment 139628 [details] [review]
Bug 30847: Cleanup calculation of holds and rank

Currently we loop over all the holds on the record to get a count of the reserves not
waiting and a rank for the upcoming hold

We can simply count the number of nont waiting holds and add 1

Note: Before this patch the count of holds included the new hold we were going to place
which meant rank and count were the same. I believe countaing existing holds and our
upcoming rank is the correct behavior

To test:
 1 - Set OPACShowHoldQueueDetails system preference to 'Show holds and their priorty level'
 2 - Place a few holds on a bib on the staff side
 3 - Mark 1 waiting
 4 - Attempt to place hold on the bib on the opac
 5 - Note 'Your priority' and 'Number of holds' are the same number
 6 - Place another hold, confirm both increase
 7 - Set another hold waiting, confirm both decrease
 8 - Apply patch
 9 - Confirm 'Your priority' is now 1 greater than Number of holds
10 - Place a hold, set a hold waiting, confirm numbers adjust as appropriate

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 37 Victor Grousset/tuxayo 2022-08-23 00:17:48 UTC
Created attachment 139629 [details] [review]
Bug 30847: Remove unused noreserve parameter

This is defined but never used

To test:
1 - git grep noreserve
2 - only one ocurrence
3 - Apply patch
4 - repeat, no ocurrence
5 - Note the plural has a few uses

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 38 Victor Grousset/tuxayo 2022-08-23 00:17:54 UTC
Created attachment 139630 [details] [review]
Bug 30847: Remove unused OPACShowHoldQueueDetails

Search for the variables, they aren't used!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 39 Victor Grousset/tuxayo 2022-08-23 00:17:58 UTC
Created attachment 139631 [details] [review]
Bug 30847: Don't fetch patron twice

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 40 Victor Grousset/tuxayo 2022-08-23 00:18:05 UTC
Created attachment 139632 [details] [review]
Bug 30847: Avoid fetching biblio object

Here the items biblionumber is either equal to the biblionumber passed
in or it is not and should be - we can just assign directly from the
item in all cases

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 41 Victor Grousset/tuxayo 2022-08-23 00:18:12 UTC
Created attachment 139633 [details] [review]
Bug 30847: Don't declare undef variable for assignment

New holds have found set to undef - we can do this directly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 42 Victor Grousset/tuxayo 2022-08-23 00:18:18 UTC
Created attachment 139634 [details] [review]
Bug 30847: Don't get biblio/item info when placing holds

The only bit we get form the hash is the rank and title
We can simply fetch the biblio once to calculate
no need to fetch all the info as we don't display or use it

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 43 Victor Grousset/tuxayo 2022-08-23 00:18:25 UTC
Created attachment 139635 [details] [review]
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot

This will save us fetching more info if we are denying the holds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 44 Victor Grousset/tuxayo 2022-08-23 00:25:41 UTC
Here is a rebase.

Placing hold from staff and OPAC still works. Changing priorities and marking as waiting still works.
Comment 45 Joonas Kylmälä 2022-08-28 14:24:39 UTC
Created attachment 139915 [details] [review]
Bug 30847: Cleanup calculation of holds and rank

Currently we loop over all the holds on the record to get a count of the reserves not
waiting and a rank for the upcoming hold

We can simply count the number of nont waiting holds and add 1

Note: Before this patch the count of holds included the new hold we were going to place
which meant rank and count were the same. I believe countaing existing holds and our
upcoming rank is the correct behavior

To test:
 1 - Set OPACShowHoldQueueDetails system preference to 'Show holds and their priorty level'
 2 - Place a few holds on a bib on the staff side
 3 - Mark 1 waiting
 4 - Attempt to place hold on the bib on the opac
 5 - Note 'Your priority' and 'Number of holds' are the same number
 6 - Place another hold, confirm both increase
 7 - Set another hold waiting, confirm both decrease
 8 - Apply patch
 9 - Confirm 'Your priority' is now 1 greater than Number of holds
10 - Place a hold, set a hold waiting, confirm numbers adjust as appropriate

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 46 Joonas Kylmälä 2022-08-28 14:24:44 UTC
Created attachment 139916 [details] [review]
Bug 30847: Remove unused noreserve parameter

This is defined but never used

To test:
1 - git grep noreserve
2 - only one ocurrence
3 - Apply patch
4 - repeat, no ocurrence
5 - Note the plural has a few uses

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 47 Joonas Kylmälä 2022-08-28 14:24:48 UTC
Created attachment 139917 [details] [review]
Bug 30847: Remove unused OPACShowHoldQueueDetails

Search for the variables, they aren't used!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 48 Joonas Kylmälä 2022-08-28 14:24:52 UTC
Created attachment 139918 [details] [review]
Bug 30847: Don't fetch patron twice

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 49 Joonas Kylmälä 2022-08-28 14:24:57 UTC
Created attachment 139919 [details] [review]
Bug 30847: Avoid fetching biblio object

Here the items biblionumber is either equal to the biblionumber passed
in or it is not and should be - we can just assign directly from the
item in all cases

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 50 Joonas Kylmälä 2022-08-28 14:25:02 UTC
Created attachment 139920 [details] [review]
Bug 30847: Don't declare undef variable for assignment

New holds have found set to undef - we can do this directly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 51 Joonas Kylmälä 2022-08-28 14:25:07 UTC
Created attachment 139921 [details] [review]
Bug 30847: Don't get biblio/item info when placing holds

The only bit we get form the hash is the rank and title
We can simply fetch the biblio once to calculate
no need to fetch all the info as we don't display or use it

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 52 Joonas Kylmälä 2022-08-28 14:25:12 UTC
Created attachment 139922 [details] [review]
Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot

This will save us fetching more info if we are denying the holds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 53 Joonas Kylmälä 2022-08-28 14:25:17 UTC
Created attachment 139923 [details] [review]
Bug 30847: Remove unused variable

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 54 Joonas Kylmälä 2022-08-28 14:26:06 UTC
Passing QA. Added one more patch, the last one, to remove an unused variable in the opac-reserve.pl file.
Comment 55 Tomás Cohen Arazi 2022-08-31 11:52:06 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 56 Lucas Gass 2022-10-31 17:59:23 UTC
Missing dependency for 22.05.x, no backport