Bug 36159 - Patron imports record a change for non-text columns that are not in the import file
Summary: Patron imports record a change for non-text columns that are not in the impor...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 3820
Blocks:
  Show dependency treegraph
 
Reported: 2024-02-23 16:33 UTC by Nick Clemens (kidclamp)
Modified: 2024-05-01 18:19 UTC (History)
5 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:
24.05.00,23.11.05


Attachments
Test import file (140 bytes, text/csv)
2024-02-23 16:34 UTC, Nick Clemens (kidclamp)
Details
Bug 36159: Patron imports record a change for non-text columns that are not in the import file (3.67 KB, patch)
2024-03-01 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36159: Patron imports record a change for non-text columns that are not in the import file (3.63 KB, patch)
2024-03-01 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36159: Patron imports record a change for non-text columns that are not in the import file (3.68 KB, patch)
2024-03-01 19:40 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 36159: Patron imports record a change for non-text columns that are not in the import file (3.68 KB, patch)
2024-03-28 10:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36159: (QA follow-up): Simplify code to supress warnings (1.24 KB, patch)
2024-03-28 10:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36159: Add unit test (1.61 KB, patch)
2024-03-28 10:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36159: Patron imports record a change for non-text columns that are not in the import file (3.77 KB, patch)
2024-03-29 08:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36159: (QA follow-up): Simplify code to supress warnings (1.33 KB, patch)
2024-03-29 08:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36159: Add unit test (1.70 KB, patch)
2024-03-29 08:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36159: (QA follow-up) Tidy code (2.03 KB, patch)
2024-03-29 11:25 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-02-23 16:33:18 UTC
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
Comment 1 Nick Clemens (kidclamp) 2024-02-23 16:34:52 UTC
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
Comment 2 Kyle M Hall 2024-03-01 14:55:54 UTC
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!
Comment 3 Kyle M Hall 2024-03-01 15:16:55 UTC
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!
Comment 4 Brendan Lawlor 2024-03-01 19:40:39 UTC
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>
Comment 5 Martin Renvoize 2024-03-05 15:28:59 UTC
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?
Comment 6 Nick Clemens (kidclamp) 2024-03-05 18:40:56 UTC
(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.
Comment 7 Marcel de Rooy 2024-03-15 09:41:10 UTC
+                        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.
Comment 8 Kyle M Hall 2024-03-28 10:19:20 UTC
(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.
Comment 9 Kyle M Hall 2024-03-28 10:37:27 UTC
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>
Comment 10 Kyle M Hall 2024-03-28 10:37:33 UTC
Created attachment 164039 [details] [review]
Bug 36159: (QA follow-up): Simplify code to supress warnings
Comment 11 Kyle M Hall 2024-03-28 10:37:35 UTC
Created attachment 164040 [details] [review]
Bug 36159: Add unit test
Comment 12 Marcel de Rooy 2024-03-29 08:46:51 UTC
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>
Comment 13 Marcel de Rooy 2024-03-29 08:46:54 UTC
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>
Comment 14 Marcel de Rooy 2024-03-29 08:46:56 UTC
Created attachment 164115 [details] [review]
Bug 36159: Add unit test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2024-03-29 08:49:42 UTC
(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 );
Comment 16 Kyle M Hall 2024-03-29 11:25:10 UTC
Created attachment 164124 [details] [review]
Bug 36159: (QA follow-up) Tidy code
Comment 17 Katrin Fischer 2024-04-02 15:38:23 UTC
Thanks for the tidy and also for the unit tests :) Always makes one feel better about changes!
Comment 18 Katrin Fischer 2024-04-02 16:00:51 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 19 Fridolin Somers 2024-04-17 15:12:27 UTC
Pushed to 23.11.x for 23.11.05