Bug 9211 - days_between wrong behaviour
Summary: days_between wrong behaviour
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Holidays (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Paul Poulain
URL:
Keywords:
Depends on: 9209
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-04 18:25 UTC by Tomás Cohen Arazi
Modified: 2014-05-26 21:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9211 - days_between wrong behaviour (1.19 KB, patch)
2012-12-04 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9211 - days_between wrong behaviour (1.31 KB, patch)
2012-12-07 17:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9211 - days_between wrong behaviour - Followup - Add Test to Calendar.t (838 bytes, patch)
2012-12-07 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9211 follow-up fixing 2 tiny mistakes in unit test (1.21 KB, patch)
2013-01-03 16:53 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 9211 follow-up fixing 2 tiny mistakes in unit test (1.21 KB, patch)
2013-01-03 16:54 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 9211 - days_between wrong behaviour - Followup - Add Test to Calendar.t (878 bytes, patch)
2013-01-03 16:54 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 9211 follow-up fixing 2 tiny mistakes in unit test (1.21 KB, patch)
2013-01-03 16:54 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2012-12-04 18:25:06 UTC
As noted in comments #15, #16 and #17 of bug 8486, and its API, Koha::Calendar->days_between should always returns a positive number irrespective of the relative order of the parameters.
This is still an open bug, which arised when rewriting the Calendar.t file.

Already have a patch for it. Attaching in a minute.
Comment 1 Tomás Cohen Arazi 2012-12-04 18:36:32 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-12-07 17:09:04 UTC
Created attachment 13945 [details] [review]
[SIGNED-OFF] Bug 9211 - days_between wrong behaviour

As noted in comments #15, #16 and #17 of bug 8486, and its API, Koha::Calendar->days_between
should always returns a positive number irrespective of the relative order of the parameters.
This is still an open bug, which arised when rewriting the Calendar.t file (Bug 9209).

Regards
To+

Sponsored-by: Universidad Nacional de Córdoba
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

http://bugs.koha-community.org/show_bug.cgi?id=9221
Comment 3 Kyle M Hall 2012-12-07 17:10:25 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2012-12-07 19:19:23 UTC
Kyle, I've written that test in the context of bug 9209.
Comment 5 Paul Poulain 2013-01-03 16:52:57 UTC
QA Comment:
  * checking usage of days_between =
    - in C4/Circulation, it's used in _debar_user_on_return. The relevant lines are:
    my $deltadays = $calendar->days_between( $dt_due, $dt_today );
        if ( $deltadays->subtract($grace)->is_positive() ) {
  => seems logical that $deltadays is positive

    - in C4/Overdues.pm, it's used in _get_chargeable_units
            $charge_duration = $calendar->days_between( $dt1, $dt2 );
  => seems OK as well

  * patch passes koha-qa.pl
  * prove t/Calendar.t is working well after a tiny followup i'll attach

TO RM = I don't see anything wrong with this patch, but it's a sensible one, so it's worth looking deeply !!!
Comment 6 Paul Poulain 2013-01-03 16:53:15 UTC Comment hidden (obsolete)
Comment 7 Paul Poulain 2013-01-03 16:54:05 UTC Comment hidden (obsolete)
Comment 8 Paul Poulain 2013-01-03 16:54:44 UTC Comment hidden (obsolete)
Comment 9 Paul Poulain 2013-01-03 16:54:56 UTC Comment hidden (obsolete)
Comment 10 Paul Poulain 2013-01-03 16:55:34 UTC
(sorry for multiple attachment, made a mistake...)
Comment 11 Tomás Cohen Arazi 2013-01-04 03:33:10 UTC
(In reply to comment #10)
> (sorry for multiple attachment, made a mistake...)

I recall there were tests for this in 9109 already (pushed). I added 3, on a per syspref setup basis, To capture the syspref independent nature. I might be erong too, on my phone right now
Comment 12 Jared Camins-Esakov 2013-01-04 13:07:55 UTC
Could we get a test plan so I can test this in the interface and not just in the unit test?
Comment 13 Tomás Cohen Arazi 2013-01-24 14:31:34 UTC
(In reply to comment #12)
> Could we get a test plan so I can test this in the interface and not just in
> the unit test?

I haven't found through the code an usage example that leaded to having the dates swapped.  I assume the days_between method is intended to work whether the dates are ordered or not just in case it is needed.

In that case, this bug just makes the implementation match the documented API.
Comment 14 Jared Camins-Esakov 2013-01-24 14:57:54 UTC
This patch has been pushed to master.
Comment 15 Chris Cormack 2013-02-01 08:44:05 UTC
Pushed to 3.10.x and 3.8.x will be in 3.10.3 and 3.8.10