Bug 9211

Summary: days_between wrong behaviour
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: HolidaysAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Paul Poulain <paul.poulain>
Severity: normal    
Priority: P5 - low CC: chris, kyle, paul.poulain
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 9209    
Bug Blocks:    
Attachments: Bug 9211 - days_between wrong behaviour
[SIGNED-OFF] Bug 9211 - days_between wrong behaviour
Bug 9211 - days_between wrong behaviour - Followup - Add Test to Calendar.t
Bug 9211 follow-up fixing 2 tiny mistakes in unit test
Bug 9211 follow-up fixing 2 tiny mistakes in unit test
Bug 9211 - days_between wrong behaviour - Followup - Add Test to Calendar.t
Bug 9211 follow-up fixing 2 tiny mistakes in unit test

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