http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |kenza.zaki@biblibre.com Depends on| |10528 Assignee|koha-bugs@lists.koha-commun |kenza.zaki@biblibre.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |srdjan@catalyst.net.nz --- Comment #2 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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 {} -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 --- Comment #4 from kenza <kenza.zaki@biblibre.com> --- 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.

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #9 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 --- Comment #11 from kenza <kenza.zaki@biblibre.com> --- Created attachment 20628 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20628&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19984|0 |1 is obsolete| | --- Comment #12 from kenza <kenza.zaki@biblibre.com> --- Created attachment 20629 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20629&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10641 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20629|0 |1 is obsolete| | --- Comment #13 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20662 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20662&action=edit [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. -- You are receiving this mail because: You are watching all bug changes.