Bug 14739

Summary: Add several patrons to a new batch will create a new batch per patron added
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Label/patron card printingAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P3 CC: barton, costalc, gwilliams, Heinrich.Hartl, josef.moravec, katrin.fischer, liz, molinga, nengard, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14123
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13891
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12911
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14131
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15224    
Attachments: Obsoleted patch copied from bug 12911
Selection links are missing
Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
[Signed-off] Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
[SIGNED-OFF] Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
Bug 14739: Fix plack compatibility
[SIGNED-OFF] Bug 14739: Fix plack compatibility
Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
Bug 14739: Fix plack compatibility
Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
Bug 14739: Fix plack compatibility
Bug 14739 - Add several patrons to a new batch will create a new batch per patron added
Bug 14739: Fix plack compatibility
Bug 14739: Adapt wording in breadcrumbs if a new batch is created
[3.18.x rewrite] Bug 14739 - Add several patrons to a new batch will create a new batch per patron added

Description Jonathan Druart 2015-08-27 08:28:43 UTC
From bug 14123 comment 4

Patron's Batches can only have a single patron added when making a new "batch". Clicking the "Add" text next to a name keeps placing it behind the pop-up, but replacing the previously clicked user.
Comment 1 Jonathan Druart 2015-08-27 08:52:10 UTC
Actually it's even worst, a new batch is created for each user you add.

Note that this bug existed before bug 13891 (so in 3.18.x)
Comment 2 Jonathan Druart 2015-08-27 09:05:12 UTC
Introduced by

commit 2dd97f26c1e72bc35e29b1afd1fc38d301c48e9e
    Bug 12911: batch_id for new labels batches can be asssigned to several batches


exists since 3.18.7.
Comment 3 Nick Clemens 2015-08-27 12:23:07 UTC
*** Bug 14700 has been marked as a duplicate of this bug. ***
Comment 4 Nick Clemens 2015-08-27 12:30:41 UTC
I tried an easy fix (update batch_id after adding an item), but if a user clicks on several patrons to add without waiting for background page to reload the batch_id doesn't have time to update.

Barton said he is taking a look today and I will touch base with him when I get in this afternoon
Comment 5 Jonathan Druart 2015-08-27 12:40:30 UTC
I think the easier and cleaner way to do would be to create (and so get an id) the batch when the "Add items" button is clicked.

It's not perfect, but I don't see another way.
Comment 6 Nick Clemens 2015-09-05 16:25:02 UTC Comment hidden (obsolete)
Comment 7 Claudio Costales 2015-09-07 23:46:18 UTC
Also "Select", "Select all", "Select none" checkbox and links are missing. See attached image.
Comment 8 Claudio Costales 2015-09-07 23:49:28 UTC
Created attachment 42445 [details]
Selection links are missing
Comment 9 Nick Clemens 2015-09-10 10:45:35 UTC Comment hidden (obsolete)
Comment 10 Nick Clemens 2015-09-10 11:07:41 UTC
*** Bug 14800 has been marked as a duplicate of this bug. ***
Comment 11 Marc Véron 2015-09-10 11:50:36 UTC Comment hidden (obsolete)
Comment 12 Marc Véron 2015-09-10 11:58:05 UTC
BTW: A couple of months ago, I implemented a possibility to print patron cards from patronlists. Patron lists are easier to create and to handle.

See Bug 14131 (Needs sign-off)
Comment 13 Marc Véron 2015-09-10 16:00:20 UTC
*** Bug 14123 has been marked as a duplicate of this bug. ***
Comment 14 Jonathan Druart 2015-09-14 14:24:19 UTC
Why do you use "barcode" when manipulating "borrowernumber"?

Did you note that the same issue appears for labels?
Comment 15 Nick Clemens 2015-09-16 20:43:38 UTC Comment hidden (obsolete)
Comment 16 Josef Moravec 2015-09-25 19:37:05 UTC
I have problem on numbers 6-7 of test plan, when I entered invalid borrowernumbers, the new batch was created, but it wasn't empty, but there are exactly the same borrowers as in previous batch... they were somehow copied from the first batch to second one...

