Bug 7703

Summary: Don't block bulk hold action on search results if some items can't be placed on hold
Product: Koha Reporter: Owen Leonard <oleonard>
Component: Hold requestsAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, kyle, sally.healey, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.04
Bug Depends on: 28273    
Bug Blocks: 9336, 9350    
Attachments: Bug 7703 - Don't block bulk hold action on search results if some items can't be placed on hold
Bug 7703 - Don't block bulk hold action on search results if some items can't be placed on hold
[SIGNED-OFF] Bug 7703 - Don't block bulk hold action if some items can't be placed on hold
Biblio without item
Bug 7703 - Don't block bulk hold action if some items can't be placed on hold
Bug 7703 - QA Followup
Bug 7703 - QA Followup 2
Bug 7703 - Don't block bulk hold action if some items can't be placed on hold
Bug 7703 - QA Followup
Bug 7703 - QA Followup 2
[SIGNED-OFF] Bug 7703 - QA Followup
[SIGNED-OFF] Bug 7703 - QA Followup 2
Bug 7703: Don't block bulk hold action on search results if some items can't be placed on hold
Bug 7703: (QA follow-up)
Bug 7703: (follow-up) Treat no items like none available
Bug 7703: Don't block bulk hold action on search results if some items can't be placed on hold
Bug 7703: (QA follow-up)
Bug 7703: (follow-up) Treat no items like none available
Bug 7703: Don't block bulk hold action on search results if some items can't be placed on hold
Bug 7703: (QA follow-up)
Bug 7703: (follow-up) Treat no items like none available

Description Owen Leonard 2012-03-12 13:02:21 UTC
In the staff client if you check multiple lines in search results and click the button to "place hold" you might be blocked from proceeding because "One or more selected items cannot be placed on hold." The next stage of the holds process is perfectly capable of warning the user about such problems. The user should not have to search back through the list of items they checked to find which one to un-check.
Comment 1 Owen Leonard 2012-05-15 22:06:31 UTC
*** Bug 8097 has been marked as a duplicate of this bug. ***
Comment 2 Kyle M Hall 2012-05-16 12:03:20 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-05-16 12:08:39 UTC
On a tangentially connected note, disabled buttons in Koha appear no different than enabled buttons. We should really have some CSS in Koha to make disabled buttons distinguishable from enabled ones.
Comment 4 Katrin Fischer 2012-05-16 12:44:29 UTC
I think normally we don't show a button, if it is not available. We also hide areas and pages, that are not available because of missing permissions. Links in pull down menus can be greyed out sometimes.
Comment 5 Owen Leonard 2012-07-20 11:58:41 UTC
I'm not sure what problem this patch is trying to address, but it doesn't address the problem I'm trying to describe in this bug. My problem is related to the catalog search results screen. In the staff client, perform a search which will return a result set which includes at least one record with no items. Select all results and click "place hold." The operation will be blocked with the error "One or more selected items cannot be placed on hold." Since request.pl can already sensibly handle multiple holds in this situation it doesn't make sense for the results screen to block the operation.

Marking Failed QA because the patch doesn't match the bug, but perhaps the patch belongs on a new bug with a different description?
Comment 6 Jared Camins-Esakov 2013-01-02 02:41:23 UTC
Kyle, should the patch on this bug be moved to a new bug and the status of this bug changed back to ASSIGNED?
Comment 7 Kyle M Hall 2013-01-02 12:45:32 UTC
(In reply to comment #6)
> Kyle, should the patch on this bug be moved to a new bug and the status of
> this bug changed back to ASSIGNED?

I think we need to move this patch to a new bug.

Kyle
Comment 8 Kyle M Hall 2013-01-02 13:03:43 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2013-01-02 13:23:31 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-01-02 13:24:52 UTC
With the patch, attempting to place a multi-hold where one of the records has no items results in the following message on reserve/request.pl

Cannot place hold: this record has no items attached.

Is this the expected behavior?
Comment 11 Kyle M Hall 2013-01-02 13:29:25 UTC
Test plan:
1) Apply patch
2) On the staff interface, do a search
3) On the search results, select at least one record with items and one record with no items.
4) Click the 'Place hold' button.
5) You should be redirected to reserve/request.pl with the message "Cannot place hold: this record has no items attached."
Comment 12 Owen Leonard 2013-01-04 20:22:26 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2013-01-14 14:53:57 UTC
QA Comment:

I got an error:

DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 3 at /home/koha/src/C4/Koha.pm line 833.

Debug: request.pl l.320, the @itemnumbers is empty

The issue appears when I place on hold a biblio without item.

Marked as Failed QA.
Comment 14 Kyle M Hall 2013-02-12 18:25:15 UTC
Jonathan, I cannot recreate your error. When I try placing a hold on a bib with no items I get the correct behavior as described. Do you think you could provide me with some more info and perhaps re-test it?


(In reply to comment #13)
> QA Comment:
> 
> I got an error:
> 
> DBD::mysql::st execute failed: You have an error in your SQL syntax; check
> the manual that corresponds to your MySQL server version for the right
> syntax to use near ')' at line 3 at /home/koha/src/C4/Koha.pm line 833.
> 
> Debug: request.pl l.320, the @itemnumbers is empty
> 
> The issue appears when I place on hold a biblio without item.
> 
> Marked as Failed QA.
Comment 15 Jonathan Druart 2013-02-13 09:07:22 UTC
Created attachment 15290 [details]
Biblio without item

Hi Kyle,
This biblio contains no item.
When I search 'vifs', this title appears on the first page of results.
I click on the checkbox and on the "place holds' button.
I still got the same error.

reserve/request.pl: I pass into the following if statement:
    if (!@itemnumbers) {
        $template->param('noitems' => 1);
        $biblioloopiter{noitems} = 1;
    }
and just after the GetItemInfosOf routine is called with @itemnumbers which is empty.
I think we want to skip the current biblionumber if it does not contain any item. But maybe we want to raise this problem for the user too.
Comment 16 Kyle M Hall 2013-06-18 12:43:19 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2013-06-18 12:45:09 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-06-18 12:46:17 UTC
I was able to recreate your issue, and I've attached a followup I think will resolve it!

(In reply to comment #15)
> Created attachment 15290 [details]
> Biblio without item
> 
> Hi Kyle,
> This biblio contains no item.
> When I search 'vifs', this title appears on the first page of results.
> I click on the checkbox and on the "place holds' button.
> I still got the same error.
> 
> reserve/request.pl: I pass into the following if statement:
>     if (!@itemnumbers) {
>         $template->param('noitems' => 1);
>         $biblioloopiter{noitems} = 1;
>     }
> and just after the GetItemInfosOf routine is called with @itemnumbers which
> is empty.
> I think we want to skip the current biblionumber if it does not contain any
> item. But maybe we want to raise this problem for the user too.
Comment 19 Jonathan Druart 2013-06-18 20:16:25 UTC
Kyle, thanks for the followup, it fixes the issue.

Owen, why the current behavior is an issue? Is not it better to raise a javascript warning? The current behavior avoid a page to load. Moreover, the message "Cannot place hold: this record has no items attached." appears on a blank page (without back button or something else). I think it is not convincing.

Switch to in discussion (because it is not really a failed qa, but need more explanation).
Comment 20 Owen Leonard 2014-02-19 20:41:08 UTC
(In reply to Jonathan Druart from comment #19)
> Moreover, the
> message "Cannot place hold: this record has no items attached." appears on a
> blank page (without back button or something else).

This issue does make it Failed QA, because the point of the bug is to make it possible to continue to place a hold on those titles which do have items attached even if the user also selected some which have no items.
Comment 21 Kyle M Hall 2014-05-09 16:38:00 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2014-05-09 16:39:38 UTC
I'm getting very strange behavior from the holds system at the moment. Could someone please test with this new followup?

Thanks!
Comment 23 Owen Leonard 2014-08-11 15:00:48 UTC
Applying: Bug 7703 - Don't block bulk hold action if some items can't be placed on hold
Applying: Bug 7703 - QA Followup
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Items.pm
CONFLICT (content): Merge conflict in C4/Items.pm
Auto-merging C4/Biblio.pm
CONFLICT (content): Merge conflict in C4/Biblio.pm
Failed to merge in the changes.
Comment 24 Marc VĂ©ron 2015-12-20 16:01:53 UTC
Still valid?
Comment 25 Owen Leonard 2015-12-21 13:24:21 UTC
This bug is still valid in master as of 21 Dec 2015.
Comment 26 Kyle M Hall 2016-06-07 18:23:09 UTC
Created attachment 52166 [details] [review]
Bug 7703 - Don't block bulk hold action if some items can't be placed on hold

If you select multiple titles on the search results page in order to
place a bulk hold and some of those titles have no items you get a
JavaScript alert warning you can some cannot be placed on hold. You are
blocked from completing the action until you deselect the invalid hold.

This is unnecessary because the bulk hold process will safely refuse to
place a hold on these titles later in the process.

This patch removes the check that prevents submitting a multi-hold if
one or more records in the multi-hold have no items.

Test plan:
1) Apply patch
2) On the staff interface, do a search
3) On the search results, select at least one record with items and one
record with no items.
4) Click the 'Place hold' button.
5) You should be redirected to reserve/request.pl with the message
"Cannot place hold: this record has no items attached."

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Added a description of the problem and copied the test plan from the bug
report, fulfilling commit message guidelines.
Comment 27 Kyle M Hall 2016-06-07 18:23:19 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2016-06-07 18:23:22 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2016-06-08 14:16:15 UTC
Only the first patch has been signed off and it was 3 years ago.
It sounds safe to request another signoff.
Comment 30 Srdjan Jankovic 2016-06-13 03:22:02 UTC
Created attachment 52295 [details] [review]
[SIGNED-OFF] Bug 7703 - QA Followup

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Comment 31 Srdjan Jankovic 2016-06-13 03:23:19 UTC
Created attachment 52296 [details] [review]
[SIGNED-OFF] Bug 7703 - QA Followup 2

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Comment 32 Kyle M Hall 2016-06-27 16:10:04 UTC
Thanks Srdjan!
Comment 33 Katrin Fischer 2016-07-13 20:42:04 UTC
In my testing it allowed me ot place a hold on an a record without items... can you please take a look?

