Bug 5345

Summary: DataTables in Koha
Product: Koha Reporter: Paul Poulain <paul.poulain>
Component: TemplatesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: alexandre.lengereau, claire.hernandez, francois.charbonnier, jonathan.druart, katrin.fischer, m.de.rooy, sophie.meynieux, srdjan
Version: Main   
Hardware: All   
OS: All   
URL: http://wiki.koha-community.org/wiki/Table_sorters_RFC
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8395
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 6836, 8098, 8099, 8100, 8101, 8102, 8103, 8104, 8105, 8106, 8109, 8234    
Bug Blocks: 7163, 7647    
Attachments: parcel.tt
acqui-home.tt
addorderiso2709.tt
basket.tt
basketgroup.tt
histsearch.tt
lateorders.tt
neworderbiblio.tt
newordersuggestion.tt
ordered.tt
orderreceive.tt
parcels.tt
spent.tt
supplier.tt
uncertainprice.tt
z3950_search.tt
Bug 5345 [FOLLOW-UP] DataTables in Koha
DataTables missing some required images
[PASSED_QA][SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [16]
[PASSED_QA][SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [9]
parcels.tt
[SIGNED-OFF] Bug 5345 [FOLLOW-UP] DataTables in Koha
[PASSED_QA][SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [15]
[PASSED_QA][SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [13]
[PASSED_QA][SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [10]
[SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [6]
[SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [4]
[SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [3]
[PASSED_QA][SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [6]
lateorders.tt
basketgroup.tt

Description Paul Poulain 2010-11-02 02:52:05 UTC
We will add table sorter & filters « everywhere » in Koha
We will avoid adding sorters & filters where perf is a caveat (like circulation)
Comment 1 Paul Poulain 2011-11-05 10:57:48 UTC
If you want to participate to the upstreaming:
* assign the bug to yourself
* retrieve the patch(es) on git.biblibre.com
* the branch name can be found using
git branch -a|grep NNNN
Usually, it should be 
  biblibre/customer/ft/MTNNNN
For example:
   remotes/origin/stetienne/ft/MT6522

This patch is in branch number 5546
Comment 2 Jonathan Druart 2012-02-17 09:14:07 UTC
We will use DataTables, not tablesorter
Comment 3 Jonathan Druart 2012-02-17 15:02:30 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-02-17 15:02:51 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2012-02-17 15:03:08 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2012-02-17 15:03:25 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2012-02-17 15:03:38 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2012-02-17 15:03:53 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2012-02-17 15:04:06 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2012-02-17 15:04:21 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-02-17 15:04:35 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-02-17 15:04:47 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2012-02-17 15:04:59 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2012-02-17 15:05:17 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2012-02-17 15:05:41 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2012-02-17 15:05:55 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2012-02-17 15:06:10 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2012-02-17 15:06:24 UTC Comment hidden (obsolete)
Comment 19 Owen Leonard 2012-03-06 17:45:09 UTC Comment hidden (obsolete)
Comment 20 Owen Leonard 2012-03-06 17:51:54 UTC Comment hidden (obsolete)
Comment 21 Owen Leonard 2012-03-06 18:02:23 UTC
(In reply to comment #4)
> Created attachment 7720 [details] [review]
> acqui-home.tt

Are the controls really necessary on this page? On my screen the table is pretty small, and the top controls wrap to two lines in an ugly way. I would say either eliminate the controls altogether or have only a minimal set of controls on the top.
Comment 22 Owen Leonard 2012-03-07 15:32:03 UTC Comment hidden (obsolete)
Comment 23 Owen Leonard 2012-03-07 15:39:45 UTC
(In reply to comment #10)
> Created attachment 7726 [details] [review]
> neworderbiblio.tt

Search results on this page are broken into separate pages by the script. I don't think it makes sense to add a table sorter to each individual pages of results. Integration of DataTables on this page should use AJAX to pull individual pages of results.
Comment 24 Owen Leonard 2012-03-07 15:53:21 UTC Comment hidden (obsolete)
Comment 25 Owen Leonard 2012-03-07 17:10:02 UTC
(In reply to comment #12)
> Created attachment 7728 [details] [review]
> ordered.tt

I'm testing this patch with my dateformat system preference set to "metric" and the sort on the "date ordered" column isn't correct. Is DataTables supposed to detect the metric date format automatically?
Comment 26 Alexandre Lengereau 2012-03-19 13:48:48 UTC
<h1>Something went wrong !</h1>Applying: Bug 5345: DataTables integration in acquisition module [1]<br/>
Using index info to reconstruct a base tree...<br/>
Falling back to patching base and 3-way merge...<br/>
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt<br/>
Applying: Bug 5345: DataTables integration in acquisition module [2]<br/>
Applying: Bug 5345: DataTables integration in acquisition module [3]<br/>
Applying: Bug 5345: DataTables integration in acquisition module [4]<br/>
Applying: Bug 5345: DataTables integration in acquisition module [5]<br/>
Applying: Bug 5345: DataTables integration in acquisition module [6]<br/>
Applying: Bug 5345: DataTables integration in acquisition module [7]<br/>
Using index info to reconstruct a base tree...<br/>
Falling back to patching base and 3-way merge...<br/>
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt<br/>
Applying: Bug 5345: DataTables integration in acquisition module [8]<br/>
Applying: Bug 5345: DataTables integration in acquisition module [10]<br/>
Applying: Bug 5345: DataTables integration in acquisition module [11]<br/>
Applying: Bug 5345: DataTables integration in acquisition module [12]<br/>
Using index info to reconstruct a base tree...<br/>
Falling back to patching base and 3-way merge...<br/>
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcels.tt<br/>
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcels.tt<br/>
Failed to merge in the changes.<br/>
Patch failed at 0001 Bug 5345: DataTables integration in acquisition module [12]<br/>
When you have resolved this problem run git am --resolved.<br/>
If you would prefer to skip this patch, instead run git am --skip.<br/>
To restore the original branch and stop patching run git am --abort.<br/>
Bug 5345 - DataTables in Koha<br/>
<br/>
parcel.tt<br/>
Apply? [yn] <br/>
acqui-home.tt<br/>
Apply? [yn] <br/>
addorderiso2709.tt<br/>
Apply? [yn] <br/>
basket.tt<br/>
Apply? [yn] <br/>
basketgroup.tt<br/>
Apply? [yn] <br/>
histsearch.tt<br/>
Apply? [yn] <br/>
lateorders.tt<br/>
Apply? [yn] <br/>
neworderbiblio.tt<br/>
Apply? [yn] <br/>
ordered.tt<br/>
Apply? [yn] <br/>
orderreceive.tt<br/>
Apply? [yn] <br/>
parcels.tt<br/>
Apply? [yn] <br/>
Patch left in /tmp/parcelstt-AYmmg0.patch<br/>
<br/>=== translation installation ===<br/>Install templates
  Install templates 'opac
    From: /home/koha/src/koha-tmpl/opac-tmpl/prog/en/
    To  : /home/koha/src/koha-tmpl/opac-tmpl/prog/fr-FR
    With: /home/koha/src/misc/translator/po/fr-FR-i-opac-t-prog-v-3006000.po
The install seems to be successful.
  Install templates 'intranet
    From: /home/koha/src/koha-tmpl/intranet-tmpl/prog/en/
    To  : /home/koha/src/koha-tmpl/intranet-tmpl/prog/fr-FR
    With: /home/koha/src/misc/translator/po/fr-FR-i-staff-t-prog-v-3006000.po
The install seems to be successful.

If you use this system for the 1st time, you may find usefull to reach the wiki page http://wiki.koha-community.org/wiki/Sandboxes to have some information about those results, to check if everything went well & if you can test the patch properly
Comment 27 Jonathan Druart 2012-03-19 15:06:27 UTC Comment hidden (obsolete)
Comment 28 Alexandre Lengereau 2012-03-19 15:41:14 UTC
Applying: Bug 5345: DataTables integration in acquisition module [1] Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt Applying: Bug 5345: DataTables integration in acquisition module [2] Applying: Bug 5345: DataTables integration in acquisition module [3] Applying: Bug 5345: DataTables integration in acquisition module [4] Applying: Bug 5345: DataTables integration in acquisition module [5] Applying: Bug 5345: DataTables integration in acquisition module [6] Applying: Bug 5345: DataTables integration in acquisition module [7] Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt Applying: Bug 5345: DataTables integration in acquisition module [8] Applying: Bug 5345: DataTables integration in acquisition module [10] Applying: Bug 5345: DataTables integration in acquisition module [11] Applying: Bug 5345: DataTables integration in acquisition module [13] Applying: Bug 5345: DataTables integration in acquisition module [14] Applying: Bug 5345: DataTables integration in acquisition module [15] Applying: Bug 5345 [FOLLOW-UP] DataTables in Koha Applying: Bug 5345: DataTables integration in acquisition module [16] Applying: Bug 5345: DataTables integration in acquisition module [9] Applying: Bug 5345: DataTables integration in acquisition module [12] Bug 5345 - DataTables in Koha parcel.tt Apply? [yn] acqui-home.tt Apply? [yn] addorderiso2709.tt Apply? [yn] basket.tt Apply? [yn] basketgroup.tt Apply? [yn] histsearch.tt Apply? [yn] lateorders.tt Apply? [yn] neworderbiblio.tt Apply? [yn] ordered.tt Apply? [yn] orderreceive.tt Apply? [yn] spent.tt Apply? [yn] supplier.tt Apply? [yn] uncertainprice.tt Apply? [yn] Bug 5345 [FOLLOW-UP] DataTables in Koha Apply? [yn] [SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [16] Apply? [yn] [SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [9] Apply? [yn] parcels.tt Apply? [yn]
=== translation installation ===
Install templates Install templates 'opac From: /home/koha/src/koha-tmpl/opac-tmpl/prog/en/ To : /home/koha/src/koha-tmpl/opac-tmpl/prog/fr-FR With: /home/koha/src/misc/translator/po/fr-FR-i-opac-t-prog-v-3006000.po The install seems to be successful. Install templates 'intranet From: /home/koha/src/koha-tmpl/intranet-tmpl/prog/en/ To : /home/koha/src/koha-tmpl/intranet-tmpl/prog/fr-FR With: /home/koha/src/misc/translator/po/fr-FR-i-staff-t-prog-v-3006000.po The install seems to be successful.
Comment 29 Jonathan Druart 2012-03-19 16:54:46 UTC
This patch can not be tested with a sandbox. Please Test these patches individually
Comment 30 Francois Charbonnier 2012-03-21 15:03:10 UTC
After talking with Jonathan and Corinne (who tested the BZ ticket 7291), we thought that there should be a default sorting within the lateorders page.
Is it possible to get the default sorting (ASC) on the "estimated date"?
Thanks!
Comment 31 Katrin Fischer 2012-04-03 19:26:16 UTC
1) parcel.tt - still applies on current master, but basket and order line columns are not sortable. When clicking on the sort options, I see a javasript error in Firebug. FAILED QA

2) acqui-home.tt - still applies on current master and works. I am not sure having the full toolbar here is necessary - but it works. I see that Owen has commented on that too. NOT signing off.

3) addorderiso2709.tt - still applies on current master. Only affects the list of imports, not the list of records. Signing off.

4) basket.tt - still applies on current master. Modify and Delete columns are sortable. That's not necessary and could be removed in a follow up. Signing off.

5) basketgroup.tt - still applies to master but I don't see where it's applied? QUESTION

6) histsearch.tt - still applies to current master and works. Signing off.

7) lateorders.tt - does not apply. FAILED QA
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt

8) neworderbiblio.tt - still applies on current master. There is still a pagination on top of the page. I think sorting only a single result page does not make sense. We should remove the pagination in favor of the datatables or keep pagination for now. FAILED QA

9) ordered.tt - still applies on current master and works. Signing off.

10) orderreceive.tt - does not apply. FAILED QA
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt

11) spent.tt - still applies on current master and works. Signing off.

12) supplier.tt - still applies on current master and works. Here - similar to aqui-home.tt I am not sure the full toolbars are needed for the contracts table. I think having sortable columns would be enough. NOT signing off.

13) uncertainprice.tt - still applies on current master and works. Signing off.

14) DataTables missing some required images
Works, signing off.

