Bug 13244 - GetCancelledOrders and GetOrders can be merged
Summary: GetCancelledOrders and GetOrders can be merged
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-13 10:58 UTC by Jonathan Druart
Modified: 2015-12-03 22:11 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13244: Merge GetOrders and GetCancelledOrders (7.61 KB, patch)
2014-11-13 10:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13244: Merge GetOrders and GetCancelledOrders (7.61 KB, patch)
2014-11-13 11:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13244: Merge GetOrders and GetCancelledOrders (7.82 KB, patch)
2014-11-13 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13244: Follow-up for unneeded redeclaration (1.07 KB, patch)
2015-01-28 15:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13244: (follow-up) fix mixup to and from in the sql query (781 bytes, patch)
2015-01-28 15:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13244: Merge GetOrders and GetCancelledOrders (7.92 KB, patch)
2015-01-28 15:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13244: Follow-up for unneeded redeclaration (1.07 KB, patch)
2015-01-28 15:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13244: (follow-up) fix mixup to and from in the sql query (887 bytes, patch)
2015-01-28 15:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 13244: Merge GetOrders and GetCancelledOrders (7.95 KB, patch)
2015-01-30 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13244: Follow-up for unneeded redeclaration (1.10 KB, patch)
2015-01-30 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13244: (follow-up) fix mixup to and from in the sql query (914 bytes, patch)
2015-01-30 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13244: Merge GetOrders and GetCancelledOrders (8.03 KB, patch)
2015-02-19 13:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13244: (follow-up) fix mixup to and from in the sql query (894 bytes, patch)
2015-02-19 13:37 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!