Bug 12716 - Import patrons form should have drop-downs for some values
Summary: Import patrons form should have drop-downs for some values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12859 12918 12929
  Show dependency treegraph
 
Reported: 2014-08-04 12:37 UTC by Tomás Cohen Arazi
Modified: 2015-06-04 23:32 UTC (History)
4 users (show)

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


Attachments
Bug 12716: Allow the import patrons form have drop-downs and datepickers (9.77 KB, patch)
2014-08-06 17:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 12716: unit tests for GetColumnDefs (2.02 KB, patch)
2014-08-06 17:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 12716: Allow the import patrons form have drop-downs and datepickers (9.83 KB, patch)
2014-08-12 05:17 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12716: unit tests for GetColumnDefs (2.09 KB, patch)
2014-08-12 05:17 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
[PASSED QA] Bug 12716: Allow the import patrons form have drop-downs and datepickers (10.01 KB, patch)
2014-08-15 08:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12716: unit tests for GetColumnDefs (2.14 KB, patch)
2014-08-15 08:52 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2014-08-04 12:37:32 UTC
Categorycode and branches should be selectable drop downs.
Comment 1 Tomás Cohen Arazi 2014-08-06 17:25:13 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2014-08-06 17:25:54 UTC Comment hidden (obsolete)
Comment 3 Biblibre Sandboxes 2014-08-12 05:17:33 UTC
Patch tested with a sandbox, by Aleisha <aleishaamohia@hotmail.com>
Comment 4 Biblibre Sandboxes 2014-08-12 05:17:56 UTC Comment hidden (obsolete)
Comment 5 Biblibre Sandboxes 2014-08-12 05:17:59 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2014-08-15 08:46:53 UTC
Comment on attachment 30728 [details] [review]
Bug 12716: Allow the import patrons form have drop-downs and datepickers

Review of attachment 30728 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt
@@ +3,5 @@
>  <title>Koha &rsaquo; Tools &rsaquo; Import patrons [% IF ( uploadborrowers ) %]&rsaquo; Results[% END %]</title>
>  [% INCLUDE 'doc-head-close.inc' %]
>  <style type="text/css">
> +    .yui-u fieldset.rows .widelabel { width: 12em; }
> +    label.description { width: 20em; }

I'd like this a bit better in the staff CSS file, so people could overwrite it with OpacUserCSS - different language might need more or less space there. But not a blocker.

@@ +133,5 @@
> +            <option value="" selected="selected"></option>
> +        [% FOREACH branch IN branches %]
> +            <option value="[% branch.branchcode %]">
> +                [% branch.branchname %]</option>
> +        [% END %]

I think you could have used the TT plugin for branches here :)
Comment 7 Katrin Fischer 2014-08-15 08:51:59 UTC
Created attachment 30816 [details] [review]
[PASSED QA] Bug 12716: Allow the import patrons form have drop-downs and datepickers

To reproduce:
- Go to Tools > Import patrons
- Notice branchcode and categorycode don't have dropdown menus with
  valid options.
- Notice dateofbirth, dateenrolled and dateexpiry don't have date-pickers
- Notice each input field is labeled with the column name instead of a
  proper description.

To test:
- Apply the patch, go to Tools > Import patrons
- Notice branchcode and categorycode have dropdown menus with valid options,
  and by default the empty value is selected.
- Notice that if you go into the dateofbirth, dateenrolled and dateexpiry
  fields, a nice date picker widget appears.
- Notice each field is labeled with the field description, and that the
  column name is conveniently shown at the right of the input field.

Important: test switching the ExtendedPatronAttributes syspref, and verify that
if enabled, the patron_attributes field appears, and also the checkbox selector
controlling the extended patron attributes import behaviour shows too.

Bonus points: Verify that on a different language, the descriptions get translated,
and the column names show correctly

Regards
To+

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes tests and QA script.
Works as described, no regressions found.
Dates are output in ISO/database format, this is ok.
Comment 8 Katrin Fischer 2014-08-15 08:52:31 UTC
Created attachment 30817 [details] [review]
[PASSED QA] Bug 12716: unit tests for GetColumnDefs

The introduced function gets tested.

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 9 Tomás Cohen Arazi 2014-08-15 18:46:15 UTC
Patches pushed to master.
Comment 10 Jacek Ablewicz 2014-08-31 10:41:59 UTC
It may be something specific to my test setup, but I've just tried to test-import ~30k patron records with lots and lots of extended attributes - there are no errors reported, but none of those attributes are getting added to the database.

I guess that removal of this code part:

-if ($extended) {
-    push @columnkeys, 'patron_attributes';
-}

around line 65 in tools/import_borrowers.pl possibly does have some unforseen side effects, resulting in all extended attributes being ignored during import?
Comment 11 Tomás Cohen Arazi 2014-08-31 13:49:59 UTC
Jaceck, I don't think it is related to that specifically, but please fill a bug so we dig a bit.
Comment 12 Katrin Fischer 2014-08-31 14:06:01 UTC
Jacek, could you add an example line with the syntax you used in the import file too?
Comment 13 Jacek Ablewicz 2014-09-02 10:39:52 UTC
(In reply to Katrin Fischer from comment #12)
> Jacek, could you add an example line with the syntax you used in the import
> file too?

While I encountered this when trying to import patrons with somewhat more complex entries in 'patron_attibutes' CSV field, eg.:

"""DPCODE:PK-B"",""HISCHANNR:B/000123; E/000123; ^/000123"",""KNTPAP:BPK"",""KNTPAP:BWM"",""PESEL:1234567890"",""SHOW_BCODE:1"""

extended atributes import seems to be broken as well for more trivial cases - I did some tests in clean master with simpler 'patron_attributes'; same outcome.

Also, on the practical side, re-adding aforementioned 'if' solved the problem, at least for me ;).

This script (tools/import_patrons.pl) is not exactly the easiest one to analyze comprehensivelly, but it seems that 'patron_attributes' was artifically added to @columnkeys as pseudo-colum name (= a kludge, as there is no such column in borrowers table); when that pseudo column is no longer in @columnkeys, apparently extended attributes are not being picked up at all from the relevant CSV field (?).

Follow up & proposed patch: in Bug 12859.
Comment 14 Tomás Cohen Arazi 2014-09-02 11:02:50 UTC
(In reply to Jacek Ablewicz from comment #13)
>
> This script (tools/import_patrons.pl) is not exactly the easiest one to
> analyze comprehensivelly, but it seems that 'patron_attributes' was
> artifically added to @columnkeys as pseudo-colum name (= a kludge, as there
> is no such column in borrowers table); when that pseudo column is no longer
> in @columnkeys, apparently extended attributes are not being picked up at
> all from the relevant CSV field (?).
> 
> Follow up & proposed patch: in Bug 12859.

Well done Jaceck! Thanks a lot!