Bug 26139 - 'Place hold' button isn't hidden in all detail views if there are no items available for loan
Summary: 'Place hold' button isn't hidden in all detail views if there are no items av...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 23166
Blocks: 28286
  Show dependency treegraph
 
Reported: 2020-08-05 06:43 UTC by Didier Gautheron
Modified: 2021-12-13 21:12 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 26139: Centralize code for "Place hold" button (detail) (7.34 KB, patch)
2020-08-07 10:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26139: Centralize code for "Place hold" button (detail) (8.08 KB, patch)
2020-08-13 06:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26139: Centralize code for "Place hold" button (detail) (8.18 KB, patch)
2020-08-17 07:01 UTC, Didier Gautheron
Details | Diff | Splinter Review
Bug 26139: Koha::Template::Plugin::Context (2.16 KB, patch)
2020-08-24 07:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26139: Centralize code for "Place hold" button (detail) (8.10 KB, patch)
2020-08-24 08:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26139: Centralize code for "Place hold" button (detail) (8.29 KB, patch)
2020-08-25 09:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Didier Gautheron 2020-08-05 06:43:51 UTC
At cgi-bin/koha/catalogue/detail.pl 'Place Hold' button is not hidden in :
ISBD an Items view
Comment 1 Jonathan Druart 2020-08-07 10:18:06 UTC
Created attachment 107949 [details] [review]
Bug 26139: Centralize code for "Place hold" button (detail)

There is a "norequest" boolean passed to the include cat-toolbar.inc, to
display or not the "Pace hold" button.
This flag was not calculated in some place (ISBDdetail and moredetail
for instance)

Here we centralize the code to make it more robust and less regression
prone.

Note that the same problem appears at the OPAC (opac-MARCdetail is
always display the button). That will have to be fixed separately

Test plan:
Create biblio A with 0 item, B and C with 1 item and D with 2 items
item for B is not for loan, C is for loan and D has 1 of each
Go to the bibliographic detail page of each record and confirm that the
"Place hold" button appears appropriately.
Test the different tabs of the catalogue module
Comment 2 Jonathan Druart 2020-08-07 10:18:41 UTC
Depending on bug 23166 for Context.Scalar
Comment 3 Didier Gautheron 2020-08-12 19:08:46 UTC
 
A with 0 item, button is there ! BAD
B with 1 item not for loan , no button GOOD
C with 1 item for loan, button is there GOOD
D with 2 items, only one for loan, button is there GOOD
Comment 4 Jonathan Druart 2020-08-13 06:35:11 UTC
Created attachment 108159 [details] [review]
Bug 26139: Centralize code for "Place hold" button (detail)

There is a "norequest" boolean passed to the include cat-toolbar.inc, to
display or not the "Pace hold" button.
This flag was not calculated in some place (ISBDdetail and moredetail
for instance)

Here we centralize the code to make it more robust and less regression
prone.

Note that the same problem appears at the OPAC (opac-MARCdetail is
always display the button). That will have to be fixed separately

Test plan:
Create biblio A with 0 item, B and C with 1 item and D with 2 items
item for B is not for loan, C is for loan and D has 1 of each
Go to the bibliographic detail page of each record and confirm that the
"Place hold" button appears appropriately.
Test the different tabs of the catalogue module

QA note: This patch is only centralizing the existing code, but it is
still too naive. To manage the visibility of this button we certainly
want to copy what is done in C4::Search::searchResults:
  # can place a hold on a item if
  # not lost nor withdrawn
  # not damaged unless AllowHoldsOnDamagedItems is true
  # item is either for loan or on order (notforloan < 0)
  $can_place_holds = 1
    if (
         !$item->{itemlost}
      && !$item->{withdrawn}
      && ( !$item->{damaged} || C4::Context->preference('AllowHoldsOnDamagedItems') )
      && ( !$item->{notforloan} || $item->{notforloan} < 0 )
    );
Comment 5 Didier Gautheron 2020-08-17 07:01:16 UTC
Created attachment 108379 [details] [review]
Bug 26139: Centralize code for "Place hold" button (detail)

There is a "norequest" boolean passed to the include cat-toolbar.inc, to
display or not the "Pace hold" button.
This flag was not calculated in some place (ISBDdetail and moredetail
for instance)

Here we centralize the code to make it more robust and less regression
prone.

Note that the same problem appears at the OPAC (opac-MARCdetail is
always display the button). That will have to be fixed separately

Test plan:
Create biblio A with 0 item, B and C with 1 item and D with 2 items
item for B is not for loan, C is for loan and D has 1 of each
Go to the bibliographic detail page of each record and confirm that the
"Place hold" button appears appropriately.
Test the different tabs of the catalogue module

QA note: This patch is only centralizing the existing code, but it is
still too naive. To manage the visibility of this button we certainly
want to copy what is done in C4::Search::searchResults:
  # can place a hold on a item if
  # not lost nor withdrawn
  # not damaged unless AllowHoldsOnDamagedItems is true
  # item is either for loan or on order (notforloan < 0)
  $can_place_holds = 1
    if (
         !$item->{itemlost}
      && !$item->{withdrawn}
      && ( !$item->{damaged} || C4::Context->preference('AllowHoldsOnDamagedItems') )
      && ( !$item->{notforloan} || $item->{notforloan} < 0 )
    );

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 6 Katrin Fischer 2020-08-22 22:42:14 UTC
This depends on bug 23166 that hasn't been signed off yet - moving to BLOCKED. Please bring it back into the QA queue once the dependency is resolved.

