Created attachment 19942 [details] [review] 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: 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.
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 {}
Created attachment 19975 [details] [review] 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: 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.
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.
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.
Created attachment 19984 [details] [review] [SIGNED-OFF] 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: 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>
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?
Failed QA always gets a bit more attention :)
(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.
Jonathan, I think you are right - no tired QAing...
Created attachment 20628 [details] 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>
Created attachment 20629 [details] [review] 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>
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.
Pushed to master. Thanks, Kenza!