| Summary: | days_between wrong behaviour | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Component: | Holidays | Assignee: | Tomás Cohen Arazi (tcohen) <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: | ||
| Circulation function: | |||
| 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 (tcohen)
2012-12-04 18:25:06 UTC
Created attachment 13886 [details] [review] 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 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 Created attachment 13946 [details] [review] Bug 9211 - days_between wrong behaviour - Followup - Add Test to Calendar.t Kyle, I've written that test in the context of bug 9209. 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 !!!
Created attachment 14400 [details] [review] Bug 9211 follow-up fixing 2 tiny mistakes in unit test * the new test was not announced (26 announced, 27 run) * $daycount already declared, resulting in "my" variable $daycount masks earlier declaration in same scope at t/Calendar.t line 93. with this patch prove t/Calendar.t is green ;-) Created attachment 14401 [details] [review] Bug 9211 follow-up fixing 2 tiny mistakes in unit test * the new test was not announced (26 announced, 27 run) * $daycount already declared, resulting in "my" variable $daycount masks earlier declaration in same scope at t/Calendar.t line 93. with this patch prove t/Calendar.t is green ;-) Created attachment 14402 [details] [review] Bug 9211 - days_between wrong behaviour - Followup - Add Test to Calendar.t Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 14403 [details] [review] Bug 9211 follow-up fixing 2 tiny mistakes in unit test * the new test was not announced (26 announced, 27 run) * $daycount already declared, resulting in "my" variable $daycount masks earlier declaration in same scope at t/Calendar.t line 93. with this patch prove t/Calendar.t is green ;-) (sorry for multiple attachment, made a mistake...) (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 Could we get a test plan so I can test this in the interface and not just in the unit test? (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. This patch has been pushed to master. Pushed to 3.10.x and 3.8.x will be in 3.10.3 and 3.8.10 |