Description
Jonathan Druart
2014-11-13 10:58:20 UTC
Created attachment 33517 [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. Created attachment 33518 [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. Not sure it's consistent. The query are not really readable... s/are/is! Created attachment 33519 [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. 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. (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. (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 :) Created attachment 35582 [details] [review] Bug 13244: Follow-up for unneeded redeclaration Resolves: FAIL acqui/basket.pl OK critic OK forbidden patterns OK pod FAIL valid "my" variable @results masks earlier declaration in same scope Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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 Created attachment 35583 [details] [review] Bug 13244: (follow-up) fix mixup to and from in the sql query (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. Created attachment 35584 [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. Created attachment 35585 [details] [review] Bug 13244: Follow-up for unneeded redeclaration Resolves: FAIL acqui/basket.pl OK critic OK forbidden patterns OK pod FAIL valid "my" variable @results masks earlier declaration in same scope Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 35586 [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> Created attachment 35624 [details] [review] [PASSED QA] 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> Created attachment 35625 [details] [review] [PASSED QA] Bug 13244: Follow-up for unneeded redeclaration Resolves: FAIL acqui/basket.pl OK critic OK forbidden patterns OK pod FAIL valid "my" variable @results masks earlier declaration in same scope Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 35626 [details] [review] [PASSED QA] 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> Unfortunately this one is not cleanly applying after I pushed 12976 and 12970. Please rebase. 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> 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> Rebased, second patch skipped (not used anymore). Patches pushed to master. Thanks Jonathan! |