Bug 36981

Summary: Check for subscription length value
Product: Koha Reporter: pierre.genty
Component: SerialsAssignee: Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: trivial    
Priority: P5 - low CC: alban.peyrat_koha, baptiste.wojtkowski, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 36981: Check for subscription length value
Bug 36981: Check for subscription length value

Description pierre.genty 2024-05-29 08:57:02 UTC
When editing serial subscription, it is possible to set amount such as 999999999 in "Subscription length" field. 
If no end date is set this could cause an infinite loop.
Comment 1 Baptiste Wojtkowski (bwoj) 2024-07-01 09:41:08 UTC
Created attachment 168308 [details] [review]
Bug 36981: Check for subscription length value

Original comment: When editing serial subscription, it is possible to set amount such as 999999999 in "Subscription length" field.
If no end date is set this could cause an infinite loop.

I changed all concerned fields toward number fields with minimum 0 and
max 2000 (arbitrary but I think it is nearly impossible to reach such an
        high number)

TEST PLAN:
1 - Apply patch
2 - go to Serials -> search -> pick a subsciption
3 - Click on Edit->Edit subscription
4 - fill correct infos and go to page 2
5 - notice that the second field after the label subscription length is
  a number field
6 - try and fill -1 in this field -> notice it is impossible to save
7 - try and fill 5000 in this field -> notice it is impossible to save
8 - Repeat the process py clicking on "Renew" at step (3)
9 - Repeat the process py clicking on "numbering patterns" at step (3)
  (note, you can test the pattern but when clicking done you will be
   prevented from)
Comment 2 Baptiste Wojtkowski (bwoj) 2024-07-09 11:04:57 UTC
Note : changing from enhancement to trivial since providing a too high value may impair the normal operation of Koha
Comment 3 David Nind 2024-07-11 04:14:17 UTC
Created attachment 168773 [details] [review]
Bug 36981: Check for subscription length value

Original comment: When editing serial subscription, it is possible to set amount such as 999999999 in "Subscription length" field.
If no end date is set this could cause an infinite loop.

I changed all concerned fields toward number fields with minimum 0 and
max 2000 (arbitrary but I think it is nearly impossible to reach such an
        high number)

TEST PLAN:
1 - Apply patch
2 - go to Serials -> search -> pick a subsciption
3 - Click on Edit->Edit subscription
4 - fill correct infos and go to page 2
5 - notice that the second field after the label subscription length is
  a number field
6 - try and fill -1 in this field -> notice it is impossible to save
7 - try and fill 5000 in this field -> notice it is impossible to save
8 - Repeat the process py clicking on "Renew" at step (3)
9 - Repeat the process py clicking on "numbering patterns" at step (3)
  (note, you can test the pattern but when clicking done you will be
   prevented from)

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Katrin Fischer 2024-07-12 07:50:21 UTC
I am not sure 200 is a good default here. If you pick "issues" and you have a daily newspaper, it would be 365 at least.
Saying: I don't think 200 is big enough for common use cases here.
Comment 5 Baptiste Wojtkowski (bwoj) 2024-07-12 08:17:35 UTC
Hi Karin, thank you for your reply !

You might have misread the patch, I proposed 2000 as a limit, not 200, is it still too low ?
Comment 6 Katrin Fischer 2024-07-12 08:19:29 UTC
Ooops!
Comment 7 Baptiste Wojtkowski (bwoj) 2024-07-12 08:20:00 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2024-07-12 08:20:29 UTC
Value is high, but not sure if that is the best approach. Leaving it for proper QA. Thanks for the quick feedback!
Comment 9 Marcel de Rooy 2024-08-02 09:29:53 UTC
Understand concerns of Katrin. This does not feel like the best solution. More a workaround.
Where does it cause the infinite loop exactly? Can we fix that? Please provide some more details.
Comment 10 Marcel de Rooy 2024-08-16 07:37:24 UTC
Please respond to raised questions. Changing status
Comment 11 Baptiste Wojtkowski (bwoj) 2024-09-27 13:06:55 UTC
Sry for taking long time to answer, needed to reproduce the pb I did not experiences myself ^^

When loading subscription-details.pl, it computes every occurrence of each record, so if the number of records is too high, it will compute every single occurrence, which may take time. It's not an infinite loop but it can be very long.

I think this comes from here:

> 68 
> 69 my ($totalissues,@serialslist) = GetSerials($subscriptionid);
> 70 $totalissues-- if $totalissues; # the -1 is to have 0 if this is a new > subscription (only 1 issue)
> 71
Comment 12 Marcel de Rooy 2024-11-15 08:01:38 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-numberpatterns.tt
   FAIL   forbidden patterns
                forbidden pattern: Don't use type=number for inputs (bug 17098) (line 216)

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt
   FAIL   forbidden patterns
                forbidden pattern: Don't use type=number for inputs (bug 17098) (line 90)