Bug 5742

Summary: batch edit patrons
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: enhancement    
Priority: P5 - low CC: dpavlin, gmcharlt, jonathan.druart, julien.sicot, katrin.fischer, mathilde.formery, paul.poulain, stephane.delaye
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: But 5742: Batch edit patrons
But 5742 Batch edit patrons
But 5742 Batch edit patrons
But 5742 Batch edit patrons
But 5742 Batch edit patrons
But 5742 Batch edit patrons
[SIGN OFF] But 5742: Batch edit patrons
Bug 5742: Followup: sort* fields can to be filled with an input text
[SIGNED OFF] But 5742: Batch edit patrons
Bug 5742: Followup: sort* fields can to be filled with an input text

Description Nicole C. Engard 2011-02-11 17:46:32 UTC
It would be awesome if there were a batch patron edit tool that included the patron attributes in it.  An example use of this would be for school libraries that keep track of home rooms as an attribute - that changes each year and it would be great to be able to batch edit that.
Comment 1 Jonathan Druart 2012-03-29 13:57:05 UTC Comment hidden (obsolete)
Comment 2 Julien Sicot 2012-03-29 14:43:07 UTC
I applied the patch and followed the plan test but I've got an error on step 1 by clicking on Patrons modification link (/cgi-bin/koha/tools/modborrowers.pl).

The error message is below :

"Software error:
Type of arg 1 to values must be hash (not private variable) at /home/koha/src/tools/modborrowers.pl line 89, near "$patron_attribute_types ) "
Execution of /home/koha/src/tools/modborrowers.pl aborted due to compilation errors."

Tested on sandbox.
Comment 3 Jonathan Druart 2012-03-29 15:51:14 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-03-30 08:17:49 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2012-03-30 08:20:32 UTC Comment hidden (obsolete)
Comment 6 delaye 2012-04-03 10:18:10 UTC
i try to test the patch in a sandbox (number 1) but i have this message

Applying: But 5742: Batch edit patrons
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Patch failed at 0001 But 5742: Batch edit patrons
Comment 7 Jonathan Druart 2012-04-03 11:29:20 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2012-04-03 12:42:20 UTC
I get an error when submitting card numbers, whether it be in a file or via the textarea:

Can't use an undefined value as a HASH reference at /home/oleonard/kohaclone/tools/modborrowers.pl line 336
Comment 9 delaye 2012-04-03 13:32:35 UTC
I have tested the patch in a sandbox and it works.

With the scans of cardnumbers(textarea) or with a file that lists of cardnumbers

1/ update the borrower branch --> ok
2/ update the category --> ok
3/ changing the expiration date --> ok
4/ add a debarred date and a comment --> ok
5/ add a borrower note --> ok
6/ Delete the borrower note --> ok
7/ Add or replace one or more borrower attributes --> ok
Comment 10 Jonathan Druart 2012-04-03 13:37:54 UTC
Hi Owen, 
I can't reproduce this error.
l.336 is:
$borrower->{category_description} = GetBorrowercategory( $borrower->{categorycode} )->{description};

