Bug 12859 - Patron import tool doesn't import extened attributes values any longer
Summary: Patron import tool doesn't import extened attributes values any longer
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jacek Ablewicz
QA Contact: Testopia
URL:
Keywords:
Depends on: 12716
Blocks: 12929
  Show dependency treegraph
 
Reported: 2014-09-02 09:33 UTC by Jacek Ablewicz
Modified: 2015-06-04 23:33 UTC (History)
4 users (show)

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


Attachments
Bug 12859 - Patron import tool doesn't import extened attributes values any longer (1.72 KB, patch)
2014-09-02 10:00 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 12859 - Patron import tool doesn't import extened attributes values any longer (1.86 KB, patch)
2014-09-10 07:42 UTC, Petter Goksøyr Åsen
Details | Diff | Splinter Review
Bug 12859 - Patron import tool doesn't import extened attributes values any longer (1.92 KB, patch)
2014-09-16 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2014-09-02 09:33:48 UTC
To reproduce:

1/ define/set up extended pattron attribute in administration module,
2/ using "Tools -> Import patrons", try to import some sample patron record[s], having some arbitrary (non-empty) value assigned in 'pattron_attributes' CSV column for attribute defined in step 1/,
3/ observe that extended attribute defined in step 1/ was not imported (not added to the database), without any warnings.

Ad. 1: Note that it's also strictly necessary to have all possible extended attributes types which may be present in CSV import file defined in the administration, before performing an import; if even one of them is not defined, all other extended attributes for a given patron are being silently skipped as well. But that's outside of the scope of this particular bug.
Comment 1 Jacek Ablewicz 2014-09-02 10:00:36 UTC Comment hidden (obsolete)
Comment 2 Petter Goksøyr Åsen 2014-09-08 02:09:59 UTC
I recently run into this bug, and can confirm that this patch sovles it.

I'm happy to sign off, only I couldn't get it to work to specify
default value for extended attribute. How is the syntax for that?
Same as in the CSV? I tried all variants with quotes and without quotes,
but no luck..
Comment 3 Katrin Fischer 2014-09-08 19:18:30 UTC
Hi Petter, not sure what you mean by default, can you give an example?
Comment 4 Petter Goksøyr Åsen 2014-09-09 00:18:51 UTC
Hi Katrin,
I was refering to step 4 in the test plan above.

In cgi-bin/koha/tools/import_borrowers.pl there is a form titled "default values" where you can specify default values for any of the columns in the CSV, including the column patron_attributes, which contain the extended patron attributes. It works for the standard columns that correspond to fields in the borrowers table, but I could't get it to work for the patron_attributes column, which is inserted into the borrower_attributes table.
Comment 5 Jacek Ablewicz 2014-09-09 08:47:00 UTC
(In reply to Petter Goksøyr Åsen from comment #2)

> I'm happy to sign off, only I couldn't get it to work to specify
> default value for extended attribute. How is the syntax for that?
> Same as in the CSV?

Not the same syntax; for patron_attributes column in csv file, it may be something like that (double/triple quotes)

   """DPCODE:PK-B"",""KNTPAP:BPK"",""KNTPAP:BWM"",""SHOW_BCODE:1"""

but when specified as default value it needs to be in the different format (single quotes around each one of the atrributes, or no quotes at all):

   "DPCODE:PK-B","KNTPAP:BPK","KNTPAP:BWM","SHOW_BCODE:1"

It works (at least for me) when the default values are given in such format. But I only tested some trivial cases ("Ignore this one, keep the existing one": checked, "Replace all patron attributes": checked, no matchpoints defined for extended atrributes).

Another possible reason for the attributes not being added to the database is mentioned in comment #0 (Ad.1): in such a case, there would be also some errors in the logfile:

import_borrowers.pl: Database returned the following error: Cannot add or update a child row: a foreign key constraint fails (`koha`.`borrower_attributes`, CONSTRAINT `borrower_attributes_ibfk_2` FOREIGN KEY (`code`) REFERENCES `borrower_attribute_types` (`code`) ON DELETE CASCADE ON UPDATE CASCADE) at /home/koha/devkohaclone/C4/Members/Attributes.pm line 232
Comment 6 Petter Goksøyr Åsen 2014-09-10 07:15:49 UTC
OK this is what I found out:

If I remove the patron_attributes column from the CSV, the default values for extended attributes are used. But if the patron_attributes column is present, they are not used. This is different from the other columns, where default values are used if the column is empty for any particular value.

Can you confirm this behaviour?

I'd still like to sign off, as this patch at least solves import of extended patron attributes from the CSV-file.
Comment 7 Katrin Fischer 2014-09-10 07:20:56 UTC
I think as this fixes an accidental regression by putting deleted code back, it should go in as it is. And the new issues found could be on a separate bug.
Comment 8 Petter Goksøyr Åsen 2014-09-10 07:42:41 UTC Comment hidden (obsolete)
Comment 9 Petter Goksøyr Åsen 2014-09-10 07:43:15 UTC
> I think as this fixes an accidental regression by putting deleted code back, it should go in as it is. And the new issues found could be on a separate bug.

Agreed:) Patch signed off.
Comment 10 Jonathan Druart 2014-09-10 14:45:45 UTC
Hello Jacek,
I don't know very well this script, but I suspect that all the code which has been removed by the commit you pointed out should be reintroduced.
Don't you think?

cc Tomas.
Comment 11 Jonathan Druart 2014-09-10 14:46:20 UTC
For example borrowernumber should not be appear on the default values form :)
Comment 12 Katrin Fischer 2014-09-10 21:33:41 UTC
As this is already signed off - could we handle the columns in a separate patch?
Comment 13 Jonathan Druart 2014-09-16 14:04:27 UTC
Created attachment 31633 [details] [review]
Bug 12859 - Patron import tool doesn't import extened attributes values any longer

To reproduce:

1/ define/set up extended pattron attribute in administration module,
2/ using "Tools -> Import patrons", try to import some sample patron
record[s], having some arbitrary (non-empty) value assigned in
'pattron_attributes' CSV column for attribute defined in step 1/,
3/ observe that extended attribute defined in step 1/ was not imported
(not added to the database), without any warnings.

This patch restores a small code chunk which went AWOL somewhere around
commit b4158f2d2485c9e57bd7f14362b210732bef9d35.

To test:

1/ apply patch & retest,
3/ ensure that this issue is no longer reproductible, and there are no
apparent regressions of any kind,
4/ make sure that the ability to specify default value for extended
attributes added in bug Bug 12716 is still working as intended
(= follow up the test plan for bug 12716).

Signed-off-by: Petter Goksoyr Asen <boutrosboutrosboutros@gmail.com>
Patch fixes import of extended patron attributes.
Step 1-3 in test plan confirmed.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 14 Jonathan Druart 2014-09-16 14:10:56 UTC
(In reply to Katrin Fischer from comment #12)
> As this is already signed off - could we handle the columns in a separate
> patch?

See bug 12929.
Comment 15 Jonathan Druart 2014-09-16 14:30:40 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Katrin Fischer from comment #12)
> > As this is already signed off - could we handle the columns in a separate
> > patch?
> 
> See bug 12929.

Patch submitted, needs signoff.
Comment 16 Tomás Cohen Arazi 2014-09-16 18:30:12 UTC
Patch pushed to master.

Thanks Jacek!