Bug 20728 - Remove subroutines GetLastOrder*
Summary: Remove subroutines GetLastOrder*
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 20726
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-08 16:06 UTC by Jonathan Druart
Modified: 2020-11-30 21:48 UTC (History)
3 users (show)

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


Attachments
Bug 20728: Proof that there is no cheating (2.05 KB, patch)
2019-03-15 22:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent (3.64 KB, patch)
2019-03-15 22:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20728: Remove the 2 GetLastOrder* subroutines (2.43 KB, patch)
2019-03-15 22:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20728: Proof that there is no cheating (2.11 KB, patch)
2020-02-26 15:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent (3.69 KB, patch)
2020-02-26 15:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20728: Remove the 2 GetLastOrder* subroutines (2.49 KB, patch)
2020-02-26 15:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20728: Proof that there is no cheating (2.37 KB, patch)
2020-03-02 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent (4.17 KB, patch)
2020-03-02 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20728: Remove the 2 GetLastOrder* subroutines (2.54 KB, patch)
2020-03-02 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20728: Proof that there is no cheating (2.42 KB, patch)
2020-04-13 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent (4.23 KB, patch)
2020-04-13 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20728: Remove the 2 GetLastOrder* subroutines (2.60 KB, patch)
2020-04-13 17:25 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 Jonathan Druart 2018-05-08 16:06:11 UTC
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.
Comment 1 Jonathan Druart 2019-03-15 22:41:39 UTC
Created attachment 86693 [details] [review]
Bug 20728: Proof that there is no cheating
Comment 2 Jonathan Druart 2019-03-15 22:41:46 UTC
Created attachment 86694 [details] [review]
Bug 20728: Replace the calls by their Koha::Acq::Orders->search equivalent
Comment 3 Jonathan Druart 2019-03-15 22:41:53 UTC
Created attachment 86695 [details] [review]
Bug 20728: Remove the 2 GetLastOrder* subroutines
Comment 4 Martin Renvoize 2020-02-26 15:54:05 UTC
Created attachment 99655 [details] [review]
Bug 20728: Proof that there is no cheating

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-02-26 15:54:07 UTC
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>
Comment 6 Martin Renvoize 2020-02-26 15:54:10 UTC
Created attachment 99657 [details] [review]
Bug 20728: Remove the 2 GetLastOrder* subroutines

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-02-26 15:55:28 UTC
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
Comment 8 Jonathan Druart 2020-02-26 16:09:07 UTC
(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.
Comment 9 Katrin Fischer 2020-02-29 21:14:09 UTC
Please fix the commit messages :(
Comment 10 Jonathan Druart 2020-03-02 09:01:59 UTC
(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.
Comment 11 Katrin Fischer 2020-03-02 10:06:53 UTC
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.
Comment 12 Jonathan Druart 2020-03-02 10:46:02 UTC
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>
Comment 13 Jonathan Druart 2020-03-02 10:46:05 UTC
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>
Comment 14 Jonathan Druart 2020-03-02 10:46:08 UTC
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>
Comment 15 Jonathan Druart 2020-03-02 10:46:41 UTC
I have adjusted the commit messages, hope it's better now.
Comment 16 Katrin Fischer 2020-04-13 17:25:36 UTC
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>
Comment 17 Katrin Fischer 2020-04-13 17:25:39 UTC
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>
Comment 18 Katrin Fischer 2020-04-13 17:25:43 UTC
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>
Comment 19 Martin Renvoize 2020-04-14 16:23:06 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 20 Joy Nelson 2020-05-05 21:22:36 UTC
not backported to 19.11.x