There are 2 subroutines from C4::Acquisition that could be removed: - GetLastOrderReceivedFromSubscriptionid - GetLastOrderNotReceivedFromSubscriptionid After bug 20726 only GetLastOrderReceivedFromSubscriptionid will be used (from acqui/neworderempty.pl) and this call could be replaced easily with Koha::Acquisition::Orders The code (+ tests) related to these 2 subroutines could then be removed.
Created attachment 86693 [details] [review] Bug 20728: Proof that there is no cheating
Created attachment 86694 [details] [review] Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent
Created attachment 86695 [details] [review] Bug 20728: Remove the 2 GetLastOrder* subroutines
Created attachment 99655 [details] [review] Bug 20728: Proof that there is no cheating Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99656 [details] [review] Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99657 [details] [review] Bug 20728: Remove the 2 GetLastOrder* subroutines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected without regressions. I'm not entirely sure I like that we're moving from a tested method to an untested controller, but I'm struggling to come up with a better alternative. Signing off
(In reply to Martin Renvoize from comment #7) > Works as expected without regressions. > > I'm not entirely sure I like that we're moving from a tested method to an > untested controller, but I'm struggling to come up with a better alternative. > > Signing off 1 join? :) I agree it can be discussed. But the parameters for the search is basic, IMO it does not deserve its own subroutine.
Please fix the commit messages :(
(In reply to Katrin Fischer from comment #9) > Please fix the commit messages :( What do you mean? All of them explain what they are doing. With comment 0 and the 3 commit titles, everything is meaningful to me. I can explain a bit more if you want me to clarify something.
From 0445ff47aea8cbe01e5fbb212aebe4885b65deb3 Mon Sep 17 00:00:00 2001 From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Date: Fri, 15 Mar 2019 19:24:19 -0300 Subject: [PATCH] Bug 20728: Proof that there is no cheating Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> The don't have descriptions or test plans. And I already failed to understand the first one from the subject. comment #0 shoudl be in one of the commit messages.
Created attachment 99877 [details] [review] Bug 20728: Proof that there is no cheating To make sure the replacing code will acchieve the same things as the actual one, we replace the raw SQL query with the DBIC version of it. Then the tests will show us that they are equivalent. Test plan: Apply only this patch, run the tests, confirm they pass. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99878 [details] [review] Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent There are 2 subroutines from C4::Acquisition that could be removed: - GetLastOrderReceivedFromSubscriptionid - GetLastOrderNotReceivedFromSubscriptionid After bug 20726 only GetLastOrderReceivedFromSubscriptionid will be used (from acqui/neworderempty.pl) and this call could be replaced easily with Koha::Acquisition::Orders The code (+ tests) related to these 2 subroutines could then be removed. The parameters for the search is basic and does no really deserve its own subroutine. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99879 [details] [review] Bug 20728: Remove the 2 GetLastOrder* subroutines At this point the 2 subroutines are no longer in used. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I have adjusted the commit messages, hope it's better now.
Created attachment 102873 [details] [review] Bug 20728: Proof that there is no cheating To make sure the replacing code will acchieve the same things as the actual one, we replace the raw SQL query with the DBIC version of it. Then the tests will show us that they are equivalent. Test plan: Apply only this patch, run the tests, confirm they pass. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 102874 [details] [review] Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent There are 2 subroutines from C4::Acquisition that could be removed: - GetLastOrderReceivedFromSubscriptionid - GetLastOrderNotReceivedFromSubscriptionid After bug 20726 only GetLastOrderReceivedFromSubscriptionid will be used (from acqui/neworderempty.pl) and this call could be replaced easily with Koha::Acquisition::Orders The code (+ tests) related to these 2 subroutines could then be removed. The parameters for the search is basic and does no really deserve its own subroutine. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 102875 [details] [review] Bug 20728: Remove the 2 GetLastOrder* subroutines At this point the 2 subroutines are no longer in used. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Nice work everyone! Pushed to master for 20.05
not backported to 19.11.x