Bug 18697 - Fix date calculations for day/week frequencies in Serials
Summary: Fix date calculations for day/week frequencies in Serials
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:
Depends on: 18607
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-30 12:37 UTC by Marcel de Rooy
Modified: 2017-07-31 11:33 UTC (History)
5 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 18697: Fix date calculations for weekly frequencies in Serials (4.92 KB, patch)
2017-05-30 14:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18697: Adjusting unit tests for weekly serial frequencies (4.37 KB, patch)
2017-05-30 14:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18697: Fix date calculations for weekly frequencies in Serials (3.92 KB, patch)
2017-05-31 14:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18697: Adjusting unit tests for weekly serial frequencies (4.46 KB, patch)
2017-05-31 14:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18697: Fix date calculation for dayly frequencies in Serials (3.87 KB, patch)
2017-05-31 14:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18697: Adjusting unit tests for dayly serial frequencies (2.64 KB, patch)
2017-05-31 14:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18697: Final polishing (3.86 KB, patch)
2017-05-31 14:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18697: Fix date calculations for weekly frequencies in Serials (3.95 KB, patch)
2017-06-06 07:52 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18697: Adjusting unit tests for weekly serial frequencies (4.49 KB, patch)
2017-06-06 07:52 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18697: Fix date calculation for dayly frequencies in Serials (3.89 KB, patch)
2017-06-06 07:52 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18697: Adjusting unit tests for dayly serial frequencies (2.67 KB, patch)
2017-06-06 07:52 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18697: Final polishing (3.89 KB, patch)
2017-06-06 07:52 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18697: Fix date calculations for weekly frequencies in Serials (3.99 KB, patch)
2017-06-15 13:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18697: Adjusting unit tests for weekly serial frequencies (4.53 KB, patch)
2017-06-15 13:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18697: Fix date calculation for dayly frequencies in Serials (3.94 KB, patch)
2017-06-15 13:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18697: Adjusting unit tests for dayly serial frequencies (2.71 KB, patch)
2017-06-15 13:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18697: Final polishing (3.93 KB, patch)
2017-06-15 13:24 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 Marcel de Rooy 2017-05-30 12:37:33 UTC
See also bug 18356 and 18607.
Comment 1 Marcel de Rooy 2017-05-30 14:25:31 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2017-05-30 14:25:34 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-05-31 14:32:29 UTC
Created attachment 63867 [details] [review]
Bug 18697: Fix date calculations for weekly frequencies in Serials

Same solution applied as in bug 18356/18607. Consistency++

The code in _get_next_date_week is again very similar to the code in
_get_next_date_month or _get_next_date_year. I will not merge them here,
but we could consider that in the future.

Code in GetFictiveIssueNo has been adjusted similarly to month and year.

Test plan:
[1] Do not apply this patch. Create a subscription for 3/week.
    When the first issue date is on a Saturday or Sunday, the
    intervals in the prediction pattern are 0,0,7,0,0,7,etc.
    Starting on Wed-Fri 1,1,5,etc. Starting on Mon-Tue 2,2,3,etc.
[2] Apply this patch. Check again.
    The interval should be always 2,2,3 now and no longer depend on the
    day_of_week of first issue date.
[3] Check another weekly frequency with multiple units per issue.
    Say 1 issue/3 weeks.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2017-05-31 14:32:33 UTC
Created attachment 63868 [details] [review]
Bug 18697: Adjusting unit tests for weekly serial frequencies

Corrections and added unit tests following the changes of the first patch.

GetFictiveIssueNumber.t: New subtest for weekly frequencies.

GetNextDate.t: Correcting a few dates one day. If we use 2/week, we will
calculate an interval of 3 days and correct with 4 days at the end of
the cycle. The connection with firstacqui is not relevant anymore.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2017-05-31 14:32:37 UTC
Created attachment 63869 [details] [review]
Bug 18697: Fix date calculation for dayly frequencies in Serials

The changes in _get_next_date_day are actually only cosmetic. The sub
now reads exactly the same as its counterparts for other units, but
the results are exactly the same as before.