This patch also does no longer apply.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 26139: Centralize code for "Place hold" button (detail)
Using index info to reconstruct a base tree...
M	t/db_dependent/Koha/Items.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Items.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Items.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 26139: Centralize code for "Place hold" button (detail)
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-26139-Centralize-code-for-Place-hold-button-de-zcySqH.patch
Comment 7 Jonathan Druart 2020-08-24 07:59:58 UTC
Created attachment 108965 [details] [review]
Bug 26139: Koha::Template::Plugin::Context

There is something wrong with our ->search method and TT behaviours, it
is hard to retrieve object list in a scalar context.
If [% objects.method.count %] is called, objects.method will get the
first object of the list and count will explode (Koha::Object->count
does not exist)

We need to force the call in a scalar context to retrieve an iterator
and prevent to fetch all the objects (we could have called all then
size, but it's not efficient)
Comment 8 Jonathan Druart 2020-08-24 08:00:22 UTC
This patch removes the dependency on bug 23166.
Comment 9 Jonathan Druart 2020-08-24 08:00:44 UTC
Created attachment 108966 [details] [review]
Bug 26139: Centralize code for "Place hold" button (detail)

There is a "norequest" boolean passed to the include cat-toolbar.inc, to
display or not the "Pace hold" button.
This flag was not calculated in some place (ISBDdetail and moredetail
for instance)

Here we centralize the code to make it more robust and less regression
prone.

Note that the same problem appears at the OPAC (opac-MARCdetail is
always display the button). That will have to be fixed separately

Test plan:
Create biblio A with 0 item, B and C with 1 item and D with 2 items
item for B is not for loan, C is for loan and D has 1 of each
Go to the bibliographic detail page of each record and confirm that the
"Place hold" button appears appropriately.
Test the different tabs of the catalogue module

QA note: This patch is only centralizing the existing code, but it is
still too naive. To manage the visibility of this button we certainly
want to copy what is done in C4::Search::searchResults:
  # can place a hold on a item if
  # not lost nor withdrawn
  # not damaged unless AllowHoldsOnDamagedItems is true
  # item is either for loan or on order (notforloan < 0)
  $can_place_holds = 1
    if (
         !$item->{itemlost}
      && !$item->{withdrawn}
      && ( !$item->{damaged} || C4::Context->preference('AllowHoldsOnDamagedItems') )
      && ( !$item->{notforloan} || $item->{notforloan} < 0 )
    );

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 10 Joonas Kylmälä 2020-08-24 10:43:48 UTC
Moving back to "Needs Signoff" since the patch "Bug 26139: Koha::Template::Plugin::Context" is not reviewed yet by anybody.
Comment 11 Jonathan Druart 2020-08-24 10:58:34 UTC
(In reply to Joonas Kylmälä from comment #10)
> Moving back to "Needs Signoff" since the patch "Bug 26139:
> Koha::Template::Plugin::Context" is not reviewed yet by anybody.

It has been tested by Didier when he first tested this patch.
Koha::Template::Plugin::Context is coming from another bug report, I isolated it to note depend on another bug report.
Comment 12 Joonas Kylmälä 2020-08-24 12:16:00 UTC
I get the following error with biblio without items when I click the ISBD view in biblio page in staff interface:

> Template process failed: undef error - Can't call method "filter_by_for_loan" on unblessed reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 43.
> at /kohadevbox/koha/C4/Templates.pm line 122
>         C4::Templates::output('C4::Templates=HASH(0x56350c1d2188)') called at /kohadevbox/koha/catalogue/moredetail.pl line 301
>         eval {...} at /kohadevbox/koha/catalogue/moredetail.pl line 2
>         CGI::Compile::ROOT::kohadevbox_koha_catalogue_moredetail_2epl::__ANON__('CGI::Compile=HASH(0x56350a3aafa0)', '', '/kohadevbox/koha/catalogue/moredetail.pl', '/kohadevbox/koha/catalogue', 'ARRAY(0x56350b321fb0)') called at /usr/share/perl5/CGI/Compile.pm line 139
Comment 13 Jonathan Druart 2020-08-24 13:40:40 UTC
Re-adding the dependency, we need it for some minor refactoring, not only the TT plugin.
Comment 14 Jonathan Druart 2020-08-24 13:42:03 UTC
Blocked by bug 23166.
Comment 15 Joonas Kylmälä 2020-08-25 09:11:04 UTC
Created attachment 109057 [details] [review]
Bug 26139: Centralize code for "Place hold" button (detail)

There is a "norequest" boolean passed to the include cat-toolbar.inc, to
display or not the "Pace hold" button.
This flag was not calculated in some place (ISBDdetail and moredetail
for instance)

Here we centralize the code to make it more robust and less regression
prone.

Note that the same problem appears at the OPAC (opac-MARCdetail is
always display the button). That will have to be fixed separately

Test plan:
Create biblio A with 0 item, B and C with 1 item and D with 2 items
item for B is not for loan, C is for loan and D has 1 of each
Go to the bibliographic detail page of each record and confirm that the
"Place hold" button appears appropriately.
Test the different tabs of the catalogue module

QA note: This patch is only centralizing the existing code, but it is
still too naive. To manage the visibility of this button we certainly
want to copy what is done in C4::Search::searchResults:
  # can place a hold on a item if
  # not lost nor withdrawn
  # not damaged unless AllowHoldsOnDamagedItems is true
  # item is either for loan or on order (notforloan < 0)
  $can_place_holds = 1
    if (
         !$item->{itemlost}
      && !$item->{withdrawn}
      && ( !$item->{damaged} || C4::Context->preference('AllowHoldsOnDamagedItems') )
      && ( !$item->{notforloan} || $item->{notforloan} < 0 )
    );

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 16 Jonathan Druart 2020-08-25 13:16:29 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 17 Lucas Gass 2020-09-04 19:51:05 UTC
missing dependencies for 20.05.x, no backport