Created attachment 5938 [details] renew subscription length When adding a new subscription there is a pull down menu for the subscription length, but when you renew it the length is 3 text boxes which implies that you can fill them all in - or that you have to fill them all in, but I assume you're just supposed to fill in one like you do when you add a subscription.
Created attachment 5939 [details] subscription length when adding
Also, "Number of num:" should be "Number of issues:"
Still valid in current master.
Still valid.
Created attachment 69355 [details] [review] Bug 7046 - Added subscription length dropdown box
Created attachment 69356 [details] [review] Bug 7046 - Implemented dropdown sub length element To make this work I moved the _get_sub_length function from subscription-add.pl to C4/Serials.pm so that the subscription-renew.pl script could also call it to store the sublength for the appropriate field of the subscriptions database table. Test plan: 1. Create a subscription and notice that there is a dropdown box for sub length containing the values: issues, weeks, months 2. Renew the subscription and notice that there are 3 input text boxes: 'number of num', 'number of weeks' and 'number of months' 3. Input a 'Number of weeks' value of 2 4. Query the subscription database table and notice that the value of 2 has been stored in the weeklength field for the subscription record you just renewed 5. Apply the patch 6. Renew the subscription and notice that there is now a sublength dropdown box containing issues, weeks and months 7. Set the month value to 3 8. Query the database and notice that 3 was stored in the monthlength field for the subscription record 9. Create a new subscription and select the sub length values of issues and 3 10. Query the database and notice that the numberlength field for the subscription you just created is set to 3 showing that the sublength dropbox is still working for creating a new subscription Sponsored-By: Catalyst IT
Created attachment 69408 [details] [review] Bug 7046 - Implemented dropdown sub length element Patch applies and functions as expected in line with test plan. Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
(In reply to Dilan Johnpullé from comment #7) > Created attachment 69408 [details] [review] [review] > Bug 7046 - Implemented dropdown sub length element > > Patch applies and functions as expected in line with test plan. > Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au> Dilan, please do not remove the original commit message when you are sign-offing. We need it in git for better understanding of commit content/funcionality.
Comment on attachment 69408 [details] [review] Bug 7046 - Implemented dropdown sub length element Review of attachment 69408 [details] [review]: ----------------------------------------------------------------- ::: C4/Serials.pm @@ +1509,5 @@ > + $subtype eq 'issues' ? $length : 0, > + $subtype eq 'weeks' ? $length : 0, > + $subtype eq 'months' ? $length : 0, > + ); > +} Now it is not internal subroutine so the name should not start with underscores. Tests are needed for new subroutines in modules. Some refactoring using objects would be nice, but it is out of the scope of this bug report IMO, so no blocker. ::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt @@ +32,4 @@ > </li> > <li><fieldset> > <legend>Subscription length:</legend> > + <select name="subtype" id="subtype"> Here are tabs at beginning, need to be removed
Created attachment 81641 [details] [review] Bug 7046: Implemented subscription renewal dropdown sub length element To make this work I moved the _get_sub_length function from subscription-add.pl to C4/Serials.pm so that the subscription-renew.pl script could also call it to store the sublength for the appropriate field of the subscriptions database table. Test plan: 1. Create a subscription and notice that there is a dropdown box for sub length containing the values: issues, weeks, months 2. Renew the subscription and notice that there are 3 input text boxes: 'number of num', 'number of weeks' and 'number of months' 3. Input a 'Number of weeks' value of 2 4. Query the subscription database table and notice that the value of 2 has been stored in the weeklength field for the subscription record you just renewed 5. Apply the patch 6. Renew the subscription and notice that there is now a sublength dropdown box containing issues, weeks and months 7. Set the month value to 3 8. Query the database and notice that 3 was stored in the monthlength field for the subscription record 9. Create a new subscription and select the sub length values of issues and 3 10. Query the database and notice that the numberlength field for the subscription you just created is set to 3 showing that the sublength dropbox is still working for creating a new subscription Sponsored-By: Catalyst IT Patch applies and functions as expected in line with test plan. Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Created attachment 81642 [details] [review] Bug 7046: (follow-up) Tidy up to address test feedback * Removed underscores from start of subroutine * Removed tabs in subscription-renew.tt * Added test for C4/Serials/getsublength() in t/db_dependen/Serials/ReNewSubscription.t * Added missing filters Sponsored-By: Catalyst IT
When you click on the button "Renew subscription", there is warning: Use of uninitialized value $subtype in string eq at /home/vagrant/kohaclone/C4/Serials.pm line 1468
Comment on attachment 81642 [details] [review] Bug 7046: (follow-up) Tidy up to address test feedback Review of attachment 81642 [details] [review]: ----------------------------------------------------------------- ::: C4/Serials.pm @@ +1453,4 @@ > return $subscriptionid; > } > > +=head2 getsublength in C4 namespace it should be spelled "GetSubLength" or "GetSubscriptionLength" ::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt @@ +65,5 @@ > [% END %] > </option> > [% END %] > </select> > + <input type="text" name="sublength" id="sublength" value="[% sublength | html %]" size="3" />(enter amount in numerals) The "sublength" param is not passed from perl script, so this input is always empty ::: t/db_dependent/Serials/ReNewSubscription.t @@ +85,5 @@ > # Actual testing starts here! > > +# Calculate the subscription length for the renewal for issues, days and months > +my ($numberlength, $weeklength, $monthlength) = getsublength('issues', 7); > +is ( $numberlength, 7, "Sub length is 7 issues"); You should test other variables too. Also, you should test bad params too - undef, other string, ...
Created attachment 102275 [details] [review] Bug 7046: Implemented subscription renewal dropdown sub length element To make this work I moved the _get_sub_length function from subscription-add.pl to C4/Serials.pm so that the subscription-renew.pl script could also call it to store the sublength for the appropriate field of the subscriptions database table. Test plan: 1. Create a subscription and notice that there is a dropdown box for sub length containing the values: issues, weeks, months 2. Renew the subscription and notice that there are 3 input text boxes: 'number of num', 'number of weeks' and 'number of months' 3. Input a 'Number of weeks' value of 2 4. Query the subscription database table and notice that the value of 2 has been stored in the weeklength field for the subscription record you just renewed 5. Apply the patch 6. Renew the subscription and notice that there is now a sublength dropdown box containing issues, weeks and months 7. Set the month value to 3 8. Query the database and notice that 3 was stored in the monthlength field for the subscription record 9. Create a new subscription and select the sub length values of issues and 3 10. Query the database and notice that the numberlength field for the subscription you just created is set to 3 showing that the sublength dropbox is still working for creating a new subscription Sponsored-By: Catalyst IT Patch applies and functions as expected in line with test plan. Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Created attachment 102276 [details] [review] Bug 7046: (follow-up) Tidy up to address test feedback * Removed underscores from start of subroutine * Removed tabs in subscription-renew.tt * Added test for C4/Serials/getsublength() in t/db_dependen/Serials/ReNewSubscription.t * Added missing filters Sponsored-By: Catalyst IT
Created attachment 102278 [details] [review] Bug 7046: (follow-up) Changed subroutine name and added unit tests * Fixed uninitialised value in $subtype in C4/Serials.pm line 1468 * Fixed sub routine name to 'GetSubscriptionLength' * Removed sublength param from being in value of sublength HTML input field * Added more unit tests to include bad params: undef, letters and special characters Test plan: Please follow test plan in first patch Sponsored-By: Catalyst IT
Hi all, I've rebased the first two commits against master and done a followup patch to address Josef's points in comment #12 and #13. Please let me know if I should remove the sign off line in the first commit, and if there are further changes to these commits you feel would be useful. Many thanks, Alex
Hi Alex. When I go to add a ne subscription (step 9) I get an error when trying to save the subscription: Can't locate object method "getsublength" via package "C4::Serials" at /kohadevbox/koha/serials/subscription-add.pl line 322 in CGI::Compile::ROOT::kohadevbox_koha_serials_subscription_2dadd_2epl::redirect_add_subscription at /kohadevbox/koha/serials/subscription-add.pl line 322 319: 320: my $subtype = $query->param('subtype'); 321: my $sublength = $query->param('sublength'); 322: my ( $numberlength, $weeklength, $monthlength ) 323: = C4::Serials->getsublength( $subtype, $sublength ); 324: my $add1 = $query->param('add1'); 325: my $lastvalue1 = $query->param('lastvalue1'); in (eval) at /kohadevbox/koha/serials/subscription-add.pl line 184 181: } 182: 183: if ($op eq 'addsubscription') { 184: redirect_add_subscription(); 185: } elsif ($op eq 'modsubscription') { 186: redirect_mod_subscription(); 187: } else { in CGI::Compile::ROOT::kohadevbox_koha_serials_subscription_2dadd_2epl::__ANON__ at /kohadevbox/koha/serials/subscription-add.pl line 519 516: return; 517: } Show function arguments
Hi David, Thanks so much for testing, and reporting that problem. I am fixing that now. Thanks, Alex
Created attachment 102425 [details] [review] Bug 7046: (follow-up) Update function call in subscription-add.pl Also removed border around Subscription length in subscription-renew.tt Test plan: Please follow test plan in first patch Sponsored-By: Catalyst IT
Hi David, That should now be fixed up. I also removed the border around the subscription length in subscription-renew.tt so that it is centered on the page along with the other labels and inputs. Can you please re-test? Many thanks, Alex
Created attachment 102427 [details] [review] Bug 7046: Implemented subscription renewal dropdown sub length element To make this work I moved the _get_sub_length function from subscription-add.pl to C4/Serials.pm so that the subscription-renew.pl script could also call it to store the sublength for the appropriate field of the subscriptions database table. Test plan: 1. Create a subscription and notice that there is a dropdown box for sub length containing the values: issues, weeks, months 2. Renew the subscription and notice that there are 3 input text boxes: 'number of num', 'number of weeks' and 'number of months' 3. Input a 'Number of weeks' value of 2 4. Query the subscription database table and notice that the value of 2 has been stored in the weeklength field for the subscription record you just renewed 5. Apply the patch 6. Renew the subscription and notice that there is now a sublength dropdown box containing issues, weeks and months 7. Set the month value to 3 8. Query the database and notice that 3 was stored in the monthlength field for the subscription record 9. Create a new subscription and select the sub length values of issues and 3 10. Query the database and notice that the numberlength field for the subscription you just created is set to 3 showing that the sublength dropbox is still working for creating a new subscription Sponsored-By: Catalyst IT Patch applies and functions as expected in line with test plan. Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102428 [details] [review] Bug 7046: (follow-up) Tidy up to address test feedback * Removed underscores from start of subroutine * Removed tabs in subscription-renew.tt * Added test for C4/Serials/getsublength() in t/db_dependen/Serials/ReNewSubscription.t * Added missing filters Sponsored-By: Catalyst IT Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102429 [details] [review] Bug 7046: (follow-up) Changed subroutine name and added unit tests * Fixed uninitialised value in $subtype in C4/Serials.pm line 1468 * Fixed sub routine name to 'GetSubscriptionLength' * Removed sublength param from being in value of sublength HTML input field * Added more unit tests to include bad params: undef, letters and special characters Test plan: Please follow test plan in first patch Sponsored-By: Catalyst IT Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102430 [details] [review] Bug 7046: (follow-up) Update function call in subscription-add.pl Also removed border around Subscription length in subscription-renew.tt Test plan: Please follow test plan in first patch Sponsored-By: Catalyst IT Signed-off-by: David Nind <david@davidnind.com>
Thank you very much David!
I think this is not correct: + my $numberpattern = $query->param('numbering_pattern'); And it displays a warning in the log "my" variable $numberpattern masks earlier declaration in same scope at /kohadevbox/koha/serials/subscription-add.pl line 437.
Created attachment 102535 [details] [review] Bug 7046: Simplify dropdown list construction
Created attachment 102536 [details] [review] Bug 7046: Remove "earlier declaration in same scope" "my" variable $numberpattern masks earlier declaration in same scope at /kohadevbox/koha/serials/subscription-add.pl line 437.
I am working on this, I do not think it works correctly.
Created attachment 102537 [details] [review] Bug 7046: Implemented subscription renewal dropdown sub length element To make this work I moved the _get_sub_length function from subscription-add.pl to C4/Serials.pm so that the subscription-renew.pl script could also call it to store the sublength for the appropriate field of the subscriptions database table. Test plan: 1. Create a subscription and notice that there is a dropdown box for sub length containing the values: issues, weeks, months 2. Renew the subscription and notice that there are 3 input text boxes: 'number of num', 'number of weeks' and 'number of months' 3. Input a 'Number of weeks' value of 2 4. Query the subscription database table and notice that the value of 2 has been stored in the weeklength field for the subscription record you just renewed 5. Apply the patch 6. Renew the subscription and notice that there is now a sublength dropdown box containing issues, weeks and months 7. Set the month value to 3 8. Query the database and notice that 3 was stored in the monthlength field for the subscription record 9. Create a new subscription and select the sub length values of issues and 3 10. Query the database and notice that the numberlength field for the subscription you just created is set to 3 showing that the sublength dropbox is still working for creating a new subscription Sponsored-By: Catalyst IT Patch applies and functions as expected in line with test plan. Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au> Signed-off-by: David Nind <david@davidnind.com> Bug 7046: (follow-up) Tidy up to address test feedback * Removed underscores from start of subroutine * Removed tabs in subscription-renew.tt * Added test for C4/Serials/getsublength() in t/db_dependen/Serials/ReNewSubscription.t * Added missing filters Sponsored-By: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Bug 7046: (follow-up) Changed subroutine name and added unit tests * Fixed uninitialised value in $subtype in C4/Serials.pm line 1468 * Fixed sub routine name to 'GetSubscriptionLength' * Removed sublength param from being in value of sublength HTML input field * Added more unit tests to include bad params: undef, letters and special characters Test plan: Please follow test plan in first patch Sponsored-By: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Bug 7046: (follow-up) Update function call in subscription-add.pl Also removed border around Subscription length in subscription-renew.tt Test plan: Please follow test plan in first patch Sponsored-By: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 102538 [details] [review] Bug 7046: Preselect dropdown option and prefill input With previous renew values Bug 7046: Simplify dropdown list construction Bug 7046: Remove "earlier declaration in same scope" "my" variable $numberpattern masks earlier declaration in same scope at /kohadevbox/koha/serials/subscription-add.pl line 437. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Hello Alex, I squashed your patches for readability (sub renamed, etc.). Also the follow-up restores a previous behaviour, we need to pre-fill the renew form with info from the previous renew. (In reply to Jonathan Druart from comment #30) > I am working on this, I do not think it works correctly. It did, apart from the pre-fill, it was my follow-up!
Hi Jonathan, Ah OK yes that makes sense. Many thanks for rebasing my patches and adding that follow up to pre-fill the renew form! Thanks, Alex
Nice work everyone! Pushed to master for 20.05
Many thanks everyone!
Created attachment 102564 [details] [review] Bug 7046: (RM follow-up) Add Filters Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
enhancement not backported to 19.11.x