|
Description
Kyle M Hall (khall)
2021-03-10 18:40:23 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 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". 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". 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 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 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 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> 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> 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> 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. 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> Are you sure about the test plan? No need to test the UI? "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 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. 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> Created attachment 139018 [details] [review] Bug 27920: (QA follow-up) Update expiration date Checkbox should be enabled when overwriting existing patrons Created attachment 139019 [details] [review] Bug 27920: (QA follow-up) Fix syntax error +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? 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] Created attachment 142176 [details] [review] Bug 27920: (QA follow-up) Change radios to a single select pulldown Created attachment 142178 [details] [review] Bug 27920: (QA follow-up) Fix POD in cli tool > 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! Back here. (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! 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> 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> Created attachment 142740 [details] [review] Bug 27920: (QA follow-up) Fix syntax error Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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> 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> 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> 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> Pushed to master for 22.11. Nice work everyone, thanks! |