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.
Created attachment 31319 [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).
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..
Hi Petter, not sure what you mean by default, can you give an example?
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.
(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
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.
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.
Created attachment 31497 [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.
> 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.
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.
For example borrowernumber should not be appear on the default values form :)
As this is already signed off - could we handle the columns in a separate patch?
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>
(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.
(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.
Patch pushed to master. Thanks Jacek!