Bug 18356 - Prediction pattern wrong, skips years, for some year based frequencies
Summary: Prediction pattern wrong, skips years, for some year based frequencies
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 16116 (view as bug list)
Depends on:
Blocks: 18607
  Show dependency treegraph
 
Reported: 2017-03-30 20:41 UTC by Nick Clemens
Modified: 2017-07-31 11:31 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18356: Extend GetNextDate.t, add GetFictiveIssueNumber.t (unit=year) (7.87 KB, patch)
2017-05-15 13:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18356: Fix date calculations for yearly frequencies in Serials (4.96 KB, patch)
2017-05-15 13:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18356: Extend GetNextDate.t, add GetFictiveIssueNumber.t (unit=year) (7.82 KB, patch)
2017-05-29 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18356: Fix date calculations for yearly frequencies in Serials (5.62 KB, patch)
2017-05-29 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18356: Extend GetNextDate.t, add GetFictiveIssueNumber.t (unit=year) (8.05 KB, patch)
2017-05-31 14:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18356: Fix date calculations for yearly frequencies in Serials (5.80 KB, patch)
2017-05-31 14:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18356: Extend GetNextDate.t, add GetFictiveIssueNumber.t (unit=year) (8.08 KB, patch)
2017-06-06 07:30 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18356: Fix date calculations for yearly frequencies in Serials (5.83 KB, patch)
2017-06-06 07:30 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18356: Extend GetNextDate.t, add GetFictiveIssueNumber.t (unit=year) (8.12 KB, patch)
2017-06-15 11:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18356: Fix date calculations for yearly frequencies in Serials (5.88 KB, patch)
2017-06-15 11:21 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-03-30 20:41:09 UTC
To reproduce:
Set a frequency with:
Unit: Year
Issues per unit: 9
Units per issue: 1

OR 

Unit: Year
Issues per unit: 12
Units per issue: 1


Notice the prediction pattern works for about 1 cycle, then crossed to next year, then jumps a year:

No. 82 	03/01/2017 	
No. 83 	03/31/2017 	
No. 84 	04/30/2017 	
No. 85 	05/30/2017 	
No. 86 	06/29/2017 	
No. 87 	07/29/2017 	
No. 88 	08/28/2017 	
No. 89 	09/27/2017 	
No. 90 	10/27/2017 	
No. 91 	11/26/2017 	
No. 92 	12/26/2017 	
No. 93 	01/25/2018  
No. 94 	03/01/2019 WRONG!!
No. 95 	03/31/2019 	
No. 96 	04/30/2019 	
No. 97 	05/30/2019 	
No. 98 	06/29/2019 	
No. 99 	07/29/2019 	
No. 100 	08/28/2019 	
No. 101 	09/27/2019 	
No. 102 	10/27/2019 	
No. 103 	11/26/2019 	
No. 104 	12/26/2019 	
No. 105 	01/25/2020 	
No. 106 	03/01/2021 WRONG!!

Seems to be a problem in some cases when we call:

($year) = Add_Delta_YM($year,$month,$day, $freqdata->{"unitsperissue"},0);

As this call will always add a year, it ends up in:
/usr/share/perl5/Date/Calc/PP.pm
sub DateCalc_add_year_month

Where we add $year + $freqdata->{"unitsperissue"} and return 1 if it is greater than 1

Suspecting the pass of $freqdata->{"unitsperissue"} is incorrect
Comment 1 Marcel de Rooy 2017-05-09 14:10:10 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2017-05-15 13:12:03 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-05-15 13:12:08 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-05-18 07:56:49 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2017-05-29 09:54:23 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2017-05-29 09:54:28 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2017-05-29 09:55:45 UTC
Will add links to follow-up reports that handle the other frequencies.
Comment 8 Marcel de Rooy 2017-05-31 14:37:54 UTC
Created attachment 63875 [details] [review]
Bug 18356: Extend GetNextDate.t, add GetFictiveIssueNumber.t (unit=year)

This patch deals with tests for yearly frequencies.

