Bug 7703 - Don't block bulk hold action on search results if some items can't be placed on hold
Summary: Don't block bulk hold action on search results if some items can't be placed ...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Keywords:
: 4249 (view as bug list)
Depends on:
Blocks: 9336 9350
  Show dependency treegraph
 
Reported: 2012-03-12 13:02 UTC by Owen Leonard
Modified: 2016-12-01 21:57 UTC (History)
6 users (show)

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


Attachments
Bug 7703 - Don't block bulk hold action on search results if some items can't be placed on hold (2.56 KB, patch)
2012-05-16 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7703 - Don't block bulk hold action on search results if some items can't be placed on hold (1.69 KB, patch)
2013-01-02 13:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7703 - Don't block bulk hold action if some items can't be placed on hold (2.60 KB, patch)
2013-01-04 20:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Biblio without item (951 bytes, application/octet-stream)
2013-02-13 09:07 UTC, Jonathan Druart
Details
Bug 7703 - Don't block bulk hold action if some items can't be placed on hold (3.30 KB, patch)
2013-06-18 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7703 - QA Followup (943 bytes, patch)
2013-06-18 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7703 - QA Followup 2 (1.88 KB, patch)
2014-05-09 16:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7703 - Don't block bulk hold action if some items can't be placed on hold (2.55 KB, patch)
2016-06-07 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7703 - QA Followup (986 bytes, patch)
2016-06-07 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7703 - QA Followup 2 (1.88 KB, patch)
2016-06-07 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7703 - QA Followup (1.01 KB, patch)
2016-06-13 03:22 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7703 - QA Followup 2 (1.93 KB, patch)
2016-06-13 03:23 UTC, Srdjan Jankovic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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. ***