Bug 36981 - Check for subscription length value
Summary: Check for subscription length value
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial
Assignee: Baptiste Wojtkowski (bwoj)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-29 08:57 UTC by pierre.genty
Modified: 2024-09-30 10:04 UTC (History)
3 users (show)

See Also:
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 (4.07 KB, patch)
2024-07-01 09:41 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 36981: Check for subscription length value (4.13 KB, patch)
2024-07-11 04:14 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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