Bug 34883 - Regression in Patron Import dateexpiry function
Summary: Regression in Patron Import dateexpiry function
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 22.11
Hardware: All All
: P5 - low minor with 10 votes (vote)
Assignee: Matt Blenkinsop
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 27920
Blocks:
  Show dependency treegraph
 
Reported: 2023-09-22 11:06 UTC by Jake Deery
Modified: 2023-11-08 16:55 UTC (History)
7 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:
23.11.00,23.05.05,22.11.11


Attachments
Bug 34883: Stop patron expiry date being set to NULL during import (1.96 KB, patch)
2023-09-25 10:47 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34883: Stop patron expiry date being set to NULL during import (1.99 KB, patch)
2023-09-25 22:37 UTC, David Nind
Details | Diff | Splinter Review
Sample patron import file - Bug 34883 (122 bytes, text/csv)
2023-09-25 22:42 UTC, David Nind
Details
Bug 34883: Add unit test (3.06 KB, patch)
2023-09-29 09:06 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34883: Stop patron expiry date being set to NULL during import (2.09 KB, patch)
2023-09-29 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34883: Add unit test (3.13 KB, patch)
2023-09-29 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jake Deery 2023-09-22 11:06:38 UTC
Hello all,

We are seeing issues in our import_patrons.pl which is causing issue for our academic customer base.

In versions prior to 22.11, running import_patrons.pl would cause the dateexpiry to either:
— get set based on the contents of the dateexpiry in the CSV file; or
— get left as the value already in Koha

This worked well for our academics, because they could opt when to update the dateexpiry rather than sending them over every night (e.g. if a student exited a course early, they could be expired in the LMS to prevent them from accessing the services).

In 22.11, the update-expiration and expiration-from-today flags have been added to the script. I believe the logic behind these flags is to update the dateexpiry based on enrolment plus user category duration, or today plus user category duration. This probably works great for most libraries, but it isn't necessarily desirable for Colleges.

The option still exists to set dateexpiry from the CSV, by not passing the aforementioned flags. However, the dateexpiry now gets set to the following, when using this:
— get set based on the contents of the dateexpiry in the CSV file; or
— get left as the value already in Koha

As you can see, this is a clear regression. Could we please look to remediate this, so I can pass on some good news to our affected customers?

Many thanks,
Jake Deery
PTFS Europe
Comment 1 Jake Deery 2023-09-22 11:14:23 UTC
> The option still exists to set dateexpiry from the CSV, by not passing the
> aforementioned flags. However, the dateexpiry now gets set to the following,
> when using this:
> — get set based on the contents of the dateexpiry in the CSV file; or
> — get left as the value already in Koha

Apologies, I meant to say that the regressed behaviour is as follows:
— get set based on the contents of the dateexpiry in the CSV file; or
— gets nulled out, essentially setting a patron as never expired
Comment 2 Martin Renvoize 2023-09-22 12:36:01 UTC
This will undoubtedly be a side effect of bug 27920.
Comment 3 Matt Blenkinsop 2023-09-25 10:47:16 UTC
Created attachment 156149 [details] [review]
Bug 34883: Stop patron expiry date being set to NULL during import

A regression has been identified whereby an empty field in the dateexpiry field in a patron import file will cause the patron's expiry date to be set to NULL. This patch addresses this by checking for an empty field and using the existing expiry date if one is found.

Test plan:
1) Setup a csv with column headers:
    surname	firstname	branchcode	categorycode	cardnumber	dateenrolled dateexpiry
2) Add values:
    Acosta	Edna	CPL	PT	23529001000463	02/01/2013
3) Leave the dateexpiry column blank
4) Check Edna and make a note of her patron expiry date
5) Run the import_patrons.pl script with the following flags:
    a) --file <filepath_for_your_csv_file>
    b) --matchpoint cardnumber
    c) --confirm
    d) --overwrite
