Bug 10124 - Pending orders: ergonomic improvement
Summary: Pending orders: ergonomic improvement
Status: CLOSED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 10044 10723 10724 10725
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-26 07:20 UTC by Jonathan Druart
Modified: 2014-11-29 09:10 UTC (History)
3 users (show)

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


Attachments
Bug 10124: Merge GetPendingOrders and SearchOrders routines (17.27 KB, patch)
2013-04-26 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add basket group name in filters (6.35 KB, patch)
2013-04-26 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add filters on the pending orders table (8.30 KB, patch)
2013-04-26 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add filters on the pending orders table (5.74 KB, patch)
2013-04-26 13:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Merge GetPendingOrders and SearchOrders routines (17.38 KB, patch)
2013-05-30 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add basket group name in filters (6.35 KB, patch)
2013-05-30 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add filters on the pending orders table (5.74 KB, patch)
2013-05-30 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Merge GetPendingOrders and SearchOrders routines (17.43 KB, patch)
2013-05-30 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add basket group name in filters (6.40 KB, patch)
2013-05-30 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add filters on the pending orders table (5.80 KB, patch)
2013-05-30 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: QA followup (2.39 KB, patch)
2013-07-01 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Merge GetPendingOrders and SearchOrders routines (17.11 KB, patch)
2013-07-15 07:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add basket group name in filters (6.39 KB, patch)
2013-07-15 07:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Add filters on the pending orders table (5.46 KB, patch)
2013-07-15 07:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: QA followup (2.39 KB, patch)
2013-07-15 07:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10124: Fix a js error : YAHOO is not defined (982 bytes, patch)
2013-07-15 07:55 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 2013-04-26 07:20:56 UTC
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.
Comment 1 Jonathan Druart 2013-04-26 07:38:53 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-04-26 07:38:55 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-04-26 07:38:57 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2013-04-26 11:48:21 UTC
*** Bug 10044 has been marked as a duplicate of this bug. ***
Comment 5 Katrin Fischer 2013-04-26 11:49:59 UTC
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?
Comment 6 Jonathan Druart 2013-04-26 12:02:13 UTC
(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 :)
Comment 7 Jonathan Druart 2013-04-26 13:15:52 UTC Comment hidden (obsolete)
Comment 8 Mathieu Saby 2013-05-01 11:02:17 UTC
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
Comment 9 Jonathan Druart 2013-05-02 08:30:38 UTC
(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.
Comment 10 Mathieu Saby 2013-05-02 08:35:16 UTC
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
Comment 11 Mathieu Saby 2013-05-23 10:14:20 UTC
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
Comment 12 Pierre Angot 2013-05-30 07:09:31 UTC
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
Comment 13 Jonathan Druart 2013-05-30 07:39:11 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2013-05-30 07:39:15 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-05-30 07:39:19 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-05-30 09:00:14 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-05-30 09:00:19 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2013-05-30 09:00:24 UTC Comment hidden (obsolete)
Comment 19 Pierre Angot 2013-05-30 09:08:26 UTC
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.
Comment 20 Katrin Fischer 2013-06-30 20:23:59 UTC
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).
Comment 21 Jonathan Druart 2013-07-01 09:50:44 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2013-07-14 16:20:34 UTC
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
Comment 23 Jonathan Druart 2013-07-15 07:55:35 UTC
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>
Comment 24 Jonathan Druart 2013-07-15 07:55:42 UTC
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>
Comment 25 Jonathan Druart 2013-07-15 07:55:46 UTC
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>
Comment 26 Jonathan Druart 2013-07-15 07:55:52 UTC
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.
Comment 27 Jonathan Druart 2013-07-15 07:55:58 UTC
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.
Comment 28 Jonathan Druart 2013-07-15 07:57:11 UTC
(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.
Comment 29 Katrin Fischer 2013-08-04 13:18:17 UTC
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?
Comment 30 Jonathan Druart 2013-08-14 08:51:52 UTC
(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.
Comment 31 Jonathan Druart 2013-08-14 09:02:54 UTC
Please see bug 10723, bug 10724 and bug 10725.