Adjust/extend GetNextDate.t:
[1] Adjust mixup of units/issues in a description.
[2] Add testing +2 years on 29-2 of leap year for freq 1 issue/2 years.
[3] Add tests for freq 9 issues/year.

Add GetFictiveIssueNumber.t:
[1] Two subtests are provided for irregular frequencies (very trivial) and
    for year frequencies (with four specific test cases).

Test plan:
[1] Run t/db_dependent/Serials/GetNextDate.t
[2] Run t/db_dependent/Serials/GetFictiveIssueNumber.t

Note: Without the second patch both tests should fail. This shows the need
of the adjustments in the second patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-05-31 14:37:59 UTC
Created attachment 63876 [details] [review]
Bug 18356: Fix date calculations for yearly frequencies in Serials

The problem as described on BZ 18356 is a combination of an error in
GetFictiveIssueNumber and GetNextDate for unit==year.

[1] In GetNextDate the Add_Delta_YM calculation should be applied only to
frequencies based on years per unit.
In the case of multiple units per year we calculate the number of days to
add. And if we have reached the end of a cycle, we correct the
rounding applied in the cycle.
NOTE 1: We obsolete the idea here of rebasing dates on firstacqui. In case
of manual adjustments, we probably do not want it. And otherwise we do not
need it anymore due to the correction at the end of a cycle.
NOTE 2: The calls to Add_Delta_YM are intentionally not corrected for leap
years. Say you start at 2016-02-29. If you use 1/yr or 1/2yr, you will
switch to the Feb 28th in the following years. In 2020 there will be no
switch to Feb 29 again; if someone should need it, please use a manual
adjustment. This is probably highly exceptional.

[2] In GetFictiveIssueNumber the year should be decreased by one if you
have more units per year and you did not yet reach firstacqui day and
month. This affects calculations in GetNextDate with irregularities.
NOTE 1: I added a wrapper around Date::Calc::N_Delta_YMD in order to improve
its results; this will especially be needed when we use it later for
month units.
NOTE 2: In case of manual adjustments this calculation does not really make
sense. Another report should deal with improving irregularities.

Test plan:
[1] Verify that both GetNextDate.t as well as GetFictiveIssueNumber.t pass.
[2] Look at the prediction pattern for a few frequencies.
    For example: 1 iss/y, 1 iss/2y, 5 iss/y.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Josef Moravec 2017-06-06 07:30:14 UTC
Created attachment 64021 [details] [review]
[SIGNED-OFF] Bug 18356: Extend GetNextDate.t, add GetFictiveIssueNumber.t (unit=year)

This patch deals with tests for yearly frequencies.

Adjust/extend GetNextDate.t:
[1] Adjust mixup of units/issues in a description.
[2] Add testing +2 years on 29-2 of leap year for freq 1 issue/2 years.
[3] Add tests for freq 9 issues/year.

Add GetFictiveIssueNumber.t:
[1] Two subtests are provided for irregular frequencies (very trivial) and
    for year frequencies (with four specific test cases).

Test plan:
[1] Run t/db_dependent/Serials/GetNextDate.t
[2] Run t/db_dependent/Serials/GetFictiveIssueNumber.t

Note: Without the second patch both tests should fail. This shows the need
of the adjustments in the second patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Josef Moravec 2017-06-06 07:30:18 UTC
Created attachment 64022 [details] [review]
[SIGNED-OFF] Bug 18356: Fix date calculations for yearly frequencies in Serials

The problem as described on BZ 18356 is a combination of an error in
GetFictiveIssueNumber and GetNextDate for unit==year.

[1] In GetNextDate the Add_Delta_YM calculation should be applied only to
frequencies based on years per unit.
In the case of multiple units per year we calculate the number of days to
add. And if we have reached the end of a cycle, we correct the
rounding applied in the cycle.
NOTE 1: We obsolete the idea here of rebasing dates on firstacqui. In case
of manual adjustments, we probably do not want it. And otherwise we do not
need it anymore due to the correction at the end of a cycle.
NOTE 2: The calls to Add_Delta_YM are intentionally not corrected for leap
years. Say you start at 2016-02-29. If you use 1/yr or 1/2yr, you will
switch to the Feb 28th in the following years. In 2020 there will be no
switch to Feb 29 again; if someone should need it, please use a manual
adjustment. This is probably highly exceptional.

