When importing patrons we assume a default of '' for borrower columns not supplied in the file. When saving we compare the new object we built to the one form the database - for columns are that are not text type we get undef from the db and '' in the object we make. This means we see a difference and log into the BorrowersLog: "date_renewed" : { "after" : "", "before" : null }, "dateofbirth" : { "after" : "", "before" : null }, "debarred" : { "after" : "", "before" : null }, "flags" : { "after" : "", "before" : null }, "gonenoaddress" : { "after" : "", "before" : null }, "lost" : { "after" : "", "before" : null }, "password_expiration_date" : { "after" : "", "before" : null }, "sms_provider_id" : { "after" : "", "before" : null } } This can mean a lot of useless logging in sites that do automated imports
Created attachment 162392 [details] Test import file Enable 'BorrowersLog' system preference Import this file, matchig on cardnuber, and overwriting Check the logs, see the modification of columns that have no date Import again We log it again
Created attachment 162669 [details] [review] Bug 36159: Patron imports record a change for non-text columns that are not in the import file When importing patrons we assume a default of '' for borrower columns not supplied in the file. When saving we compare the new object we built to the one form the database - for columns are that are not text type we get undef from the db and '' in the object we make. This means we see a difference and log into the BorrowersLog: "date_renewed" : { "after" : "", "before" : null }, "dateofbirth" : { "after" : "", "before" : null }, "debarred" : { "after" : "", "before" : null }, "flags" : { "after" : "", "before" : null }, "gonenoaddress" : { "after" : "", "before" : null }, "lost" : { "after" : "", "before" : null }, "password_expiration_date" : { "after" : "", "before" : null }, "sms_provider_id" : { "after" : "", "before" : null } } This can mean a lot of useless logging in sites that do automated imports Test Plan: 1) Enable 'BorrowersLog' system preference 2) Import the borrowers file attach do this bug report file, matchig on cardnuber, and overwriting Contents of the borrowers file are : surname,firstname,branchcode,categorycode,cardnumber,dateenrolled,patron_attributes,lastseen Acosta,Ednb,CPL,PT,23529001000463,02/01/2013,, 3) Check the logs, note the modification of columns that have no date 4) Import the file again with the same settings 5) Note the new action log 6) Apply this patch 7) Restart all the things! 8) Import the file again with the same settings 9) Note no new action log was created!
Created attachment 162672 [details] [review] Bug 36159: Patron imports record a change for non-text columns that are not in the import file When importing patrons we assume a default of '' for borrower columns not supplied in the file. When saving we compare the new object we built to the one form the database - for columns are that are not text type we get undef from the db and '' in the object we make. This means we see a difference and log into the BorrowersLog: "date_renewed" : { "after" : "", "before" : null }, "dateofbirth" : { "after" : "", "before" : null }, "debarred" : { "after" : "", "before" : null }, "flags" : { "after" : "", "before" : null }, "gonenoaddress" : { "after" : "", "before" : null }, "lost" : { "after" : "", "before" : null }, "password_expiration_date" : { "after" : "", "before" : null }, "sms_provider_id" : { "after" : "", "before" : null } } This can mean a lot of useless logging in sites that do automated imports Test Plan: 1) Enable 'BorrowersLog' system preference 2) Import the borrowers file attach do this bug report file, matchig on cardnuber, and overwriting Contents of the borrowers file are : surname,firstname,branchcode,categorycode,cardnumber,dateenrolled,patron_attributes,lastseen Acosta,Ednb,CPL,PT,23529001000463,02/01/2013,, 3) Check the logs, note the modification of columns that have no date 4) Import the file again with the same settings 5) Note the new action log 6) Apply this patch 7) Restart all the things! 8) Import the file again with the same settings 9) Note no new action log was created!
Created attachment 162689 [details] [review] Bug 36159: Patron imports record a change for non-text columns that are not in the import file When importing patrons we assume a default of '' for borrower columns not supplied in the file. When saving we compare the new object we built to the one form the database - for columns are that are not text type we get undef from the db and '' in the object we make. This means we see a difference and log into the BorrowersLog: "date_renewed" : { "after" : "", "before" : null }, "dateofbirth" : { "after" : "", "before" : null }, "debarred" : { "after" : "", "before" : null }, "flags" : { "after" : "", "before" : null }, "gonenoaddress" : { "after" : "", "before" : null }, "lost" : { "after" : "", "before" : null }, "password_expiration_date" : { "after" : "", "before" : null }, "sms_provider_id" : { "after" : "", "before" : null } } This can mean a lot of useless logging in sites that do automated imports Test Plan: 1) Enable 'BorrowersLog' system preference 2) Import the borrowers file attach do this bug report file, matchig on cardnuber, and overwriting Contents of the borrowers file are : surname,firstname,branchcode,categorycode,cardnumber,dateenrolled,patron_attributes,lastseen Acosta,Ednb,CPL,PT,23529001000463,02/01/2013,, 3) Check the logs, note the modification of columns that have no date 4) Import the file again with the same settings 5) Note the new action log 6) Apply this patch 7) Restart all the things! 8) Import the file again with the same settings 9) Note no new action log was created! Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
I think the defined check is there so we don't trigger warnings in the comparison... shouldn't we attempt to fix this at the import script level?
(In reply to Martin Renvoize from comment #5) > I think the defined check is there so we don't trigger warnings in the > comparison... shouldn't we attempt to fix this at the import script level? I think they will both always have all of the columns: - the db object has them because of schema - the incoming object has them all because we fill everything found in set_column_keys Testing I don't see errors from this new code I do get an unrelated warn before and after the patch: [2024/03/05 18:40:18] [WARN] Use of uninitialized value within %csvkeycol in array element at /kohadevbox/koha/Koha/Patrons/Import.pm line 288, <$fh> line 2.
+ if ( ( $from_storage->{$key} || $from_object->{$key} ) + && ( $from_storage->{$key} ne $from_object->{$key} ) ) Not sure why this should be changed. But this will trigger uninit warnings. This fix needs a test. Please add.
(In reply to Marcel de Rooy from comment #7) > + if ( ( $from_storage->{$key} || > $from_object->{$key} ) > + && ( $from_storage->{$key} ne > $from_object->{$key} ) ) > > Not sure why this should be changed. This is the actual fix for the bug. The other part of the patch is for handling datexpiry specifically.
Created attachment 164038 [details] [review] Bug 36159: Patron imports record a change for non-text columns that are not in the import file When importing patrons we assume a default of '' for borrower columns not supplied in the file. When saving we compare the new object we built to the one form the database - for columns are that are not text type we get undef from the db and '' in the object we make. This means we see a difference and log into the BorrowersLog: "date_renewed" : { "after" : "", "before" : null }, "dateofbirth" : { "after" : "", "before" : null }, "debarred" : { "after" : "", "before" : null }, "flags" : { "after" : "", "before" : null }, "gonenoaddress" : { "after" : "", "before" : null }, "lost" : { "after" : "", "before" : null }, "password_expiration_date" : { "after" : "", "before" : null }, "sms_provider_id" : { "after" : "", "before" : null } } This can mean a lot of useless logging in sites that do automated imports Test Plan: 1) Enable 'BorrowersLog' system preference 2) Import the borrowers file attach do this bug report file, matchig on cardnuber, and overwriting Contents of the borrowers file are : surname,firstname,branchcode,categorycode,cardnumber,dateenrolled,patron_attributes,lastseen Acosta,Ednb,CPL,PT,23529001000463,02/01/2013,, 3) Check the logs, note the modification of columns that have no date 4) Import the file again with the same settings 5) Note the new action log 6) Apply this patch 7) Restart all the things! 8) Import the file again with the same settings 9) Note no new action log was created! Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Created attachment 164039 [details] [review] Bug 36159: (QA follow-up): Simplify code to supress warnings
Created attachment 164040 [details] [review] Bug 36159: Add unit test
Created attachment 164113 [details] [review] Bug 36159: Patron imports record a change for non-text columns that are not in the import file When importing patrons we assume a default of '' for borrower columns not supplied in the file. When saving we compare the new object we built to the one form the database - for columns are that are not text type we get undef from the db and '' in the object we make. This means we see a difference and log into the BorrowersLog: "date_renewed" : { "after" : "", "before" : null }, "dateofbirth" : { "after" : "", "before" : null }, "debarred" : { "after" : "", "before" : null }, "flags" : { "after" : "", "before" : null }, "gonenoaddress" : { "after" : "", "before" : null }, "lost" : { "after" : "", "before" : null }, "password_expiration_date" : { "after" : "", "before" : null }, "sms_provider_id" : { "after" : "", "before" : null } } This can mean a lot of useless logging in sites that do automated imports Test Plan: 1) Enable 'BorrowersLog' system preference 2) Import the borrowers file attach do this bug report file, matchig on cardnuber, and overwriting Contents of the borrowers file are : surname,firstname,branchcode,categorycode,cardnumber,dateenrolled,patron_attributes,lastseen Acosta,Ednb,CPL,PT,23529001000463,02/01/2013,, 3) Check the logs, note the modification of columns that have no date 4) Import the file again with the same settings 5) Note the new action log 6) Apply this patch 7) Restart all the things! 8) Import the file again with the same settings 9) Note no new action log was created! Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 164114 [details] [review] Bug 36159: (QA follow-up): Simplify code to supress warnings Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 164115 [details] [review] Bug 36159: Add unit test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Kyle M Hall from comment #8) > This is the actual fix for the bug. The other part of the patch is for > handling datexpiry specifically. Had a closer look. Fine with me. Closing my eyes for just a few untidy warns. Probably the datexpiry thing comes from an earlier: $self->dateexpiry( $self->category->get_expiry_date );
Created attachment 164124 [details] [review] Bug 36159: (QA follow-up) Tidy code
Thanks for the tidy and also for the unit tests :) Always makes one feel better about changes!
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.05
Doesn't apply to 23.05.x, no backport.