Bug 7046 - subscription renewal period should be a pull down
Summary: subscription renewal period should be a pull down
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Buckley
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-17 17:46 UTC by Nicole C. Engard
Modified: 2020-05-01 17:41 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This enhancement changes the form for the serials renewal period for a subscription into a drop down list - this is consistent with the create subscription form.
Version(s) released in:
20.05.00


Attachments
renew subscription length (31.61 KB, image/png)
2011-10-17 17:46 UTC, Nicole C. Engard
Details
subscription length when adding (25.32 KB, image/png)
2011-10-17 17:46 UTC, Nicole C. Engard
Details
Bug 7046 - Added subscription length dropdown box (4.67 KB, patch)
2017-11-27 02:34 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 7046 - Implemented dropdown sub length element (8.13 KB, patch)
2017-11-27 04:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 7046 - Implemented dropdown sub length element (7.07 KB, patch)
2017-11-28 01:11 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 7046: Implemented subscription renewal dropdown sub length element (8.01 KB, patch)
2018-10-31 02:42 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 7046: (follow-up) Tidy up to address test feedback (7.11 KB, patch)
2018-10-31 02:42 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 7046: Implemented subscription renewal dropdown sub length element (8.55 KB, patch)
2020-04-02 09:18 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 7046: (follow-up) Tidy up to address test feedback (6.92 KB, patch)
2020-04-02 09:18 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 7046: (follow-up) Changed subroutine name and added unit tests (7.84 KB, patch)
2020-04-02 09:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 7046: (follow-up) Update function call in subscription-add.pl (2.29 KB, patch)
2020-04-05 22:53 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 7046: Implemented subscription renewal dropdown sub length element (8.60 KB, patch)
2020-04-06 00:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 7046: (follow-up) Tidy up to address test feedback (6.96 KB, patch)
2020-04-06 00:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 7046: (follow-up) Changed subroutine name and added unit tests (7.88 KB, patch)
2020-04-06 00:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 7046: (follow-up) Update function call in subscription-add.pl (2.33 KB, patch)
2020-04-06 00:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 7046: Simplify dropdown list construction (2.77 KB, patch)
2020-04-08 08:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7046: Remove "earlier declaration in same scope" (1.05 KB, patch)
2020-04-08 08:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7046: Implemented subscription renewal dropdown sub length element (14.16 KB, patch)
2020-04-08 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7046: Preselect dropdown option and prefill input (4.27 KB, patch)
2020-04-08 09:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7046: (RM follow-up) Add Filters (1.75 KB, patch)
2020-04-08 12:19 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-10-17 17:46:24 UTC
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.
Comment 1 Nicole C. Engard 2011-10-17 17:46:42 UTC
Created attachment 5939 [details]
subscription length when adding
Comment 2 Nicole C. Engard 2011-10-17 17:57:05 UTC
Also, "Number of num:" should be "Number of issues:"
Comment 3 Katrin Fischer 2014-03-26 22:48:25 UTC
Still valid in current master.
Comment 4 Katrin Fischer 2015-04-28 12:05:01 UTC
Still valid.
Comment 5 Alex Buckley 2017-11-27 02:34:49 UTC
Created attachment 69355 [details] [review]
Bug 7046 - Added subscription length dropdown box
Comment 6 Alex Buckley 2017-11-27 04:08:53 UTC
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
Comment 7 Dilan Johnpullé 2017-11-28 01:11:08 UTC
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>
Comment 8 Josef Moravec 2017-12-05 08:33:22 UTC
(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 9 Josef Moravec 2017-12-05 08:58:23 UTC
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
Comment 10 Alex Buckley 2018-10-31 02:42:31 UTC
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>
Comment 11 Alex Buckley 2018-10-31 02:42:52 UTC
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
Comment 12 Josef Moravec 2018-10-31 09:14:34 UTC
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 13 Josef Moravec 2018-10-31 09:19:41 UTC
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, ...
Comment 14 Alex Buckley 2020-04-02 09:18:06 UTC
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>
Comment 15 Alex Buckley 2020-04-02 09:18:27 UTC
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
Comment 16 Alex Buckley 2020-04-02 09:25:50 UTC
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
Comment 17 Alex Buckley 2020-04-02 09:28:05 UTC
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
Comment 18 David Nind 2020-04-05 09:39:45 UTC
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
Comment 19 Alex Buckley 2020-04-05 20:39:12 UTC
Hi David, 

Thanks so much for testing, and reporting that problem. 

I am fixing that now. 

Thanks,
Alex
Comment 20 Alex Buckley 2020-04-05 22:53:09 UTC
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
Comment 21 Alex Buckley 2020-04-05 23:00:57 UTC
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
Comment 22 David Nind 2020-04-06 00:05:57 UTC
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>
Comment 23 David Nind 2020-04-06 00:06:01 UTC
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>
Comment 24 David Nind 2020-04-06 00:06:06 UTC
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>
Comment 25 David Nind 2020-04-06 00:06:10 UTC
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>
Comment 26 Alex Buckley 2020-04-06 00:55:10 UTC
Thank you very much David!
Comment 27 Jonathan Druart 2020-04-08 08:47:39 UTC
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.
Comment 28 Jonathan Druart 2020-04-08 08:54:25 UTC
Created attachment 102535 [details] [review]
Bug 7046: Simplify dropdown list construction
Comment 29 Jonathan Druart 2020-04-08 08:54:29 UTC
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.
Comment 30 Jonathan Druart 2020-04-08 09:05:04 UTC
I am working on this, I do not think it works correctly.
Comment 31 Jonathan Druart 2020-04-08 09:15:58 UTC
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>
Comment 32 Jonathan Druart 2020-04-08 09:16:02 UTC
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>
Comment 33 Jonathan Druart 2020-04-08 09:18:23 UTC
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!
Comment 34 Alex Buckley 2020-04-08 09:23:57 UTC
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
Comment 35 Martin Renvoize 2020-04-08 10:55:15 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 36 Alex Buckley 2020-04-08 10:56:23 UTC
Many thanks everyone!
Comment 37 Martin Renvoize 2020-04-08 12:19:20 UTC
Created attachment 102564 [details] [review]
Bug 7046: (RM follow-up) Add Filters

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Joy Nelson 2020-05-01 17:41:06 UTC
enhancement not backported to 19.11.x