In GetFictiveIssueNumber we now call _delta_units for each type of unit.
The two Delta_Days calls are moved to _delta_units. Note that this also
is a cosmetic change; results should be exactly the same.

Test plan:
[1] Edit a subscription. Test predication pattern for some daily freq.
[2] Run t/db_dependent/GetNextDate.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-05-31 14:32:40 UTC
Created attachment 63870 [details] [review]
Bug 18697: Adjusting unit tests for dayly serial frequencies

No changes were needed for GetNextDate.t.
In GetFictiveIssueNumber.t we add a subtest for daily frequencies.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-05-31 14:32:44 UTC
Created attachment 63871 [details] [review]
Bug 18697: Final polishing

GetFictiveIssueNumber:
Returns undef instead of 0 for irregular frequencies. Also added to POD.
Removed unused variable $wkno.
Adding a return makes the if(unit) unneeded.
Replaced (a+b)/b by 1+a/b.

_delta_units:
Added a comment about its parameters.

GetFictiveIssueNumber.t:
Adjusted the tests for irregular frequencies accordingly.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2017-05-31 14:47:49 UTC
Note for testers: As you can read in the commit messages, the fix mainly concerns weekly frequencies. The daily frequencies work fine; the code is only changed to make it more consistent with week/month/year.
Comment 9 Josef Moravec 2017-06-06 07:52:26 UTC
Created attachment 64025 [details] [review]
[SIGNED-OFF] Bug 18697: Fix date calculations for weekly frequencies in Serials

Same solution applied as in bug 18356/18607. Consistency++

The code in _get_next_date_week is again very similar to the code in
_get_next_date_month or _get_next_date_year. I will not merge them here,
but we could consider that in the future.

Code in GetFictiveIssueNo has been adjusted similarly to month and year.

Test plan:
[1] Do not apply this patch. Create a subscription for 3/week.
    When the first issue date is on a Saturday or Sunday, the
    intervals in the prediction pattern are 0,0,7,0,0,7,etc.
    Starting on Wed-Fri 1,1,5,etc. Starting on Mon-Tue 2,2,3,etc.
[2] Apply this patch. Check again.
    The interval should be always 2,2,3 now and no longer depend on the
    day_of_week of first issue date.
[3] Check another weekly frequency with multiple units per issue.
    Say 1 issue/3 weeks.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2017-06-06 07:52:29 UTC
Created attachment 64026 [details] [review]
[SIGNED-OFF] Bug 18697: Adjusting unit tests for weekly serial frequencies

Corrections and added unit tests following the changes of the first patch.

GetFictiveIssueNumber.t: New subtest for weekly frequencies.

GetNextDate.t: Correcting a few dates one day. If we use 2/week, we will
calculate an interval of 3 days and correct with 4 days at the end of
the cycle. The connection with firstacqui is not relevant anymore.

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

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:52:33 UTC
Created attachment 64027 [details] [review]
[SIGNED-OFF] Bug 18697: Fix date calculation for dayly frequencies in Serials

The changes in _get_next_date_day are actually only cosmetic. The sub
now reads exactly the same as its counterparts for other units, but
the results are exactly the same as before.

In GetFictiveIssueNumber we now call _delta_units for each type of unit.
The two Delta_Days calls are moved to _delta_units. Note that this also
is a cosmetic change; results should be exactly the same.

Test plan:
[1] Edit a subscription. Test predication pattern for some daily freq.
[2] Run t/db_dependent/GetNextDate.t

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2017-06-06 07:52:36 UTC
Created attachment 64028 [details] [review]
[SIGNED-OFF] Bug 18697: Adjusting unit tests for dayly serial frequencies

No changes were needed for GetNextDate.t.
In GetFictiveIssueNumber.t we add a subtest for daily frequencies.

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

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2017-06-06 07:52:39 UTC
Created attachment 64029 [details] [review]
[SIGNED-OFF] Bug 18697: Final polishing