Then I tried to delete four items of this batch, with success, then I tried to add valid number and it was added ok, but again all items from first batch was added too...

on label batches this works correctly
Comment 17 Nick Clemens 2015-09-30 00:59:08 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2015-09-30 01:03:13 UTC
(In reply to Josef Moravec from comment #16)
> I have problem on numbers 6-7 of test plan, 

I couldn't recreate, but I added a safeguard against there being items saved in batch 0 which could have caused the error.  Can you retest and let me know if it happens again?

I also cleaned up some debug code I found left behind.
Comment 19 Josef Moravec 2015-09-30 09:06:01 UTC
I'm sorry but I still have problems.... 

Try this:

1. Go to patron label creator -> manage batches
2. Remove all batches (just to be sure we have the same environment)
3. Click on "new batch"
4. Click on "add items" and pick 5 patrons
5. Close the patron picker window and click on "add items" again, the batch is created as expected
6. Go back to manage batches
7. Click on "new batch"
8. insert one right borrowernumber and one wrong borrowernumber
9. click on "add items"
10. the one right is added, but the five patrons from previous batch is added too
Comment 20 Nick Clemens 2015-09-30 20:08:51 UTC
(In reply to Josef Moravec from comment #19)
> I'm sorry but I still have problems.... 
> 

I tested again in my VM and on a sandbox and cannot recreate.  What environment are you testing in?
Comment 21 Josef Moravec 2015-09-30 20:14:29 UTC
I have VM with Debian Jessie, but with MariaDB instead of MySQL... I'll try it on another machine with MySQL tomorow...
Comment 22 Jonathan Druart 2015-10-01 10:23:57 UTC
(In reply to Josef Moravec from comment #21)
> I have VM with Debian Jessie, but with MariaDB instead of MySQL... I'll try
> it on another machine with MySQL tomorow...

Using MariaDB I don't get the issue you have described.
Comment 23 Josef Moravec 2015-10-01 10:36:33 UTC
Hm, on another machine everything works perfect... So probably some problem with my specific data, or something on that one VM. So please, ignore me ;)
Comment 24 Jonathan Druart 2015-10-01 11:14:43 UTC
Josef, if you are still able to reproduce the issue, with the last patches against master, I think we should try to understand what is the problem :)
Could you confirm you got the issue even with a non-empty creator_batches table? I have not tried with en empty table.
Comment 25 Josef Moravec 2015-10-01 11:42:56 UTC
I tried it on non-empty table, but have the same issue... I am not able to figure out where is the problem....
Comment 26 Barton Chittenden 2015-10-01 16:46:58 UTC
Getting

Software error:

Can't call method "get_attr" without a package or object reference at /home/barton/kohaclone/patroncards/edit-batch.pl line 125.

To re-create:

1/ go to Home › Tools › Patron card creator › Manage card Batches (cgi-bin/koha/patroncards/manage.pl?card_element=batch)
2/ click 'New Bath' (cgi-bin/koha/patroncards/edit-batch.pl?op=new)
3/ click 'Add items' (opens new window -- cgi-bin/koha/patroncards/add_user_search.pl)
4/ click 'Search' to show all patrons (I don't have that many)
5/ click 'Add' on first patron -> Software error.
Comment 27 Barton Chittenden 2015-10-01 20:32:02 UTC
(In reply to Barton Chittenden from comment #26)
> Getting
> 
> Software error:
> 
> Can't call method "get_attr" without a package or object reference at
> /home/barton/kohaclone/patroncards/edit-batch.pl line 125.
> 
> To re-create:
> 
> 1/ go to Home › Tools › Patron card creator › Manage card Batches
> (cgi-bin/koha/patroncards/manage.pl?card_element=batch)
> 2/ click 'New Bath' (cgi-bin/koha/patroncards/edit-batch.pl?op=new)
> 3/ click 'Add items' (opens new window --
> cgi-bin/koha/patroncards/add_user_search.pl)
> 4/ click 'Search' to show all patrons (I don't have that many)
> 5/ click 'Add' on first patron -> Software error.

This was user error on my part.
Comment 28 Nick Clemens 2015-10-01 21:03:36 UTC Comment hidden (obsolete)
Comment 29 Nick Clemens 2015-10-01 21:48:50 UTC Comment hidden (obsolete)
Comment 30 Barton Chittenden 2015-10-01 22:15:48 UTC Comment hidden (obsolete)
Comment 31 Barton Chittenden 2015-10-01 22:20:13 UTC
(In reply to Barton Chittenden from comment #30)
> Created attachment 43040 [details] [review] [review]
> [SIGNED-OFF] Bug 14739 - Add several patrons to a new batch will create a
> new batch per patron added
> 
> This patch changes the functionality of adding patrons to patroncard
> batches.  Instead of using only the patron search, there is now a textarea
> which can either be manually populated with borrowernumbers, or can be
> populated using the patron-search
> 
> To test:
> 1 - Create a new patron batch
> 2 - Click the add items button, if there is no data in the borrowrnumber
> form you should get a popup
> 3 - Add items to batch using the popup, borrowernumbers should populate to
> the form
> 4 - Click add items again and items should be added to a new batch
> 5 - Create a new batch
> 6 - Manually enter invalid borrowernumbers to the form
> 7 - Click add items, batch should return empty
> 8 - Manually enter valid borrowernumber
> 9 - Click add items, batch should be updated
> 10 - Edit an existing batch and confirm add items works as above
> 11 - Create a new label batch
> 12 - Click add items with no data, popup item search should appear
> 13 - Adding items by checking boxes or using 'Add' button - they should
> populate the text box on the edit batch page
> 14 - When adding using form the 'Enter by itemnumber' button should be
> marked automatically
> 15 - Submit items and confirm they are added to batch
> 16 - Create a new batch, enter by barcode manually and check 'Enter by
> itemnumber"
> 17 - Click add items, no items should be added
> 18 - Repopulate form with barcodes, check 'Enter by barcode'
> 19 - Click add items, items should be added correctly
> 20 - Test various combinations of radio buttons and numbers and using form
> or not
> 
> Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>

I've tested without error, and I'm signing off, but I know that Jonathan wants to see if we can replicate the errors that Josef Moravec was seeing, so I'm not going to set the status of the bug as a whole to 'Signed Off'. This is simply saying 'the patch works for me'.
Comment 32 Josef Moravec 2015-10-02 05:52:49 UTC
Now I now where is the difference. It's plack!

When I stop using plack, everything works as expected.
Comment 33 Jonathan Druart 2015-10-02 08:52:02 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2015-10-02 08:52:28 UTC
(In reply to Josef Moravec from comment #32)
> Now I now where is the difference. It's plack!
> 
> When I stop using plack, everything works as expected.

hehe, thanks Josef!
Here is a patch!
Comment 35 Josef Moravec 2015-10-02 09:22:36 UTC Comment hidden (obsolete)
Comment 36 Josef Moravec 2015-10-02 09:23:57 UTC
It works like a charm ;) 

Thank you Jonathan
Comment 37 Jonathan Druart 2015-10-02 09:36:34 UTC
There is something wrong in the first patch
 59 my @item_numbers; #= $cgi->param('item_number') if $cgi->param('item_number');

Could someone re-upload a valid patch?
Comment 38 Josef Moravec 2015-10-02 09:38:49 UTC
Sorry, I did a mistake with obsoleting patches, now it should be OK, could you try it again please?
Comment 39 Jonathan Druart 2015-10-02 11:39:09 UTC
The second patch does not apply anymore, but maybe it was not based on the good one.
Comment 40 Josef Moravec 2015-10-02 12:34:30 UTC Comment hidden (obsolete)
Comment 41 Josef Moravec 2015-10-02 12:34:44 UTC Comment hidden (obsolete)
Comment 42 Josef Moravec 2015-10-02 12:37:56 UTC
I rebased the patches in my local branch, could you please check if it's OK?
Comment 43 Jonathan Druart 2015-10-02 12:43:40 UTC
From the first patch:
-my $batch_id = $cgi->param('element_id') || $cgi->param('batch_id') || undef;
+my $batch_id = $cgi->param('element_id') || $cgi->param('batch_id') || 0;

From the second patch:
-my $batch_id = $cgi->param('element_id') || $cgi->param('batch_id') || 0;
+my $batch_id = $cgi->param('element_id') || $cgi->param('batch_id') || undef;

Initially my patch (the second one) didn't modify this line.
Comment 44 Josef Moravec 2015-10-02 12:57:33 UTC Comment hidden (obsolete)
Comment 45 Josef Moravec 2015-10-02 12:57:43 UTC Comment hidden (obsolete)
Comment 46 Josef Moravec 2015-10-02 12:59:20 UTC
Is that better? I am sorry for this inconvenience...
Comment 47 Jonathan Druart 2015-10-05 08:21:22 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2015-10-05 08:21:26 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2015-10-05 08:21:29 UTC Comment hidden (obsolete)
Comment 50 Tomás Cohen Arazi 2015-10-06 14:03:48 UTC
Patches pushed to master.

Thanks Nick!
Comment 51 Liz Rea 2015-10-08 01:18:23 UTC
Hi,

If you feel like it, a version of this for 3.18 would be pretty nice, as this one doesn't apply cleanly.

(Maybe I'll have a look, but don't count on it!)

Cheers,
Liz
Comment 52 Liz Rea 2015-10-08 20:42:14 UTC
This change might require documentation updates.
Comment 53 Nick Clemens 2015-11-09 17:46:28 UTC
Created attachment 44663 [details] [review]
[3.18.x rewrite] Bug 14739 - Add several patrons to a new batch will create a new batch per patron added

This patch changes the functionality of adding patrons to patroncard batches.  Instead of using only the patron search, there is now a textarea which can either be manually populated with borrowernumbers, or can be populated using the patron-search

To test:
1 - Create a new patron batch
2 - Click the add items button, if there is no data in the borrowrnumber form you should get a popup
3 - Add items to batch using the popup, borrowernumbers should populate to the form
4 - Click add items again and items should be added to a new batch
5 - Create a new batch
6 - Manually enter invalid borrowernumbers to the form
7 - Click add items, batch should return empty
8 - Manually enter valid borrowernumber
9 - Click add items, batch should be updated
10 - Edit an existing batch and confirm add items works as above
11 - Create a new label batch
12 - Click add items with no data, popup item search should appear
13 - Adding items by checking boxes or using 'Add' button - they should populate the text box on the edit batch page
14 - When adding using form the 'Enter by itemnumber' button should be marked automatically
15 - Submit items and confirm they are added to batch
16 - Create a new batch, enter by barcode manually and check 'Enter by itemnumber"
17 - Click add items, no items should be added
18 - Repopulate form with barcodes, check 'Enter by barcode'
19 - Click add items, items should be added correctly
20 - Test various combinations of radio buttons and numbers and using form or not

Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 54 Barton Chittenden 2015-11-29 21:48:29 UTC
I'm wondering if this can get pushed into 3.18?
Comment 55 Nick Clemens 2015-12-17 18:58:24 UTC
*** Bug 15393 has been marked as a duplicate of this bug. ***
Comment 56 Liz Rea 2016-01-06 21:04:37 UTC
Pushed to 3.18.13, and released.