Is it possible you have a problem with your borrower's data ?
$borrower must have a categorycode and GetBorrowercategory returns a hashref for this categorycode.
Comment 11 Owen Leonard 2012-04-03 13:57:32 UTC
(In reply to comment #10)
> Is it possible you have a problem with your borrower's data ?

Indeed I did. I was working from a fresh install and I neglected to import my patron categories. Sorry for the false alarm.

One concern I have: Should we even allow the librarian to empty the patron's home branch or category fields? This seems like a dangerous operation.

I wonder if we shouldn't also prevent the user from emptying any fields which are specified in the BorrowerMandatoryField system preference?
Comment 12 delaye 2012-04-03 14:38:53 UTC
one more system preference ?....
instead : it is possible to hide the opportunity to give clear in front the fields
 category and branch ?
Comment 13 Jonathan Druart 2012-04-04 13:20:55 UTC Comment hidden (obsolete)
Comment 14 Paul Poulain 2012-04-04 15:08:04 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2012-04-05 06:15:52 UTC
Hi, 
I was curious about the interface and did some tests. I have one suggestion about the sort1 and sort2 fields - some libraries use those as free text fields. The new tool will only let you change them when there is an authorized value connected to it. So it might be a nice addition to remove that limit and make it possible to empty/change the fields without a connected authorized value too.
But that's not something that should stop this :)
Comment 16 Jonathan Druart 2012-04-05 08:16:43 UTC Comment hidden (obsolete)
Comment 17 MathildeF 2012-04-12 13:48:32 UTC
I have tested the patch in a sandbox and it don't works.


Something went wrong !
Applying: But 5742: Batch edit patrons
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Members.pm
CONFLICT (content): Merge conflict in C4/Members.pm
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt
Failed to merge in the changes.
Patch failed at 0001 But 5742: Batch edit patrons
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 5742 - batch edit patrons
Comment 18 Jonathan Druart 2012-04-12 14:56:22 UTC
Created attachment 9149 [details] [review]
[SIGNED OFF] But 5742: Batch edit patrons

rebased patch
Comment 19 Jonathan Druart 2012-04-12 14:56:44 UTC
Created attachment 9150 [details] [review]
Bug 5742: Followup: sort* fields can to be filled with an input text

Rebased patch
Comment 20 delaye 2012-04-27 15:10:35 UTC
I test the patch in a sandbox(1) but in the screen
../cgi-bin/koha/tools/tools-home.pl

i don't see the link : ../cgi-bin/koha/tools/modborrowers.pl
Comment 21 delaye 2012-04-30 14:55:57 UTC
i don't see the new permission
Comment 22 delaye 2012-05-07 13:06:40 UTC
after test in a sand box

i test the update of the attribute , the dateexpiry, debarrred, location etc , its ok
i change
Comment 23 Paul Poulain 2012-05-15 14:54:25 UTC
QA comment:
 * this patch contains also a lot of coding guidelines or PBP fixes, like
  - replacing tabs by spaces
  - avoiding things like "return undef" or my $x if $x eq 'condition';
That's a good thing, but the "usefull" code is harder to spot !

 * Question = why do you define new subs like UpdateBorrowerAttribute, and don't export them ? I think you should (even if they are not used outside from the package for now, couldn't they be in the future ?)

 * tiny harmless typo "modifivation" instead of "modification" ;-)

 * modborrowers.pl passes perlcritic

 * the new template is valid

=> just answer my question (with a follow-up if applicable), and it will earn pass QA !
Comment 24 Paul Poulain 2012-05-15 15:14:20 UTC
(In reply to comment #23)
> QA comment:
>  * this patch contains also a lot of coding guidelines or PBP fixes, like
>   - replacing tabs by spaces
>   - avoiding things like "return undef" or my $x if $x eq 'condition';
> That's a good thing, but the "usefull" code is harder to spot !
> 
>  * Question = why do you define new subs like UpdateBorrowerAttribute, and
> don't export them ? I think you should (even if they are not used outside
> from the package for now, couldn't they be in the future ?)
OK, silly me ! they are.

(note that this patch has been signed off by Mathilde during the hackfest, through sandboxes, I'll add his signoff on the patch itself, with the one from Stéphane)
Comment 25 Paul Poulain 2012-05-15 16:01:25 UTC
QA comments (continued)
 * C4/Members passes perlcritic
 * tools/modborrowers.pl passes perlcritic

prove xt/permissions.t fails =
xt/permissions.t .. 1/? 
#   Failed test 'No user permissions to add'
#   at xt/permissions.t line 95.
# User permissions to add in uk-UA/mandatory/permissions_and_user_flags.sql: edit_patrons

#   Failed test 'No user permissions to add'
#   at xt/permissions.t line 95.
# User permissions to add in ru-RU/mandatory/permissions_and_user_flags.sql: edit_patrons
# Looks like you failed 2 tests of 34.
=> i've added a small follow-up to add the 2 permissions, now passes this test

passed QA
Comment 26 Dobrica Pavlinusic 2012-06-06 10:31:31 UTC
This bug breaks members/memberentry.pl?op=add when you are using extended attributes since it displays all extended attributes available in database as opposed to empty input boxes for available attributes. Bug 8196 will contain fix for this problem.
Comment 27 Jared Camins-Esakov 2012-12-31 01:03:27 UTC
There have been no further reports of problems so I am marking this bug resolved.