GetFictiveIssueNumber:
Returns undef instead of 0 for irregular frequencies. Also added to POD.
Removed unused variable $wkno.
Adding a return makes the if(unit) unneeded.
Replaced (a+b)/b by 1+a/b.

_delta_units:
Added a comment about its parameters.

GetFictiveIssueNumber.t:
Adjusted the tests for irregular frequencies accordingly.

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

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Marcel de Rooy 2017-06-06 07:53:42 UTC
Thanks Josef !
Comment 15 Kyle M Hall 2017-06-15 13:23:22 UTC
Created attachment 64317 [details] [review]
Bug 18697: Fix date calculations for weekly frequencies in Serials

Same solution applied as in bug 18356/18607. Consistency++

The code in _get_next_date_week is again very similar to the code in
_get_next_date_month or _get_next_date_year. I will not merge them here,
but we could consider that in the future.

Code in GetFictiveIssueNo has been adjusted similarly to month and year.

Test plan:
[1] Do not apply this patch. Create a subscription for 3/week.
    When the first issue date is on a Saturday or Sunday, the
    intervals in the prediction pattern are 0,0,7,0,0,7,etc.
    Starting on Wed-Fri 1,1,5,etc. Starting on Mon-Tue 2,2,3,etc.
[2] Apply this patch. Check again.
    The interval should be always 2,2,3 now and no longer depend on the
    day_of_week of first issue date.
[3] Check another weekly frequency with multiple units per issue.
    Say 1 issue/3 weeks.

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 16 Kyle M Hall 2017-06-15 13:23:40 UTC
Created attachment 64318 [details] [review]
Bug 18697: Adjusting unit tests for weekly serial frequencies

Corrections and added unit tests following the changes of the first patch.

GetFictiveIssueNumber.t: New subtest for weekly frequencies.

GetNextDate.t: Correcting a few dates one day. If we use 2/week, we will
calculate an interval of 3 days and correct with 4 days at the end of
the cycle. The connection with firstacqui is not relevant anymore.

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

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 17 Kyle M Hall 2017-06-15 13:23:53 UTC
Created attachment 64319 [details] [review]
Bug 18697: Fix date calculation for dayly frequencies in Serials

The changes in _get_next_date_day are actually only cosmetic. The sub
now reads exactly the same as its counterparts for other units, but
the results are exactly the same as before.

In GetFictiveIssueNumber we now call _delta_units for each type of unit.
The two Delta_Days calls are moved to _delta_units. Note that this also
is a cosmetic change; results should be exactly the same.

Test plan:
[1] Edit a subscription. Test predication pattern for some daily freq.
[2] Run t/db_dependent/GetNextDate.t

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 18 Kyle M Hall 2017-06-15 13:24:10 UTC
Created attachment 64320 [details] [review]
Bug 18697: Adjusting unit tests for dayly serial frequencies

No changes were needed for GetNextDate.t.
In GetFictiveIssueNumber.t we add a subtest for daily frequencies.

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

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 19 Kyle M Hall 2017-06-15 13:24:44 UTC
Created attachment 64321 [details] [review]
Bug 18697: Final polishing

GetFictiveIssueNumber:
Returns undef instead of 0 for irregular frequencies. Also added to POD.
Removed unused variable $wkno.
Adding a return makes the if(unit) unneeded.
Replaced (a+b)/b by 1+a/b.

_delta_units:
Added a comment about its parameters.

GetFictiveIssueNumber.t:
Adjusted the tests for irregular frequencies accordingly.

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

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 20 Jonathan Druart 2017-06-19 19:38:29 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 21 Marcel de Rooy 2017-06-20 13:53:47 UTC
Changing severity. If the 18356/607 are backported, it would be logical to pick this one too. Daily frequencies are not affected (in behavior); this report fixes bugs for weekly frequencies.
Comment 22 Fridolin SOMERS 2017-06-29 07:15:16 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 23 Katrin Fischer 2017-06-29 20:37:35 UTC
These patches have been pushed to 16.11.x and will be in 16.11.10.
Comment 24 Mason James 2017-07-31 11:33:20 UTC
Pushed to 16.05.x, for 16.05.15 release