I marked 2 records, one with items and one without. 
I used the 'place hold' button on top of the result list.
Both records showed the same in the screen - both holds were placed.
One record has no items... and a hold now (obviously :) ).
Comment 34 Owen Leonard 2016-08-19 19:13:16 UTC
*** Bug 4249 has been marked as a duplicate of this bug. ***
Comment 35 Owen Leonard 2021-08-06 13:08:00 UTC
Created attachment 123575 [details] [review]
Bug 7703: Don't block bulk hold action on search results if some items can't be placed on hold

If you select multiple titles on the search results page in order to
place a bulk hold and some of those titles have no items you get a
JavaScript alert warning you can some cannot be placed on hold. You are
blocked from completing the action until you deselect the invalid hold.

This is unnecessary because the bulk hold process will safely refuse to
place a hold on these titles later in the process.

This patch removes the check that prevents submitting a multi-hold if
one or more records in the multi-hold have no items.

Test plan:
1) Apply patch
2) On the staff interface, do a search
3) On the search results, select at least one record with items and one
   record with no items.
4) Click the 'Place hold' button.
5) You should be redirected to reserve/request.pl with the message
   "Cannot place hold: this record has no items attached."

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 36 Owen Leonard 2021-08-06 13:08:04 UTC
Created attachment 123576 [details] [review]
Bug 7703: (QA follow-up)

Revised test plan from Owen:

This patch modifies the hold process so that if one of the titles in a
multi-hold process has no items the process doesn't abort completely.

To test, apply the patch and perform a search in the catalog which will
return one or more records with no items attached.

 - Check checkboxes for multiple results, some of which have items and
   at least one of which has no items.
 - Click "Place hold."
 - You should be taken to the page for placing multiple holds, with a
   heading, "Cannot place hold on some items."
 - Note: You will not be able to complete the holds process without the
   next patch.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 37 Owen Leonard 2021-08-06 13:08:08 UTC
Created attachment 123577 [details] [review]
Bug 7703: (follow-up) Treat no items like none available

This patch modifies the holds template so that during the
multi-hold process, titles with no items attached are treated the same
way as titles with no items available (items exist but cannot be place
don hold):

 - The row showing such a record will say "No items are available to be
    placed on hold."
 - The pickup location dropdown will be hidden.

To test, follow the previous test plan and confirm that these change are
reflected. Now that the pickup location field isn't present for titles
without items you should be able to complete the holds process.
Comment 38 Kyle M Hall 2021-08-12 14:06:02 UTC
Created attachment 123812 [details] [review]
Bug 7703: Don't block bulk hold action on search results if some items can't be placed on hold

If you select multiple titles on the search results page in order to
place a bulk hold and some of those titles have no items you get a
JavaScript alert warning you can some cannot be placed on hold. You are
blocked from completing the action until you deselect the invalid hold.

