Bug 23233 - AllowItemsOnHoldCheckout is misnamed and should only work for for SIP-based checkouts
Summary: AllowItemsOnHoldCheckout is misnamed and should only work for for SIP-based c...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 21572
  Show dependency treegraph
 
Reported: 2019-06-28 17:19 UTC by Kyle M Hall
Modified: 2020-02-02 20:51 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.02, 19.05.08


Attachments
Bug 23233: AllowItemsOnHoldCheckout is misnamed and does not indicate it is for SIP-based checkouts only (7.91 KB, patch)
2019-06-28 18:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23233: Remove use of AllowItemsOnHoldCheckout from Koha::Item::has_pending_hold, it can cause false data to be returned (2.10 KB, patch)
2019-07-01 11:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo (1.76 KB, patch)
2019-07-01 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo (3.04 KB, patch)
2019-07-01 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23233: AllowItemsOnHoldCheckout is misnamed and does not indicate it is for SIP-based checkouts only (8.03 KB, patch)
2019-10-22 11:33 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23233: Remove use of AllowItemsOnHoldCheckout from Koha::Item::has_pending_hold, it can cause false data to be returned (2.23 KB, patch)
2019-10-22 11:33 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo (3.17 KB, patch)
2019-10-22 11:33 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23233: (follow-up) Remove accidentally left comment (1.14 KB, patch)
2019-10-22 11:33 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23233: (follow-up) Add description to atomic upgrade (898 bytes, patch)
2019-10-22 11:33 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23233: (follow-up) Correct test description (1.11 KB, patch)
2019-10-22 11:33 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23233: (follow-up) Remove accidentally left comment (1.20 KB, patch)
2019-10-22 13:24 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 23233: (follow-up) Add description to atomic upgrade (954 bytes, patch)
2019-10-22 13:24 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 23233: (follow-up) Correct test description (1.17 KB, patch)
2019-10-22 13:25 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 23233: AllowItemsOnHoldCheckout is misnamed and does not indicate it is for SIP-based checkouts only (8.16 KB, patch)
2019-12-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23233: Remove use of AllowItemsOnHoldCheckout from Koha::Item::has_pending_hold, it can cause false data to be returned (2.30 KB, patch)
2019-12-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo (3.24 KB, patch)
2019-12-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23233: (follow-up) Remove accidentally left comment (1.27 KB, patch)
2019-12-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23233: (follow-up) Add description to atomic upgrade (1.00 KB, patch)
2019-12-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23233: (follow-up) Correct test description (1.24 KB, patch)
2019-12-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23233: Fix UsageStats.t (897 bytes, patch)
2019-12-16 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23233: (RM follow-up) Rename syspref in test (1.03 KB, patch)
2019-12-17 09:09 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 Kyle M Hall 2019-06-28 17:19:26 UTC
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.
Comment 1 Kyle M Hall 2019-06-28 18:14:33 UTC
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
Comment 2 Joonas Kylmälä 2019-07-01 09:09:37 UTC
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.
Comment 3 Kyle M Hall 2019-07-01 11:13:09 UTC
(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.
Comment 4 Kyle M Hall 2019-07-01 11:20:21 UTC
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
Comment 5 Joonas Kylmälä 2019-07-01 11:50:31 UTC
(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?
Comment 6 Kyle M Hall 2019-07-01 12:19:22 UTC
(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.
Comment 7 Kyle M Hall 2019-07-01 12:45:17 UTC
Created attachment 91143 [details] [review]
Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo
Comment 8 Joonas Kylmälä 2019-07-01 12:50:01 UTC
(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?
Comment 9 Kyle M Hall 2019-07-01 12:56:42 UTC
Created attachment 91144 [details] [review]
Bug 23233: Remove use of AllowItemsOnHoldCheckout from C4::Items::GetItemsInfo
Comment 10 Kyle M Hall 2019-07-01 13:07:10 UTC
(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!
Comment 11 Joonas Kylmälä 2019-10-22 11:33:08 UTC
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>
Comment 12 Joonas Kylmälä 2019-10-22 11:33:12 UTC
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>
Comment 13 Joonas Kylmälä 2019-10-22 11:33:15 UTC
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>
Comment 14 Joonas Kylmälä 2019-10-22 11:33:18 UTC
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.
Comment 15 Joonas Kylmälä 2019-10-22 11:33:22 UTC
Created attachment 94530 [details] [review]
Bug 23233: (follow-up) Add description to atomic upgrade
Comment 16 Joonas Kylmälä 2019-10-22 11:33:26 UTC
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.
Comment 17 Lari Taskula 2019-10-22 13:24:40 UTC
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>
Comment 18 Lari Taskula 2019-10-22 13:24:55 UTC
Created attachment 94557 [details] [review]
Bug 23233: (follow-up) Add description to atomic upgrade

Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 19 Lari Taskula 2019-10-22 13:25:13 UTC
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>
Comment 20 Katrin Fischer 2019-11-01 21:19:40 UTC
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.
Comment 21 Joonas Kylmälä 2019-11-04 08:50:48 UTC
(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?
Comment 22 Kyle M Hall 2019-11-04 10:56:44 UTC
(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!
Comment 23 Katrin Fischer 2019-11-24 11:54:11 UTC
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?
Comment 24 Jonathan Druart 2019-11-25 14:57:50 UTC
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.
Comment 25 Marcel de Rooy 2019-12-13 10:38:58 UTC
This needs feedback from the author. See previous question of Jonathan. Changing status.
Comment 26 Joonas Kylmälä 2019-12-13 10:51:25 UTC
(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?
Comment 27 Joonas Kylmälä 2019-12-13 12:02:08 UTC
Jonathan confirmed on IRC that it is clear now so QA please review this.
Comment 28 Jonathan Druart 2019-12-13 14:25:22 UTC
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>
Comment 29 Jonathan Druart 2019-12-13 14:25:26 UTC
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>
Comment 30 Jonathan Druart 2019-12-13 14:25:30 UTC
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>
Comment 31 Jonathan Druart 2019-12-13 14:25:34 UTC
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>
Comment 32 Jonathan Druart 2019-12-13 14:25:37 UTC
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>
Comment 33 Jonathan Druart 2019-12-13 14:25:41 UTC
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>
Comment 34 Jonathan Druart 2019-12-13 14:28:04 UTC
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
Comment 35 Martin Renvoize 2019-12-16 11:07:06 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 36 Jonathan Druart 2019-12-16 15:06:27 UTC
Created attachment 96345 [details] [review]
Bug 23233: Fix UsageStats.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 37 Martin Renvoize 2019-12-17 09:09:13 UTC
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>
Comment 38 Martin Renvoize 2019-12-17 09:09:31 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 39 Martin Renvoize 2019-12-17 09:12:38 UTC
Ooops. Totally missed that you had also provided that exact same followup Jonathan :)
Comment 40 Joy Nelson 2020-01-15 00:20:11 UTC
Pushed to 19.11.x branch for 19.11.02
Comment 41 Lucas Gass 2020-01-31 21:01:44 UTC
backported to 19.05.x for 19.05.08
Comment 42 Hayley Mapley 2020-02-02 20:51:11 UTC
Enhancement will not be backported to 18.11.x series.