Bug 27920 - Add ability to update patron expiration dates when importing patrons
Summary: Add ability to update patron expiration dates when importing patrons
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 27883
Blocks: 34621 34883
  Show dependency treegraph
 
Reported: 2021-03-10 18:40 UTC by Kyle M Hall
Modified: 2023-09-22 12:35 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This adds a new option to the patron import from that allows to use the expiration date from the import file or recalculate the patron's expiration date using today's date or the original expiration date as base.
Version(s) released in:
22.11.00


Attachments
Bug 27920: Add ability to update patron expiration dates when importing patrons (8.20 KB, patch)
2021-03-10 18:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: Add ability to update patron expiration dates when importing patrons (8.20 KB, patch)
2021-04-06 16:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: Add ability to update patron expiration dates when importing patrons (8.20 KB, patch)
2021-05-13 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: Add ability to update patron expiration dates when importing patrons (8.27 KB, patch)
2021-05-13 19:45 UTC, David Nind
Details | Diff | Splinter Review
Bug 27920: Add ability to update patron expiration dates when importing patrons (11.23 KB, patch)
2021-06-12 11:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: Add ability to update patron expiration dates when importing patrons (11.20 KB, patch)
2021-06-12 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: Add ability to update patron expiration dates when importing patrons (9.93 KB, patch)
2022-08-03 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: Add ability to update patron expiration dates when importing patrons (9.93 KB, patch)
2022-08-11 11:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Update expiration date Checkbox should be enabled when overwriting existing patrons (1.40 KB, patch)
2022-08-11 11:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Fix syntax error (1.06 KB, patch)
2022-08-11 11:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Change radios to a single select pulldown (4.48 KB, patch)
2022-10-19 15:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Fix POD in cli tool (1.03 KB, patch)
2022-10-19 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27920: Add ability to update patron expiration dates when importing patrons (9.92 KB, patch)
2022-10-27 21:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Update expiration date Checkbox should be enabled when overwriting existing patrons (1.45 KB, patch)
2022-10-27 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Fix syntax error (1.11 KB, patch)
2022-10-27 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Change radios to a single select pulldown (4.54 KB, patch)
2022-10-27 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Fix POD in cli tool (1.08 KB, patch)
2022-10-27 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Remove now obsolete JavaScript lines (1.48 KB, patch)
2022-10-27 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27920: (QA follow-up) Improve alignment of new option on patron form (1.89 KB, patch)
2022-10-27 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2021-03-10 18:40:23 UTC
Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.
Comment 1 Kyle M Hall 2021-03-10 18:50:45 UTC
Created attachment 118079 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t
Comment 2 Amit Gupta 2021-03-23 15:29:53 UTC
Could not apply the patch
------------------------------------

Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: a
Applying: Bug 27920: Add ability to update patron expiration dates when importing patrons
fatal: sha1 information is lacking or useless (Koha/Patrons/Import.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 27920: Add ability to update patron expiration dates when importing patrons
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 3 Kyle M Hall 2021-03-24 11:25:42 UTC
You'll need to apply the patches from bug 27883 first.

(In reply to Amit Gupta from comment #2)
> Could not apply the patch
> ------------------------------------
> 
> Bug 27920: Add ability to update patron expiration dates when importing
> patrons
> 
> Some libraries need to recalculate a patron's expiration date any time they
> are updated via a patron import from file.
> 
> Test Plan:
> 1) Apply this patch
> 2) prove t/db_dependent/Koha/Patrons/Import.t
> --------------------------
> Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: a
> Applying: Bug 27920: Add ability to update patron expiration dates when
> importing patrons
> fatal: sha1 information is lacking or useless (Koha/Patrons/Import.pm).
> error: could not build fake ancestor
> Patch failed at 0001 Bug 27920: Add ability to update patron expiration
> dates when importing patrons
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
Comment 4 Kyle M Hall 2021-04-06 16:06:30 UTC
Created attachment 119234 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t
Comment 5 David Nind 2021-04-10 22:04:49 UTC
Hi Kyle.

I get this message when applying the patch (after applying 27883):

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 27920: Add ability to update patron expiration dates when importing patrons
error: sha1 information is lacking or useless (Koha/Patrons/Import.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 27920: Add ability to update patron expiration dates when importing patrons
...

David
Comment 6 Kyle M Hall 2021-05-13 15:31:11 UTC
Created attachment 120940 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t
Comment 7 David Nind 2021-05-13 19:45:23 UTC
Created attachment 120951 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Kyle M Hall 2021-06-12 11:25:09 UTC
Created attachment 121885 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Kyle M Hall 2021-06-12 13:41:10 UTC
Created attachment 121886 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Katrin Fischer 2021-07-25 01:00:48 UTC
QA tests fail, please fix!

 FAIL	misc/import_patrons.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		syntax error 
		        update_dateexpiry_from_today"
		Global symbol "$return" requires explicit package name (did you forget to declare "my $return"?) 
		misc/import_patrons.pl had compilation errors.
Comment 11 Kyle M Hall 2022-08-03 11:14:59 UTC
Created attachment 138524 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Jonathan Druart 2022-08-09 08:30:49 UTC
Are you sure about the test plan? No need to test the UI?
Comment 13 Jonathan Druart 2022-08-09 08:33:52 UTC
"Update patron's expiration date based on the current date instead of the patron's enrollment date"

This checkbox is always disabled, how is it useful?
Comment 14 Jonathan Druart 2022-08-09 08:48:26 UTC
Comment 10 still valid

 FAIL   misc/import_patrons.pl
   FAIL   valid
                syntax error
                        update_dateexpiry_from_today"
                Global symbol "$return" requires explicit package name (did you forget to declare "my $return"?)
                misc/import_patrons.pl had compilation errors.
Comment 15 Kyle M Hall 2022-08-11 11:40:12 UTC
Created attachment 139017 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 Kyle M Hall 2022-08-11 11:40:44 UTC
Created attachment 139018 [details] [review]
Bug 27920: (QA follow-up) Update expiration date Checkbox should be enabled when overwriting existing patrons
Comment 17 Kyle M Hall 2022-08-11 11:40:48 UTC
Created attachment 139019 [details] [review]
Bug 27920: (QA follow-up) Fix syntax error
Comment 18 Marcel de Rooy 2022-09-30 09:20:47 UTC
+import_patrons.pl --file /path/to/patrons.csv --matchpoint cardnumber --confirm [--default branchcode=MPL] [--overwrite] [--preserve_field <column>] [--preserve-extended-attributes] [--update-expiration] [--verbose]

Only one added?
Comment 19 Katrin Fischer 2022-10-18 20:00:19 UTC
1) Unit test