[2] In GetFictiveIssueNumber the year should be decreased by one if you
have more units per year and you did not yet reach firstacqui day and
month. This affects calculations in GetNextDate with irregularities.
NOTE 1: I added a wrapper around Date::Calc::N_Delta_YMD in order to improve
its results; this will especially be needed when we use it later for
month units.
NOTE 2: In case of manual adjustments this calculation does not really make
sense. Another report should deal with improving irregularities.

Test plan:
[1] Verify that both GetNextDate.t as well as GetFictiveIssueNumber.t pass.
[2] Look at the prediction pattern for a few frequencies.
    For example: 1 iss/y, 1 iss/2y, 5 iss/y.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Kyle M Hall 2017-06-15 11:20:52 UTC
Created attachment 64310 [details] [review]
Bug 18356: Extend GetNextDate.t, add GetFictiveIssueNumber.t (unit=year)

This patch deals with tests for yearly frequencies.

Adjust/extend GetNextDate.t:
[1] Adjust mixup of units/issues in a description.
[2] Add testing +2 years on 29-2 of leap year for freq 1 issue/2 years.
[3] Add tests for freq 9 issues/year.

Add GetFictiveIssueNumber.t:
[1] Two subtests are provided for irregular frequencies (very trivial) and
    for year frequencies (with four specific test cases).

Test plan:
[1] Run t/db_dependent/Serials/GetNextDate.t
[2] Run t/db_dependent/Serials/GetFictiveIssueNumber.t

Note: Without the second patch both tests should fail. This shows the need
of the adjustments in the second patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2017-06-15 11:21:24 UTC
Created attachment 64311 [details] [review]
Bug 18356: Fix date calculations for yearly frequencies in Serials

The problem as described on BZ 18356 is a combination of an error in
GetFictiveIssueNumber and GetNextDate for unit==year.

[1] In GetNextDate the Add_Delta_YM calculation should be applied only to
frequencies based on years per unit.
In the case of multiple units per year we calculate the number of days to
add. And if we have reached the end of a cycle, we correct the
rounding applied in the cycle.
NOTE 1: We obsolete the idea here of rebasing dates on firstacqui. In case
of manual adjustments, we probably do not want it. And otherwise we do not
need it anymore due to the correction at the end of a cycle.
NOTE 2: The calls to Add_Delta_YM are intentionally not corrected for leap
years. Say you start at 2016-02-29. If you use 1/yr or 1/2yr, you will
switch to the Feb 28th in the following years. In 2020 there will be no
switch to Feb 29 again; if someone should need it, please use a manual
adjustment. This is probably highly exceptional.

[2] In GetFictiveIssueNumber the year should be decreased by one if you
have more units per year and you did not yet reach firstacqui day and
month. This affects calculations in GetNextDate with irregularities.
NOTE 1: I added a wrapper around Date::Calc::N_Delta_YMD in order to improve
its results; this will especially be needed when we use it later for
month units.
NOTE 2: In case of manual adjustments this calculation does not really make
sense. Another report should deal with improving irregularities.

Test plan:
[1] Verify that both GetNextDate.t as well as GetFictiveIssueNumber.t pass.
[2] Look at the prediction pattern for a few frequencies.
    For example: 1 iss/y, 1 iss/2y, 5 iss/y.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Jonathan Druart 2017-06-19 19:37:12 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 15 Marcel de Rooy 2017-06-20 14:58:21 UTC
*** Bug 16116 has been marked as a duplicate of this bug. ***
Comment 16 Fridolin SOMERS 2017-06-29 07:06:48 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 17 Katrin Fischer 2017-06-29 20:27:47 UTC
These patches have been pushed to 16.11.x and will be in 16.11.10.
Comment 18 Mason James 2017-07-31 11:31:54 UTC
Pushed to 16.05.x, for 16.05.15 release