Some stuffs has to be fixed on the pending orders page. 1/ There are 2 pagination styles. 2/ All orders are retrieved from the DB but only 20 are displayed! 3/ We cannot search by basket group name.
Created attachment 17691 [details] [review] Bug 10124: Merge GetPendingOrders and SearchOrders routines Preliminary work: In the C4::Acquisition module, 2 routines do the same work. This patch merges these 2 routines. Test plan: test the acqui/orderreceive.pl, acqui/uncertainprice.pl and serials/acqui-search-result.pl scripts. acqui/parcel.pl will be tested with the others patches.
Created attachment 17692 [details] [review] Bug 10124: Add basket group name in filters This patch adds a new filter "basket group name" for pending orders searches. Test plan: Try different filters and check that results are consistent.
Created attachment 17693 [details] [review] Bug 10124: Add filters on the pending orders table Since the parcel.pl script get *all* pending orders, there is no reason not to display all of them. Like that, DataTable manages pagination and sorting correctly (on all data). This patch adds filters on the table header (using columnFilter). Test plan: Try filters on the left of the screen and filters on the table header.
*** Bug 10044 has been marked as a duplicate of this bug. ***
Hi Jonathan, I believe this was a duplicate to 10044, but now I seethis is marked enhancement. Could we extract a fix for the doubled up pagination?
(In reply to comment #5) > Hi Jonathan, I believe this was a duplicate to 10044, but now I seethis is > marked enhancement. Could we extract a fix for the doubled up pagination? Done on bug 10044 :)
Created attachment 17701 [details] [review] Bug 10124: Add filters on the pending orders table Since the parcel.pl script get *all* pending orders, there is no reason not to display all of them. Like that, DataTable manages pagination and sorting correctly (on all data). This patch adds filters on the table header (using columnFilter). Test plan: Try filters on the left of the screen and filters on the table header.
Hello Jonathan. I signed off 10044. For the improvement of filters, I think your patch is a duplicate of a patch I wrote some weeks ago : http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9560 Could you take a look at it please? Mathieu
(In reply to comment #8) > Hello Jonathan. > I signed off 10044. > For the improvement of filters, I think your patch is a duplicate of a patch > I wrote some weeks ago : > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9560 > Could you take a look at it please? > > Mathieu Mathieu, My patch adds a filter for the basket group *name*, not number. So it is not really the same filter. I think it is more useful to search by name instead of number. Moreover, my patches add filter on the table itself too.
I think searching by basketgroup number could be a good option too. For example, it is needed in Rennes 2, because we classify printed basketgroups using there number, and we use this number in our relations with vendors ;-) But I am ok with your patch. I could rewrite mine later, only to add a filter by bg number, and we will see if the community agrees. Mathieu
Your patch is better and larger, so I put mine "in discussion". I will see if it deserves to be reopened after 10124 is pushed. Mathieu
Sandbox setup by tredok.pierre@gmail.com with database 2 and bug 10124 on Thu May 30 08:42:02 2013 Something went wrong ! Applying: Bug 10124: Merge GetPendingOrders and SearchOrders routines Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Acquisition.pm Auto-merging acqui/orderreceive.pl Auto-merging t/db_dependent/Acquisition.t Applying: Bug 10124: Add basket group name in filters Applying: Bug 10124: Add filters on the pending orders table Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 10124: Add filters on the pending orders table When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Bug 10124 - Pending orders: ergonomic improvement Bug 10124: Merge GetPendingOrders and SearchOrders routines Apply? [yn] Bug 10124: Add basket group name in filters Apply? [yn] Bug 10124: Add filters on the pending orders table Apply? [yn] Patch left in /tmp/Bug-10124-Add-filters-on-the-pending-orders-table-GhyXZs.patch
Created attachment 18493 [details] [review] Bug 10124: Merge GetPendingOrders and SearchOrders routines Preliminary work: In the C4::Acquisition module, 2 routines do the same work. This patch merges these 2 routines. Test plan: test the acqui/orderreceive.pl, acqui/uncertainprice.pl and serials/acqui-search-result.pl scripts. acqui/parcel.pl will be tested with the others patches.
Created attachment 18494 [details] [review] Bug 10124: Add basket group name in filters This patch adds a new filter "basket group name" for pending orders searches. Test plan: Try different filters and check that results are consistent.
Created attachment 18495 [details] [review] Bug 10124: Add filters on the pending orders table Since the parcel.pl script get *all* pending orders, there is no reason not to display all of them. Like that, DataTable manages pagination and sorting correctly (on all data). This patch adds filters on the table header (using columnFilter). Test plan: Try filters on the left of the screen and filters on the table header.
Created attachment 18501 [details] [review] Bug 10124: Merge GetPendingOrders and SearchOrders routines Preliminary work: In the C4::Acquisition module, 2 routines do the same work. This patch merges these 2 routines. Test plan: test the acqui/orderreceive.pl, acqui/uncertainprice.pl and serials/acqui-search-result.pl scripts. acqui/parcel.pl will be tested with the others patches. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 18502 [details] [review] Bug 10124: Add basket group name in filters This patch adds a new filter "basket group name" for pending orders searches. Test plan: Try different filters and check that results are consistent. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 18503 [details] [review] Bug 10124: Add filters on the pending orders table Since the parcel.pl script get *all* pending orders, there is no reason not to display all of them. Like that, DataTable manages pagination and sorting correctly (on all data). This patch adds filters on the table header (using columnFilter). Test plan: Try filters on the left of the screen and filters on the table header. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Test on sandbox. Signed off. I've created 2 baskets, and add them to a basketgroup. Then, I've made a receive to test filters. - Authors, ... : ok - Basket : ok - Basket group name : ok - Order line : ok I've also tested column filter. Everything is fine.
This fails the QA script in 2 places, can you please take a look? FAIL C4/Acquisition.pm OK pod FAIL forbidden patterns forbidden pattern: IndependantBranches is now known as IndependentBranches (line 1553) FAIL t/db_dependent/lib/KohaTest/Acquisition/GetPendingOrders.pm OK pod OK forbidden patterns OK valid FAIL critic # Modules::RequireFilenameMatchesPackage: Got 1 violation(s).
Created attachment 19309 [details] [review] Bug 10124: QA followup - Remame the t/.../GetPendingOrders.pm to t/.../SearchOrders.pm according to the package name. - Rename IndependantBranches with IndependentBranches - FIX levels in pod Note that 1 qa issue still exists (not introduced by this development): FAIL t/db_dependent/lib/KohaTest/Acquisition/SearchOrders.pm OK pod OK forbidden patterns FAIL valid Invalid CODE attribute: Test( 1 ) OK critic These tests are never used.
Patches no longer apply cleanly for a conflict in Acquisitions.pm. Jonathan, could you double check those lines here, rebase and switch back to signed off? - &NewOrder &DelOrder &ModOrder &GetPendingOrders &GetOrder &GetOrders &GetOrdersByBiblionumber + &NewOrder &DelOrder &ModOrder &GetOrder &GetOrders &GetOrdersByBiblionumber + &NewOrder &DelOrder &ModOrder &GetOrder &GetOrders
Created attachment 19636 [details] [review] Bug 10124: Merge GetPendingOrders and SearchOrders routines Preliminary work: In the C4::Acquisition module, 2 routines do the same work. This patch merges these 2 routines. Test plan: test the acqui/orderreceive.pl, acqui/uncertainprice.pl and serials/acqui-search-result.pl scripts. acqui/parcel.pl will be tested with the others patches. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 19637 [details] [review] Bug 10124: Add basket group name in filters This patch adds a new filter "basket group name" for pending orders searches. Test plan: Try different filters and check that results are consistent. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 19638 [details] [review] Bug 10124: Add filters on the pending orders table Since the parcel.pl script get *all* pending orders, there is no reason not to display all of them. Like that, DataTable manages pagination and sorting correctly (on all data). This patch adds filters on the table header (using columnFilter). Test plan: Try filters on the left of the screen and filters on the table header. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 19639 [details] [review] Bug 10124: QA followup - Remame the t/.../GetPendingOrders.pm to t/.../SearchOrders.pm according to the package name. - Rename IndependantBranches with IndependentBranches - FIX levels in pod Note that 1 qa issue still exists (not introduced by this development): FAIL t/db_dependent/lib/KohaTest/Acquisition/SearchOrders.pm OK pod OK forbidden patterns FAIL valid Invalid CODE attribute: Test( 1 ) OK critic These tests are never used.
Created attachment 19640 [details] [review] Bug 10124: Fix a js error : YAHOO is not defined json.min.js is not used in this page. The js include is useless.
(In reply to Katrin Fischer from comment #22) > Patches no longer apply cleanly for a conflict in Acquisitions.pm. > Jonathan, could you double check those lines here, rebase and switch back to > signed off? > > - &NewOrder &DelOrder &ModOrder &GetPendingOrders &GetOrder > &GetOrders &GetOrdersByBiblionumber > + &NewOrder &DelOrder &ModOrder &GetOrder &GetOrders > &GetOrdersByBiblionumber > + &NewOrder &DelOrder &ModOrder &GetOrder &GetOrders Rebased. I added a little followup, I find a js error on reloading the page. Back to Signed Off.
I have spent some time looking at those patches and I am worried that they try to do too many things. I think i have identified what they do, but I am not sure (which is not a good sign :) ). I think we have 3 things here, that ideally should be broken up into 3 different bugs to make this easier testable: - Merging GetPendingOrders and GetOrders + regression tests - Adding basket group name search - Adding datatables to the pending oders page Jonathan, do you think you could break it up on different bugs with test plans for each?
(In reply to Katrin Fischer from comment #29) > I have spent some time looking at those patches and I am worried that they > try to do too many things. I think i have identified what they do, but I am > not sure (which is not a good sign :) ). I think we have 3 things here, that > ideally should be broken up into 3 different bugs to make this easier > testable: > > - Merging GetPendingOrders and GetOrders + regression tests > - Adding basket group name search > - Adding datatables to the pending oders page > > Jonathan, do you think you could break it up on different bugs with test > plans for each? The 3 patchs can be tested together so I don't think it is relevant, but I will do that.
Please see bug 10723, bug 10724 and bug 10725.