Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences
Summary: GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: kenza
QA Contact: Testopia
URL:
Keywords:
Depends on: 10528
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-25 08:22 UTC by kenza
Modified: 2014-12-07 20:02 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences (9.47 KB, patch)
2013-07-25 09:13 UTC, kenza
Details | Diff | Splinter Review
Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences (9.38 KB, patch)
2013-07-29 07:28 UTC, kenza
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences (9.43 KB, patch)
2013-07-30 04:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences (10.02 KB, text/plain)
2013-08-26 14:54 UTC, kenza
Details
Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences (10.02 KB, patch)
2013-08-26 14:58 UTC, kenza
Details | Diff | Splinter Review
[PASSED QA] Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences (10.11 KB, patch)
2013-08-27 19:55 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description kenza 2013-07-25 08:22:33 UTC

    
Comment 1 kenza 2013-07-25 09:13:47 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-07-29 03:27:05 UTC
There are some things that would be nice to address:

1. if ( defined $delay && $delay >= 0 ), elsif ( defined $delay && $delay < 0 )
is too much,
if ( $delay >= 0 ), elsif ( $delay < 0 )
should do

2. Around line 137 - closing { for elsif ( defined $delay && $delay < 0 ) - identation becomes broken (probably tabs need replacing with blank spaces)

3. if ( defined $estimateddeliverydateto ) {} elsif ( !defined $estimateddeliverydateto ) {}
should probably be
if ( defined $estimateddeliverydateto ) {} else {}
Comment 3 kenza 2013-07-29 07:28:30 UTC Comment hidden (obsolete)
Comment 4 kenza 2013-07-29 07:34:03 UTC
I have modified the patch in accordance with your advices.
"if ( defined $delay && $delay >= 0 )" is usefull, if we use only "if ( $delay >= 0 )" it breaks the tests. More,"elsif ( $delay < 0 )" adds some warnings.
Comment 5 Srdjan Jankovic 2013-07-29 13:40:09 UTC
Sure, sorry, I was doing two things at the time, never a good idea. I wan ted to say to have a if (defined $delay) wrapper, just a bit easier to read, but that's ok.
Comment 6 Srdjan Jankovic 2013-07-30 04:18:57 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2013-08-26 05:22:58 UTC
Hi Kenza,

I think this would be a bit better as 2 patches - one for the changes Booksellers.pm and one for the tests, so you can compare the results of the tests before and after the change. Could you split the patch up?
Comment 8 Katrin Fischer 2013-08-26 05:23:23 UTC
Failed QA always gets a bit more attention :)
Comment 9 Jonathan Druart 2013-08-26 07:29:36 UTC
(In reply to Katrin Fischer from comment #7)
> Hi Kenza,
> 
> I think this would be a bit better as 2 patches - one for the changes
> Booksellers.pm and one for the tests, so you can compare the results of the
> tests before and after the change. Could you split the patch up?

Katrin,
Kenza already adds a report for testing this routine (bug 10528). In this patch, a lot of lines are uncommented in order to remove previous FIXME. I am not sure to understand the goal of your request.

However I think this patch needs a better test plan: the feature should be tested in the interface.
Comment 10 Katrin Fischer 2013-08-26 07:31:25 UTC
Jonathan, I think you are right - no tired QAing...
Comment 11 kenza 2013-08-26 14:54:56 UTC Comment hidden (obsolete)
Comment 12 kenza 2013-08-26 14:58:18 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2013-08-27 19:55:43 UTC
Created attachment 20662 [details] [review]
[PASSED QA] Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences

This patch fixes some incoherences of the routine GetBooksellerWithOrders.
Now it considers the field $estimateddeliverydateto and it replaces it by now() only if it is undef.
More it doesn't test if $aqbookseller.deliverytime is not Null anymore but if $deliverytime = null or undef, it replaces it by 0.
It also verifies if $delay is >= 0 and return undef if it is a negative value.

To Test:
Before, this routine sorts out the BookSellerWithLateOrders. If a Bookseller did not specify a deliverytime, he would never appears in the list of LateOrders.More if the field "Estimated delivery date to" was specified, it didn't take care of the value and it returns the late order up to today's date.
Now, the returned list considers all the fields give and if the deliverytime of the Bookseller is not specified, it calculates the late orders as if the deliverytime is 0.By default , all booksellers which have orders in late until today are listed unless "estimated delivery date to" is specified.

prove t/db_dependent/Bookseller.t
t/db_dependent/Bookseller.t ..
[Some warnings about uninitialized values]
WARNING: GetBooksellerWithLateOrders is called with a negative value at C4/Bookseller.pm line 135.
t/db_dependent/Bookseller.t .. ok
All tests successful.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests and QA script pass.
Comment 14 Galen Charlton 2013-10-09 04:07:28 UTC
Pushed to master.  Thanks, Kenza!