The system preference AllowItemsOnHoldCheckout only affects SIP based checkouts. I believe we should rename it AllowItemsOnHoldCheckoutSIP so it matches AllowItemsOnHoldCheckoutSCO. We should also update the description as well. As far as I know AllowItemsOnHoldCheckout has never affected anything except SIP traffic. That does not preclude the addition of another AllowItemsOnHoldCheckout syspref that would affect the staff interface, but that would be an enhancement and is outside the scope of this bug report.
Created attachment 91114 [details] [review] Bug 23233: AllowItemsOnHoldCheckout is misnamed and does not indicate it is for SIP-based checkouts only The system preference AllowItemsOnHoldCheckout only affects SIP based checkouts. I believe we should rename it AllowItemsOnHoldCheckoutSIP so it matches AllowItemsOnHoldCheckoutSCO. We should also update the description as well. As far as I know AllowItemsOnHoldCheckout has never affected anything except SIP traffic. That does not preclude the addition of another AllowItemsOnHoldCheckout syspref that would affect the staff interface, but that would be an enhancement and is outside the scope of this bug report. Test Plan: 1) Test AllowItemsOnHoldCheckout via SIP 2) Apply this patch 3) Test AllowItemsOnHoldCheckoutSIP via SIP 4) Note there is no difference! 5) Note the syspref description has been update to mention SIP
The syspref affects also the code in Koha/Item.pm, function "has_pending_hold". I believe it gets indirectly used in opac/intranet so we can't do this by simply renaming.
(In reply to Joonas Kylmälä from comment #2) > The syspref affects also the code in Koha/Item.pm, function > "has_pending_hold". I believe it gets indirectly used in opac/intranet so we > can't do this by simply renaming. The one and only use of this method is in koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc so supressing the pending hold status here based on the system preference should be considered buggy behavior as well! I'll file a followup to remove the use of the syspref for that method.
Created attachment 91139 [details] [review] Bug 23233: Remove use of AllowItemsOnHoldCheckout from Koha::Item::has_pending_hold, it can cause false data to be returned
(In reply to Kyle M Hall from comment #4) > Created attachment 91139 [details] [review] [review] > Bug 23233: Remove use of AllowItemsOnHoldCheckout from > Koha::Item::has_pending_hold, it can cause false data to be returned Thanks. So just to summarize what this does: In koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc it is shown to patron whether the item is available for checkout and without this patch it actually showed whether it is available through SIP server and didn't consider it could have been available through intranet or SCO. This same bug seems to be in two other lines in the same Koha/Items.pm file: C4/Items.pm: $query .= ",IF(tmp_holdsqueue.itemnumber,1,0) AS has_pending_hold" if !C4::Context->preference('AllowItemsOnHoldCheckout'); C4/Items.pm: LEFT JOIN tmp_holdsqueue USING (itemnumber)" if !C4::Context->preference('AllowItemsOnHoldCheckout'); Shouldn't those also be fixed?
(In reply to Joonas Kylmälä from comment #5) > (In reply to Kyle M Hall from comment #4) > > Created attachment 91139 [details] [review] [review] [review] > > Bug 23233: Remove use of AllowItemsOnHoldCheckout from > > Koha::Item::has_pending_hold, it can cause false data to be returned > > Thanks. So just to summarize what this does: In > koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc it is shown to > patron whether the item is available for checkout and without this patch it > actually showed whether it is available through SIP server and didn't > consider it could have been available through intranet or SCO. This same bug > seems to be in two other lines in the same Koha/Items.pm file: > > C4/Items.pm: $query .= ",IF(tmp_holdsqueue.itemnumber,1,0) AS > has_pending_hold" if !C4::Context->preference('AllowItemsOnHoldCheckout'); > > C4/Items.pm: LEFT JOIN tmp_holdsqueue USING (itemnumber)" if > !C4::Context->preference('AllowItemsOnHoldCheckout'); > > Shouldn't those also be fixed? You are absolutely right! Good catch :) I'll update my followup to include those changes.
Created attachment 91143 [details] [review] Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo
(In reply to Kyle M Hall from comment #7) > Created attachment 91143 [details] [review] [review] > Bug 23233: Remove use of AllowItemsOnHoldCheckout from > C4::Items::GetItemsInfo You accidentally obsoleted the new patch instead of old one?
Created attachment 91144 [details] [review] Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo
(In reply to Joonas Kylmälä from comment #8) > (In reply to Kyle M Hall from comment #7) > > Created attachment 91143 [details] [review] [review] [review] > > Bug 23233: Remove use of AllowItemsOnHoldCheckout from > > C4::Items::GetItemsInfo > > You accidentally obsoleted the new patch instead of old one? I realized it wasn't complete and I decided to keep it separate for clarity. Thanks for checking!
Created attachment 94526 [details] [review] Bug 23233: AllowItemsOnHoldCheckout is misnamed and does not indicate it is for SIP-based checkouts only The system preference AllowItemsOnHoldCheckout only affects SIP based checkouts. I believe we should rename it AllowItemsOnHoldCheckoutSIP so it matches AllowItemsOnHoldCheckoutSCO. We should also update the description as well. As far as I know AllowItemsOnHoldCheckout has never affected anything except SIP traffic. That does not preclude the addition of another AllowItemsOnHoldCheckout syspref that would affect the staff interface, but that would be an enhancement and is outside the scope of this bug report. Test Plan: 1) Test AllowItemsOnHoldCheckout via SIP 2) Apply this patch 3) Test AllowItemsOnHoldCheckoutSIP via SIP 4) Note there is no difference! 5) Note the syspref description has been update to mention SIP Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 94527 [details] [review] Bug 23233: Remove use of AllowItemsOnHoldCheckout from Koha::Item::has_pending_hold, it can cause false data to be returned Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 94528 [details] [review] Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 94529 [details] [review] Bug 23233: (follow-up) Remove accidentally left comment The line t::lib::Mocks::mock_preference( 'AllowItemsOnHoldCheckout', 0 ); was removed from below this comment line but the comment explaining what it does was forgotten.
Created attachment 94530 [details] [review] Bug 23233: (follow-up) Add description to atomic upgrade
Created attachment 94531 [details] [review] Bug 23233: (follow-up) Correct test description AllowItemsOnHoldCheckout is not checked anymore but instead only whether the tmp_holdsqueue is empty or not.
Created attachment 94556 [details] [review] Bug 23233: (follow-up) Remove accidentally left comment The line t::lib::Mocks::mock_preference( 'AllowItemsOnHoldCheckout', 0 ); was removed from below this comment line but the comment explaining what it does was forgotten. Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Created attachment 94557 [details] [review] Bug 23233: (follow-up) Add description to atomic upgrade Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Created attachment 94558 [details] [review] Bug 23233: (follow-up) Correct test description AllowItemsOnHoldCheckout is not checked anymore but instead only whether the tmp_holdsqueue is empty or not. Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Hi Joonas, this one is confusing to me - from the bug title/description it only seemed to affect the description of the pref/naming of the pref, so I expected an easy QA, but there is definitely more going on here. Can you explain why you removed the checks for the prefs in several parts of the code? Especially the changes to GetItemsInfo.
(In reply to Katrin Fischer from comment #20) > Hi Joonas, > > this one is confusing to me - from the bug title/description it only seemed > to affect the description of the pref/naming of the pref, so I expected an > easy QA, but there is definitely more going on here. > > Can you explain why you removed the checks for the prefs in several parts of > the code? Especially the changes to GetItemsInfo. Does the comment #6 explain (and the ones before that)? Kyle, maybe you would like to improve the commit messages?
(In reply to Joonas Kylmälä from comment #21) > (In reply to Katrin Fischer from comment #20) > > Hi Joonas, > > > > this one is confusing to me - from the bug title/description it only seemed > > to affect the description of the pref/naming of the pref, so I expected an > > easy QA, but there is definitely more going on here. > > > > Can you explain why you removed the checks for the prefs in several parts of > > the code? Especially the changes to GetItemsInfo. > > Does the comment #6 explain (and the ones before that)? > > Kyle, maybe you would like to improve the commit messages? I've re-read the commit message and even with the followups if still reads accurately. What happened is that Joonas noticed that some places in Koha would present certain data only if AllowItemsOnHoldCheckoutSIP although that data should be displayed regardless of AllowItemsOnHoldCheckoutSIP!
Switching back to Signed off. Changes make sense to me now, tests pass, database update is ok, but I can't test the SIP part - can someone take over from here?
I do not understand the following change: +++ b/Koha/Item.pm @@ -382,7 +382,7 @@ This method checks the tmp_holdsqueue to see if this item has been selected for sub has_pending_hold { my ( $self ) = @_; my $pending_hold = $self->_result->tmp_holdsqueues; - return !C4::Context->preference('AllowItemsOnHoldCheckout') && $pending_hold->count ? 1: 0; + return $pending_hold->count ? 1: 0; } I also do not understand what it meant originally.
This needs feedback from the author. See previous question of Jonathan. Changing status.
(In reply to Jonathan Druart from comment #24) > I do not understand the following change: > > +++ b/Koha/Item.pm > @@ -382,7 +382,7 @@ This method checks the tmp_holdsqueue to see if this > item has been selected for > sub has_pending_hold { > my ( $self ) = @_; > my $pending_hold = $self->_result->tmp_holdsqueues; > - return !C4::Context->preference('AllowItemsOnHoldCheckout') && > $pending_hold->count ? 1: 0; > + return $pending_hold->count ? 1: 0; > } > > > I also do not understand what it meant originally. This was explained in more detail in the comment number 3. I'm switching this back to Signed Off, OK?
Jonathan confirmed on IRC that it is clear now so QA please review this.
Created attachment 96247 [details] [review] Bug 23233: AllowItemsOnHoldCheckout is misnamed and does not indicate it is for SIP-based checkouts only The system preference AllowItemsOnHoldCheckout only affects SIP based checkouts. I believe we should rename it AllowItemsOnHoldCheckoutSIP so it matches AllowItemsOnHoldCheckoutSCO. We should also update the description as well. As far as I know AllowItemsOnHoldCheckout has never affected anything except SIP traffic. That does not preclude the addition of another AllowItemsOnHoldCheckout syspref that would affect the staff interface, but that would be an enhancement and is outside the scope of this bug report. Test Plan: 1) Test AllowItemsOnHoldCheckout via SIP 2) Apply this patch 3) Test AllowItemsOnHoldCheckoutSIP via SIP 4) Note there is no difference! 5) Note the syspref description has been update to mention SIP Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 96248 [details] [review] Bug 23233: Remove use of AllowItemsOnHoldCheckout from Koha::Item::has_pending_hold, it can cause false data to be returned Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 96249 [details] [review] Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 96250 [details] [review] Bug 23233: (follow-up) Remove accidentally left comment The line t::lib::Mocks::mock_preference( 'AllowItemsOnHoldCheckout', 0 ); was removed from below this comment line but the comment explaining what it does was forgotten. Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 96251 [details] [review] Bug 23233: (follow-up) Add description to atomic upgrade Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 96252 [details] [review] Bug 23233: (follow-up) Correct test description AllowItemsOnHoldCheckout is not checked anymore but instead only whether the tmp_holdsqueue is empty or not. Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I did not test deeply this patchset but the changes make sense to me. A quick note: Koha::Item->has_pending_hold should be replaced by Koha::Item->pending_holds
Nice work everyone! Pushed to master for 20.05
Created attachment 96345 [details] [review] Bug 23233: Fix UsageStats.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 96358 [details] [review] Bug 23233: (RM follow-up) Rename syspref in test The AllowItemsOnHoldCheckout system preference was renamed to AllowItemsOnHoldCheckoutSIP to clarify it's use, but one case was missed in the test suit. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Ooops. Totally missed that you had also provided that exact same followup Jonathan :)
Pushed to 19.11.x branch for 19.11.02
backported to 19.05.x for 19.05.08
Enhancement will not be backported to 18.11.x series.
*** Bug 23223 has been marked as a duplicate of this bug. ***