15) parcels.tt - still applies on current master, but there is a merge marker in the patch - FAILED QA.
#   Failed test './koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcels.tt contains merge conflict markers in line 79'
#   at t/00-merge-conflict-markers.t line 51.
# Looks like you failed 1 test of 6260.
Comment 32 Katrin Fischer 2012-04-03 19:27:53 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2012-04-03 19:28:10 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2012-04-03 19:28:24 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2012-04-03 19:28:37 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2012-04-03 19:29:33 UTC Comment hidden (obsolete)
Comment 37 Katrin Fischer 2012-04-03 19:29:46 UTC Comment hidden (obsolete)
Comment 38 Katrin Fischer 2012-04-03 19:30:00 UTC Comment hidden (obsolete)
Comment 39 Paul Poulain 2012-04-13 13:45:41 UTC
Comment on attachment 8830 [details] [review]
[SIGNED-OFF] Bug 5345 [FOLLOW-UP] DataTables in Koha

This patch has been pushed: the -disabled png are referenced in the code already, so they are needed
Comment 40 Marcel de Rooy 2012-05-10 08:13:15 UTC
Attachment 8057 [details] passed qa. Will continue later on.
Comment 41 Paul Poulain 2012-05-11 09:33:48 UTC
Comment on attachment 8057 [details] [review]
[PASSED_QA][SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [16]

this patch has been pushed
Comment 42 Paul Poulain 2012-05-11 09:35:09 UTC
back to sign-off for the patches that are waiting for marcel's QA
Comment 43 Marcel de Rooy 2012-05-11 12:46:54 UTC
While working on the next patch, I still have trouble with the 7178 patch (stopping me from creating items in acq [again!]). So going there first..
Comment 44 Marcel de Rooy 2012-05-11 13:39:41 UTC
Attachment 8058 [details] passed qa.
Will soon continue with 8831.
Comment 45 Marcel de Rooy 2012-05-11 14:47:46 UTC Comment hidden (obsolete)
Comment 46 Marcel de Rooy 2012-05-11 14:49:34 UTC
8831, 8833 and 8834 passed qa. 8835 replaced by 9519 too.
Coming back soon for 8836 and 8837.
Comment 47 Paul Poulain 2012-05-14 13:45:34 UTC
All now-marked-obsolete patches have been pushed, others still need signoff
Comment 48 Jonathan Druart 2012-05-16 09:57:16 UTC Comment hidden (obsolete)
Comment 49 Marcel de Rooy 2012-05-16 11:26:42 UTC
(In reply to comment #47)
> All now-marked-obsolete patches have been pushed, others still need signoff

Could be that a mistake was made somewhere?
Note that Bug 5345: DataTables integration in acquisition module [6] did not yet pass QA, but has already been pushed. (Just noting for the record.) 

Also DataTables integration in acquisition module [4] has been obsoleted. Please check if that is correct. Not QAed nor pushed as far as I can see.

DataTables integration in acquisition module [3] should still be QAed.
Some of the other patches are actually in a kind of Failed QA-state; see the comments from Katrin.
Comment 50 Katrin Fischer 2012-05-16 11:31:52 UTC
Marcel is right, I failed some QA - in the comments. I didn't know how to better do it. I could have reattached the file pehraps, but instead of [SIGNED OFF] marked with [FAILED QA].

Even better I think woudl be a meta bug for changes like this, with lots of smaller bugs surrounding with single patches. Much easier to manage and not growing endlessly like this one.
Comment 51 Marcel de Rooy 2012-05-16 11:36:06 UTC
(In reply to comment #50)
> Marcel is right, I failed some QA - in the comments. I didn't know how to
> better do it. I could have reattached the file pehraps, but instead of
> [SIGNED OFF] marked with [FAILED QA].
You do not need to reattach, just change the description with Edit Details (two clicks away).
Comment 52 Jonathan Druart 2012-05-16 11:46:29 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2012-05-16 12:05:20 UTC
Comment on attachment 7719 [details] [review]
parcel.tt

see Bug 8098
Comment 54 Jonathan Druart 2012-05-16 12:05:40 UTC
Comment on attachment 7726 [details] [review]
neworderbiblio.tt

see Bug 8100
Comment 55 Jonathan Druart 2012-05-16 12:05:53 UTC
Comment on attachment 7729 [details] [review]
orderreceive.tt

see Bug 8101
Comment 56 Jonathan Druart 2012-05-16 12:06:03 UTC
Comment on attachment 7732 [details] [review]
supplier.tt

see Bug 8102
Comment 57 Jonathan Druart 2012-05-16 12:06:18 UTC
Comment on attachment 8292 [details] [review]
parcels.tt

see Bug 8103
Comment 58 Jonathan Druart 2012-05-16 12:06:43 UTC
Comment on attachment 9601 [details] [review]
lateorders.tt

see Bug 8104
Comment 59 Jonathan Druart 2012-05-16 12:07:41 UTC
Comment on attachment 8837 [details] [review]
[SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [3]

see Bug 8106
Comment 60 Jonathan Druart 2012-05-16 12:07:53 UTC
Comment on attachment 9602 [details] [review]
basketgroup.tt

see Bug 8105
Comment 61 Jonathan Druart 2012-05-16 12:09:17 UTC
Comment on attachment 7720 [details] [review]
acqui-home.tt

see Bug 8099
Comment 62 Jonathan Druart 2012-05-16 12:11:05 UTC
Comment on attachment 8837 [details] [review]
[SIGNED-OFF] Bug 5345: DataTables integration in acquisition module [3]

see Bug 8106
Comment 63 Jonathan Druart 2012-05-16 12:11:15 UTC
Comment on attachment 9602 [details] [review]
basketgroup.tt

see Bug 8105
Comment 64 Jonathan Druart 2012-05-16 12:11:55 UTC
(In reply to comment #50)
> Even better I think woudl be a meta bug for changes like this, with lots of
> smaller bugs surrounding with single patches. Much easier to manage and not
> growing endlessly like this one.

Done :)
Comment 65 Marcel de Rooy 2012-05-16 12:14:00 UTC
(In reply to comment #64)
> (In reply to comment #50)
> > Even better I think woudl be a meta bug for changes like this, with lots of
> > smaller bugs surrounding with single patches. Much easier to manage and not
> > growing endlessly like this one.
> 
> Done :)

Did you check my last questions? Appreciate your hard work, but may be harder now to look back what happened..
Comment 66 Jonathan Druart 2012-05-16 13:38:46 UTC
(In reply to comment #49)
> (In reply to comment #47)
> > All now-marked-obsolete patches have been pushed, others still need signoff
> 
> Could be that a mistake was made somewhere?
> Note that Bug 5345: DataTables integration in acquisition module [6] did not
> yet pass QA, but has already been pushed. (Just noting for the record.) 

Yes, indeed :-/

> Also DataTables integration in acquisition module [4] has been obsoleted.
> Please check if that is correct. Not QAed nor pushed as far as I can see.
 
> DataTables integration in acquisition module [3] should still be QAed.
> Some of the other patches are actually in a kind of Failed QA-state; see the
> comments from Katrin.

Yes I created 2 new Bugs for them,
Bug 8106 - DataTables in Acquisision module: addorderiso2709.tt for [3]
and Bug 8109 - DataTables in Acquisision module: basket.tt for [4]

I am under the impression that all bugs have a new Bug or have already pushed.
I hope it will be much easier to test like this.
Comment 67 Owen Leonard 2013-08-13 19:16:35 UTC
I'm marking this fixed because the staff client now has wide use of DataTables, with all pages (with the exception of the cart, Bug 9456) having been converted from the old tablesorter plugin. Future enhancements using DataTables can get their own individual bug reports.