This is unnecessary because the bulk hold process will safely refuse to
place a hold on these titles later in the process.

This patch removes the check that prevents submitting a multi-hold if
one or more records in the multi-hold have no items.

Test plan:
1) Apply patch
2) On the staff interface, do a search
3) On the search results, select at least one record with items and one
   record with no items.
4) Click the 'Place hold' button.
5) You should be redirected to reserve/request.pl with the message
   "Cannot place hold: this record has no items attached."

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 39 Kyle M Hall 2021-08-12 14:06:06 UTC
Created attachment 123813 [details] [review]
Bug 7703: (QA follow-up)

Revised test plan from Owen:

This patch modifies the hold process so that if one of the titles in a
multi-hold process has no items the process doesn't abort completely.

To test, apply the patch and perform a search in the catalog which will
return one or more records with no items attached.

 - Check checkboxes for multiple results, some of which have items and
   at least one of which has no items.
 - Click "Place hold."
 - You should be taken to the page for placing multiple holds, with a
   heading, "Cannot place hold on some items."
 - Note: You will not be able to complete the holds process without the
   next patch.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 40 Kyle M Hall 2021-08-12 14:06:09 UTC
Created attachment 123814 [details] [review]
Bug 7703: (follow-up) Treat no items like none available

This patch modifies the holds template so that during the
multi-hold process, titles with no items attached are treated the same
way as titles with no items available (items exist but cannot be place
don hold):

 - The row showing such a record will say "No items are available to be
    placed on hold."
 - The pickup location dropdown will be hidden.

To test, follow the previous test plan and confirm that these change are
reflected. Now that the pickup location field isn't present for titles
without items you should be able to complete the holds process.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 41 Sally 2021-08-12 14:06:35 UTC
Works well, big improvement - thanks Owen!
Comment 42 Katrin Fischer 2021-08-29 15:21:12 UTC
Maybe something for another bug: we could stop the process, if there are no options for placing holds - like all records selected have no items.
Comment 43 Katrin Fischer 2021-08-29 15:21:47 UTC
Created attachment 124233 [details] [review]
Bug 7703: Don't block bulk hold action on search results if some items can't be placed on hold

If you select multiple titles on the search results page in order to
place a bulk hold and some of those titles have no items you get a
JavaScript alert warning you can some cannot be placed on hold. You are
blocked from completing the action until you deselect the invalid hold.

This is unnecessary because the bulk hold process will safely refuse to
place a hold on these titles later in the process.

This patch removes the check that prevents submitting a multi-hold if
one or more records in the multi-hold have no items.

Test plan:
1) Apply patch
2) On the staff interface, do a search
3) On the search results, select at least one record with items and one
   record with no items.
4) Click the 'Place hold' button.
5) You should be redirected to reserve/request.pl with the message
   "Cannot place hold: this record has no items attached."

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 44 Katrin Fischer 2021-08-29 15:21:51 UTC
Created attachment 124234 [details] [review]
Bug 7703: (QA follow-up)

Revised test plan from Owen:

This patch modifies the hold process so that if one of the titles in a
multi-hold process has no items the process doesn't abort completely.

To test, apply the patch and perform a search in the catalog which will
return one or more records with no items attached.

 - Check checkboxes for multiple results, some of which have items and
   at least one of which has no items.
 - Click "Place hold."
 - You should be taken to the page for placing multiple holds, with a
   heading, "Cannot place hold on some items."
 - Note: You will not be able to complete the holds process without the
   next patch.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 45 Katrin Fischer 2021-08-29 15:21:56 UTC
Created attachment 124235 [details] [review]
Bug 7703: (follow-up) Treat no items like none available

This patch modifies the holds template so that during the
multi-hold process, titles with no items attached are treated the same
way as titles with no items available (items exist but cannot be place
don hold):

 - The row showing such a record will say "No items are available to be
    placed on hold."
 - The pickup location dropdown will be hidden.

To test, follow the previous test plan and confirm that these change are
reflected. Now that the pickup location field isn't present for titles
without items you should be able to complete the holds process.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 46 Jonathan Druart 2021-08-30 15:11:24 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 47 Kyle M Hall 2021-09-03 14:50:17 UTC
Pushed to 21.05.x for 21.05.04
Comment 48 Fridolin Somers 2021-09-08 20:33:49 UTC
Does not apply on 20.11.x because patches apply on code of Bug 28273