6) Check Edna, note her expiry date is now set to NULL
7) Manually edit Edna's expiry date to be reset to what it was before you ran the script
8) Apply the patch and restart_all
9) Repeat step 5
10) Check Edna, this time her expiry date should be the same as the value you set it to in step 7
11) Sign off!
Comment 4 David Nind 2023-09-25 22:37:43 UTC
Created attachment 156196 [details] [review]
Bug 34883: Stop patron expiry date being set to NULL during import

A regression has been identified whereby an empty field in the dateexpiry field in a patron import file will cause the patron's expiry date to be set to NULL. This patch addresses this by checking for an empty field and using the existing expiry date if one is found.

Test plan:
1) Setup a csv with column headers:
    surname	firstname	branchcode	categorycode	cardnumber	dateenrolled dateexpiry
2) Add values:
    Acosta	Edna	CPL	PT	23529001000463	02/01/2013
3) Leave the dateexpiry column blank
4) Check Edna and make a note of her patron expiry date
5) Run the import_patrons.pl script with the following flags:
    a) --file <filepath_for_your_csv_file>
    b) --matchpoint cardnumber
    c) --confirm
    d) --overwrite
6) Check Edna, note her expiry date is now set to NULL
7) Manually edit Edna's expiry date to be reset to what it was before you ran the script
8) Apply the patch and restart_all
9) Repeat step 5
10) Check Edna, this time her expiry date should be the same as the value you set it to in step 7
11) Sign off!

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2023-09-25 22:42:09 UTC
Created attachment 156197 [details]
Sample patron import file - Bug 34883

Testing notes (using KTD):

1. I've added the CSV file I used for testing (rename and save into /kohadevbox/koha using whatever file name you want).

2. Command: misc/import_patrons.pl --file /kohadevbox/koha/whatever-you-called-me.csv --matchpoint cardnumber --confirm --overwrite -v
Comment 6 Marcel de Rooy 2023-09-29 07:17:05 UTC
Looks good to me. But could you add a small test that will stop future regressions on this topic?
Comment 7 Matt Blenkinsop 2023-09-29 09:06:41 UTC
Created attachment 156368 [details] [review]
Bug 34883: Add unit test

prove -v t/db_dependent/Koha/Patrons/Import.t
Comment 8 Matt Blenkinsop 2023-09-29 09:07:13 UTC
Good call, have added a new test to try and prevent regressions
Comment 9 Marcel de Rooy 2023-09-29 09:16:14 UTC
Created attachment 156369 [details] [review]
Bug 34883: Stop patron expiry date being set to NULL during import

A regression has been identified whereby an empty field in the dateexpiry field in a patron import file will cause the patron's expiry date to be set to NULL. This patch addresses this by checking for an empty field and using the existing expiry date if one is found.

Test plan:
1) Setup a csv with column headers:
    surname	firstname	branchcode	categorycode	cardnumber	dateenrolled dateexpiry
2) Add values:
    Acosta	Edna	CPL	PT	23529001000463	02/01/2013
3) Leave the dateexpiry column blank
4) Check Edna and make a note of her patron expiry date
5) Run the import_patrons.pl script with the following flags:
    a) --file <filepath_for_your_csv_file>
    b) --matchpoint cardnumber
    c) --confirm
    d) --overwrite
6) Check Edna, note her expiry date is now set to NULL
7) Manually edit Edna's expiry date to be reset to what it was before you ran the script
8) Apply the patch and restart_all
9) Repeat step 5
10) Check Edna, this time her expiry date should be the same as the value you set it to in step 7
11) Sign off!

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2023-09-29 09:16:16 UTC
Created attachment 156370 [details] [review]
Bug 34883: Add unit test

prove -v t/db_dependent/Koha/Patrons/Import.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Tomás Cohen Arazi 2023-10-10 12:23:09 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 12 Fridolin Somers 2023-10-12 20:34:54 UTC
Pushed to 23.05.x for 23.05.05
Comment 13 Matt Blenkinsop 2023-10-17 17:36:12 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x