|
Description
Nick Clemens (kidclamp)
2022-05-25 13:23:25 UTC
Created attachment 135340 [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 Created attachment 135341 [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 Created attachment 135342 [details] [review] Bug 30847: Remove unused OPACShowHoldQueueDetails Search for the variables, they aren't used! Created attachment 135343 [details] [review] Bug 30847: Don't fetch patron twice We already have a patron objetc, no need to fetch Created attachment 135344 [details] [review] Bug 30847: Remove %itemInfoHash This is not used git grep itemInfoHash only 2 occurences - definition and population - never used Created attachment 135345 [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 Created attachment 135346 [details] [review] Bug 30847: Don't declrae undef variable for assignment New holds have found set to undef - we can do this directly Created attachment 135347 [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 si9mply fetch the biblio once to calculate no need to fetch all the info as we don't display or use it Created attachment 135348 [details] [review] Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot This wil save us fetching more info if we are denying the holds 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)
Nick, I am cleaning a lot on bug 27272 already! Is this still to be tested or is it a duplicate or bug 27272? (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 Created attachment 137668 [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 Created attachment 137669 [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 Created attachment 137670 [details] [review] Bug 30847: Remove unused OPACShowHoldQueueDetails Search for the variables, they aren't used! Created attachment 137671 [details] [review] Bug 30847: Don't fetch patron twice Created attachment 137672 [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 Created attachment 137673 [details] [review] Bug 30847: Don't declrae undef variable for assignment New holds have found set to undef - we can do this directly Created attachment 137674 [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 si9mply fetch the biblio once to calculate no need to fetch all the info as we don't display or use it Created attachment 137675 [details] [review] Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot This wil save us fetching more info if we are denying the holds 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. (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 ;-) 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. Created attachment 138654 [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 si9mply fetch the biblio once to calculate no need to fetch all the info as we don't display or use it Created attachment 138655 [details] [review] Bug 30847: Consolidate code to check if patron can place holds and exit if they cannot This wil save us fetching more info if we are denying the holds 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> 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> 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> Created attachment 138937 [details] [review] Bug 30847: Don't fetch patron twice Signed-off-by: Owen Leonard <oleonard@myacpl.org> 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> 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> 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> 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> Needs a rebase due to the changes introduced in the dependency bug 31314 after these patches were made. 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> 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> 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> 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> 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> 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> 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> 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> Here is a rebase. Placing hold from staff and OPAC still works. Changing priorities and marking as waiting still works. 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 139923 [details] [review] Bug 30847: Remove unused variable Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Passing QA. Added one more patch, the last one, to remove an unused variable in the opac-reserve.pl file. Pushed to master for 22.11. Nice work everyone, thanks! Missing dependency for 22.05.x, no backport |