Bug 13244

Summary: GetCancelledOrders and GetOrders can be merged
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: m.de.rooy, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 13244: Merge GetOrders and GetCancelledOrders
Bug 13244: Merge GetOrders and GetCancelledOrders
Bug 13244: Merge GetOrders and GetCancelledOrders
Bug 13244: Follow-up for unneeded redeclaration
Bug 13244: (follow-up) fix mixup to and from in the sql query
Bug 13244: Merge GetOrders and GetCancelledOrders
Bug 13244: Follow-up for unneeded redeclaration
Bug 13244: (follow-up) fix mixup to and from in the sql query
[PASSED QA] Bug 13244: Merge GetOrders and GetCancelledOrders
[PASSED QA] Bug 13244: Follow-up for unneeded redeclaration
[PASSED QA] Bug 13244: (follow-up) fix mixup to and from in the sql query
Bug 13244: Merge GetOrders and GetCancelledOrders
Bug 13244: (follow-up) fix mixup to and from in the sql query

Description Jonathan Druart 2014-11-13 10:58:20 UTC
They do the same job.
Comment 1 Jonathan Druart 2014-11-13 10:59:44 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-11-13 11:04:15 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-11-13 11:04:36 UTC
Not sure it's consistent.
The query are not really readable...
Comment 4 Jonathan Druart 2014-11-13 11:04:50 UTC
s/are/is!
Comment 5 Jonathan Druart 2014-11-13 11:05:35 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2015-01-28 13:03:36 UTC
Running the test (adjust tests to 87):

not ok 14 - GetOrder gets an order with the right number of fields
#   Failed test 'GetOrder gets an order with the right number of fields'
#   at t/db_dependent/Acquisition.t line 392.
#          got: '60'
#     expected: '59'

not ok 16 - GetOrder gets an order with no unexpected fields
#   Failed test 'GetOrder gets an order with no unexpected fields'
#   at t/db_dependent/Acquisition.t line 399.
#          got: 'cancelledby'
#     expected: ''

etc.

# Looks like you failed 8 tests of 87.

Please verify if you have the same .. Parking in FQA now.
Comment 7 Jonathan Druart 2015-01-28 13:50:55 UTC
(In reply to M. de Rooy from comment #6)
> Running the test (adjust tests to 87):
> 
> not ok 14 - GetOrder gets an order with the right number of fields
> #   Failed test 'GetOrder gets an order with the right number of fields'
> #   at t/db_dependent/Acquisition.t line 392.
> #          got: '60'
> #     expected: '59'
> 
> not ok 16 - GetOrder gets an order with no unexpected fields
> #   Failed test 'GetOrder gets an order with no unexpected fields'
> #   at t/db_dependent/Acquisition.t line 399.
> #          got: 'cancelledby'
> #     expected: ''
> 
> etc.
> 
> # Looks like you failed 8 tests of 87.
> 
> Please verify if you have the same .. Parking in FQA now.

Does they pass on master?
Maybe you have an extra DB field in your aqorders table (check the var $test_extra_fields l.392).
The tests pass here.
Comment 8 Marcel de Rooy 2015-01-28 13:56:56 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to M. de Rooy from comment #6)
> > Running the test (adjust tests to 87):
> > 
> > not ok 14 - GetOrder gets an order with the right number of fields
> > #   Failed test 'GetOrder gets an order with the right number of fields'
> > #   at t/db_dependent/Acquisition.t line 392.
> > #          got: '60'
> > #     expected: '59'
> > 
> > not ok 16 - GetOrder gets an order with no unexpected fields
> > #   Failed test 'GetOrder gets an order with no unexpected fields'
> > #   at t/db_dependent/Acquisition.t line 399.
> > #          got: 'cancelledby'
> > #     expected: ''
> > 
> > etc.
> > 
> > # Looks like you failed 8 tests of 87.
> > 
> > Please verify if you have the same .. Parking in FQA now.
> 
> Does they pass on master?
> Maybe you have an extra DB field in your aqorders table (check the var
> $test_extra_fields l.392).
> The tests pass here.

Sorry, it does not pass on master too.
Will have a look what fields were added when testing something else :)
Comment 9 Marcel de Rooy 2015-01-28 15:28:58 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2015-01-28 15:31:09 UTC
OK. The test passes now.
Overall, it looks good. One small question:
This does not look good?

    $query .= $cancelled
      ? q|
                aqorders_transfers.ordernumber_from AS transferred_to,
                aqorders_transfers.timestamp AS transferred_to_timestamp
    |
      : q|
                aqorders_transfers.ordernumber_from AS transferred_from,
                aqorders_transfers.timestamp AS transferred_from_timestamp
    |;

Did you mixup to and from in the first line?
Please compare with the lines you removed:

-            aqorders_transfers.ordernumber_to AS transferred_to,
-            aqorders_transfers.timestamp AS transferred_to_timestamp

Parking in FQA
Comment 11 Jonathan Druart 2015-01-28 15:38:09 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-01-28 15:39:12 UTC
(In reply to M. de Rooy from comment #10)
> OK. The test passes now.
> Overall, it looks good. One small question:
> This does not look good?
> 
>     $query .= $cancelled
>       ? q|
>                 aqorders_transfers.ordernumber_from AS transferred_to,
>                 aqorders_transfers.timestamp AS transferred_to_timestamp
>     |
>       : q|
>                 aqorders_transfers.ordernumber_from AS transferred_from,
>                 aqorders_transfers.timestamp AS transferred_from_timestamp
>     |;
> 
> Did you mixup to and from in the first line?

Erk, yes I did!
Thanks for catching that Marcel.
Comment 13 Marcel de Rooy 2015-01-28 15:55:06 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2015-01-28 15:55:11 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2015-01-28 15:55:15 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-01-30 13:40:47 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2015-01-30 13:41:13 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2015-01-30 13:41:16 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2015-02-19 13:19:51 UTC
Unfortunately this one is not cleanly applying after I pushed 12976 and 12970. Please rebase.
Comment 20 Jonathan Druart 2015-02-19 13:37:41 UTC
Created attachment 36034 [details] [review]
Bug 13244: Merge GetOrders and GetCancelledOrders

These two subroutines did the same job (same select, same join, etc.)

Test plan:
Go on the basket list page and verify you see the pending and the
cancelled baskets.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Two small things are adjusted in separate follow-ups.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Jonathan Druart 2015-02-19 13:37:46 UTC
Created attachment 36035 [details] [review]
Bug 13244: (follow-up) fix mixup to and from in the sql query

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Jonathan Druart 2015-02-19 13:38:30 UTC
Rebased, second patch skipped (not used anymore).
Comment 23 Tomás Cohen Arazi 2015-04-13 14:22:41 UTC
Patches pushed to master.

Thanks Jonathan!