With the patch applied, the tests output some warn, without the patch the output is clean:

prove t/db_dependent/Koha/Patrons/Import.t
t/db_dependent/Koha/Patrons/Import.t .. 166/176 Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$fh> line 2.
Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$fh> line 2.
t/db_dependent/Koha/Patrons/Import.t .. ok  BorrowerRenewalPeriodBase

2) GUI

I am not sure the options are described clearly as is:

Update existing patron expiration dates.

* Maybe instead of update it should be "recalculate"? 
Otherwise I would understand this as 'use the expiration date from the file'.

Update patron's expiration date based on the current date instead of the patron's enrollment date

* We have BorrowerRenewalPeriodBase, maybe we could have a pull down or preselect the option according to the pref? 

* The 2 options feel disconnected. Maybe the second should only be enabled, if the first was checked? Or use identation?

* What's the supposed behaviour when there is an expiration date in the import file? Will it be used or do the checkboxes take precedence? If so, maybe we could add: ... values in the import file will be ignored.

* While we are there... can we also remove the . from the only option that has it? :)


3) Test plan

Can we have a test plan please?

4) CLI

Marcel is right, expiration-from-today is missing:

+import_patrons.pl --file /path/to/patrons.csv --matchpoint cardnumber --confirm [--default branchcode=MPL] [--overwrite] [--preserve_field <column>] [--preserve-extended-attributes] [--update-expiration] [--verbose]
Comment 20 Kyle M Hall 2022-10-19 15:06:52 UTC
Created attachment 142176 [details] [review]
Bug 27920: (QA follow-up) Change radios to a single select pulldown
Comment 21 Kyle M Hall 2022-10-19 15:07:05 UTC
Created attachment 142178 [details] [review]
Bug 27920: (QA follow-up) Fix POD in cli tool
Comment 22 Kyle M Hall 2022-10-19 15:10:44 UTC
> 1) Unit test

I'm not getting that warning. Can you see if it's still there for you?

> 2) GUI

I've added a following to unify the options into a single pulldown menu. This feels more unified and clear to me. What do you think?
 
> 3) Test plan

Test Plan:
1) Create a patron CSV file with a datexpiry column
2) Apply this patch
3) Restart all the things
4) Upload your file, choose to overwrite existing patrons,
   test with each of the 3 dateexpiry options, verify each
   performs correctly!

> 4) CLI

Fixed with another followup!
Comment 23 Katrin Fischer 2022-10-27 21:09:11 UTC
Back here.
Comment 24 Katrin Fischer 2022-10-27 21:36:02 UTC
(In reply to Kyle M Hall from comment #22)
> > 1) Unit test
> 
> I'm not getting that warning. Can you see if it's still there for you?

It's gone! 

> > 2) GUI
> 
> I've added a following to unify the options into a single pulldown menu.
> This feels more unified and clear to me. What do you think?

This is much easier to understand, thx!

I made 2 changes in the follow-ups, please check.

> > 3) Test plan
> 
> Test Plan:
> 1) Create a patron CSV file with a datexpiry column
> 2) Apply this patch
> 3) Restart all the things
> 4) Upload your file, choose to overwrite existing patrons,
>    test with each of the 3 dateexpiry options, verify each
>    performs correctly!
> 
> > 4) CLI
> 
> Fixed with another followup!
Comment 25 Katrin Fischer 2022-10-27 21:37:05 UTC
Created attachment 142738 [details] [review]
Bug 27920: Add ability to update patron expiration dates when importing patrons

Some libraries need to recalculate a patron's expiration date any time they are updated via a patron import from file.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Patrons/Import.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 Katrin Fischer 2022-10-27 21:38:19 UTC
Created attachment 142739 [details] [review]
Bug 27920: (QA follow-up) Update expiration date Checkbox should be enabled when overwriting existing patrons

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2022-10-27 21:38:24 UTC
Created attachment 142740 [details] [review]
Bug 27920: (QA follow-up) Fix syntax error

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2022-10-27 21:38:29 UTC
Created attachment 142741 [details] [review]
Bug 27920: (QA follow-up) Change radios to a single select pulldown

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2022-10-27 21:38:34 UTC
Created attachment 142742 [details] [review]
Bug 27920: (QA follow-up) Fix POD in cli tool

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2022-10-27 21:38:39 UTC
Created attachment 142743 [details] [review]
Bug 27920: (QA follow-up) Remove now obsolete JavaScript lines

There JS lines were left over from the first suggested GUI
using a checkbox.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2022-10-27 21:38:43 UTC
Created attachment 142744 [details] [review]
Bug 27920: (QA follow-up) Improve alignment of new option on patron form

Switches from ul to ol as used by other options which improves the
alignment of the options.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Tomás Cohen Arazi 2022-11-09 17:39:31 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!