Bug 10212 - Columns configuration for tables
Summary: Columns configuration for tables
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 13372 13492 13864 13865 13866 14207
  Show dependency treegraph
 
Reported: 2013-05-07 13:04 UTC by Jonathan Druart
Modified: 2016-12-05 21:22 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: No Status change
When did the bot last check this: 2013-10-27 00:00:00
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 10212: Columns configuration for tables - Add Colvis files (28.09 KB, patch)
2013-05-07 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (21.99 KB, patch)
2013-05-07 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (28.34 KB, patch)
2013-06-12 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (28.41 KB, patch)
2013-08-26 08:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Make the table names translatable (1.26 KB, patch)
2013-08-27 07:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Add Colvis files (28.09 KB, patch)
2013-09-18 11:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (28.29 KB, patch)
2013-09-18 11:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Make the table names translatable (1.26 KB, patch)
2013-09-18 11:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Add Colvis files (27.86 KB, patch)
2013-09-29 06:53 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (28.47 KB, patch)
2013-09-29 06:53 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10212: Make the table names translatable (1.26 KB, patch)
2013-09-29 06:54 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Add Colvis files (28.09 KB, patch)
2014-01-09 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (28.67 KB, patch)
2014-01-09 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Make the table names translatable (1.26 KB, patch)
2014-01-09 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Add Colvis files (28.16 KB, patch)
2014-01-10 15:46 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (28.72 KB, patch)
2014-01-10 15:46 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10212: Make the table names translatable (1.30 KB, patch)
2014-01-10 15:46 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - QA FIX (2.55 KB, patch)
2014-01-10 17:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Unit tests (10.83 KB, patch)
2014-01-10 17:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: FIX QA issues (3.36 KB, patch)
2014-01-20 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Add Colvis files (28.14 KB, patch)
2014-04-28 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (28.40 KB, patch)
2014-04-28 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Make the table names translatable (1.31 KB, patch)
2014-04-28 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - QA FIX (2.55 KB, patch)
2014-04-28 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Unit tests (10.83 KB, patch)
2014-04-28 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: FIX QA issues (2.35 KB, patch)
2014-04-28 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Add Colvis files (28.14 KB, patch)
2014-06-20 07:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (28.11 KB, patch)
2014-06-20 07:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Make the table names translatable (1.31 KB, patch)
2014-06-20 07:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - QA FIX (2.55 KB, patch)
2014-06-20 07:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Unit tests (10.83 KB, patch)
2014-06-20 07:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: FIX QA issues (2.35 KB, patch)
2014-06-20 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (18.93 KB, patch)
2014-07-07 11:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - DB changes (4.49 KB, patch)
2014-07-07 11:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Unit tests (7.18 KB, patch)
2014-07-07 11:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Add Colvis files (28.14 KB, patch)
2014-07-07 11:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - POC (5.41 KB, patch)
2014-07-07 11:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables (19.95 KB, patch)
2014-07-15 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - DB changes (4.49 KB, patch)
2014-07-15 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Unit tests (7.18 KB, patch)
2014-07-15 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - Add Colvis files (28.10 KB, patch)
2014-07-15 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Columns configuration for tables - POC (5.54 KB, patch)
2014-07-15 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Add Colvis to the about page (1.14 KB, patch)
2014-07-15 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: Move colvis files to the include file (1.81 KB, patch)
2014-07-18 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10212: Columns configuration for tables (20.06 KB, patch)
2014-07-18 13:32 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10212: Columns configuration for tables - DB changes (4.60 KB, patch)
2014-07-18 13:32 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10212: Columns configuration for tables - Unit tests (7.28 KB, patch)
2014-07-18 13:32 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10212: Columns configuration for tables - Add Colvis files (27.97 KB, patch)
2014-07-18 13:32 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10212: Columns configuration for tables - POC (5.64 KB, patch)
2014-07-18 13:32 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10212: Add Colvis to the about page (1.25 KB, patch)
2014-07-18 13:32 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10212: Move colvis files to the include file (1.90 KB, patch)
2014-07-18 13:33 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 10212: Columns configuration for tables (20.12 KB, patch)
2014-07-20 19:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10212: Columns configuration for tables - DB changes (4.61 KB, patch)
2014-07-20 19:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10212: Columns configuration for tables - Unit tests (7.29 KB, patch)
2014-07-20 19:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10212: Columns configuration for tables - Add Colvis files (27.98 KB, patch)
2014-07-20 19:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10212: Columns configuration for tables - POC (5.65 KB, patch)
2014-07-20 19:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10212: Add Colvis to the about page (1.25 KB, patch)
2014-07-20 19:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10212: Move colvis files to the include file (2.24 KB, patch)
2014-07-20 19:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10212: Use the update_or_create DBIC method when possible (2.24 KB, patch)
2014-07-22 08:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10212: (qa followup) accordion collapsed by default (1.05 KB, patch)
2014-08-26 13:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10212: (qa followup) accordion collapsed by default (1.47 KB, patch)
2014-08-26 13:20 UTC, Tomás Cohen Arazi
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-05-07 13:04:10 UTC
Some libraries would want to hide columns in tables.

A configuration page would allow to select specific columns to show/hide in tables.
Comment 1 Jonathan Druart 2013-05-07 13:14:25 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-05-07 13:14:28 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-05-07 13:15:54 UTC
This enhancement is not ready for QA (UT and doc are missing).
I would like to have some feedback about it before completing it.
I switch the status to "Needs SO" for the visibility but it could be "Needs Feedback" :)

Note:
- I don't know if "accordion" is the best choice for the configuration page.
- Tables don't have a good width in some cases. This should be fixed with some css.
Comment 4 Owen Leonard 2013-06-11 17:57:47 UTC
(In reply to comment #3)
> - I don't know if "accordion" is the best choice for the configuration page.

Where is the configuration page? I get a 404 error when I follow the link on the Administration home page.

On the circulation page I get a JavaScript error: TypeError: a.aoColumns[c] is undefined
/intranet-tmpl/prog/en/lib/jquery/plugins/jquery.dataTables.min.js
Line 29
Comment 5 Owen Leonard 2013-06-11 18:00:17 UTC
...and I get this error when I try to navigate directly to /admin/columns_settings.pl:

Template process failed: file error - /koha-tmpl/intranet-tmpl/prog/en/modules/admin/columns_settings.tt: not found at /C4/Templates.pm line 132.
Comment 6 Jonathan Druart 2013-06-12 07:16:59 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-08-23 14:28:23 UTC
Owen, does the last patch fix your error?
Comment 8 Owen Leonard 2013-08-23 15:42:18 UTC
(In reply to Jonathan Druart from comment #7)
> Owen, does the last patch fix your error?

I can now read the configuration settings page without errors, and it seems to work with the circulation checkouts table. However, it had no effect on the holds table. I also don't see a "show/hide columns" control on the holds tab. 

I still get a JavaScript error on the circulation page. Perhaps this is the cause?

TypeError: a.aoColumns[c] is undefined
/intranet-tmpl/prog/en/lib/jquery/plugins/jquery.dataTables.min.js
Line 29

In my informal tests it didn't seem to slow down loading of the circulation page compared to master. I'd like to make the "show/hide columns" box less obtrusive, but I don't have any suggestions for how to do that at the moment.

Is the yml file translatable?
Comment 9 Jonathan Druart 2013-08-26 08:06:46 UTC
(In reply to Owen Leonard from comment #8)
> I still get a JavaScript error on the circulation page. Perhaps this is the
> cause?
> 
> TypeError: a.aoColumns[c] is undefined
> /intranet-tmpl/prog/en/lib/jquery/plugins/jquery.dataTables.min.js
> Line 29

I will submit a new rebased patch. I missed a column for the holds table.

> In my informal tests it didn't seem to slow down loading of the circulation
> page compared to master. I'd like to make the "show/hide columns" box less
> obtrusive, but I don't have any suggestions for how to do that at the moment.

Why do you think it is obtrusive? It is a small box on the top right. Maybe we can imagine (another) syspref for turning off this feature.

> Is the yml file translatable?

Why do you want to translate it? In fact the yaml file will be only used by Koha developers.

Thanks for testing!
Comment 10 Jonathan Druart 2013-08-26 08:08:59 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2013-08-26 18:31:54 UTC
(In reply to Jonathan Druart from comment #9)
> Why do you want to translate it? In fact the yaml file will be only used by
> Koha developers.

On the configuration page the tables are listed by their IDs: "holdst" and "issuest." These are not understandable labels if you're not familiar with the page structure. I was thinking it would be nice to be able to give these better labels, but they would have to be translatable.
Comment 12 Jonathan Druart 2013-08-27 07:20:53 UTC Comment hidden (obsolete)
Comment 13 I'm just a bot 2013-09-18 10:42:29 UTC
Applying: Bug 10212: Columns configuration for tables
Using index info to reconstruct a base tree...
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Patch failed at 0001 Bug 10212: Columns configuration for tables
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 14 Jonathan Druart 2013-09-18 11:56:57 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-09-18 11:57:05 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-09-18 11:57:10 UTC Comment hidden (obsolete)
Comment 17 I'm just a bot 2013-09-29 06:02:40 UTC
Applying: Bug 10212: Columns configuration for tables
Using index info to reconstruct a base tree...
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Patch failed at 0001 Bug 10212: Columns configuration for tables
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 18 Chris Cormack 2013-09-29 06:53:37 UTC Comment hidden (obsolete)
Comment 19 Chris Cormack 2013-09-29 06:53:53 UTC Comment hidden (obsolete)
Comment 20 Chris Cormack 2013-09-29 06:54:06 UTC Comment hidden (obsolete)
Comment 21 I'm just a bot 2013-10-27 06:30:08 UTC
Applying: Bug 10212: Columns configuration for tables
Using index info to reconstruct a base tree...
M	circ/circulation.pl
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging circ/circulation.pl
Patch failed at 0001 Bug 10212: Columns configuration for tables
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 22 Chris Cormack 2013-10-27 06:32:31 UTC
Just db conflicts
Comment 23 Jonathan Druart 2014-01-09 14:02:13 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2014-01-09 14:02:21 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-01-09 14:02:35 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-01-09 14:03:03 UTC
Rebased patches.
Comment 27 Koha Team Lyon 3 2014-01-10 15:44:05 UTC
Test after placing syspref UseTablesortForCirc = enable
It works for checkout table and for holds table : column is hided and you can add it with the link "show/hide columns" "if Cannot be toggled" in admin.
If "Cannot be toggled" is check you can't see the column name with the link "show/hide columns" 
It's a wonderful patch and it'll be great if we can have the thing in the serials module.
Comment 28 sandboxes@biblibre.com 2014-01-10 15:46:10 UTC
Patch tested with a sandbox, by sonia bouis <sonia.bouis@univ-lyon3.fr>
Comment 29 sandboxes@biblibre.com 2014-01-10 15:46:48 UTC Comment hidden (obsolete)
Comment 30 sandboxes@biblibre.com 2014-01-10 15:46:53 UTC Comment hidden (obsolete)
Comment 31 sandboxes@biblibre.com 2014-01-10 15:46:57 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2014-01-10 16:38:18 UTC
I will provide unit tests for new routines.
Comment 33 Jonathan Druart 2014-01-10 17:32:47 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2014-01-10 17:32:54 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2014-01-17 19:06:04 UTC
FAIL   Koha/Template/Plugin/ColumnsSettings.pm
   OK     pod
   FAIL   forbidden patterns
                forbidden pattern: Koha is now under the GPLv3 license (line 9)
   OK     valid
   OK     critic

FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
   FAIL   forbidden patterns
                forbidden pattern: To include datatables files, please use the include file (see bug 10868) (line 20)
   OK     tt_valid
   OK     valid_template
Comment 36 Jonathan Druart 2014-01-20 10:07:29 UTC Comment hidden (obsolete)
Comment 37 Katrin Fischer 2014-04-27 13:09:07 UTC
Sorry, lots of conflicts:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 10212: Columns configuration for tables - Add Colvis files
Applying: Bug 10212: Columns configuration for tables
Using index info to reconstruct a base tree...
M	circ/circulation.pl
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
<stdin>:223: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Comment 38 Jonathan Druart 2014-04-28 08:47:46 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2014-04-28 08:47:51 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2014-04-28 08:47:56 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2014-04-28 08:48:01 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2014-04-28 08:48:07 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2014-04-28 08:48:12 UTC Comment hidden (obsolete)
Comment 44 Katrin Fischer 2014-06-19 23:20:16 UTC
There are conflicts for the usual suspects and one in circulation.tt
Comment 45 Jonathan Druart 2014-06-20 07:15:26 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2014-06-20 07:15:38 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2014-06-20 07:15:44 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2014-06-20 07:15:51 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2014-06-20 07:15:55 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2014-06-20 07:16:00 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2014-06-20 07:16:46 UTC
Last patches fix conflicts with bug 12089.
Comment 52 Katrin Fischer 2014-07-06 20:34:25 UTC
I am sorry Jonathan, with the changes to circ this will no longer apply :( 
Maybe another table could be used for the POC to avoid more conflicts while we are tying to work through this?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 10212: Columns configuration for tables - Add Colvis files
Applying: Bug 10212: Columns configuration for tables
Using index info to reconstruct a base tree...
M	circ/circulation.pl
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
M	koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
<stdin>:223: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/js/datatables.js
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging circ/circulation.pl
CONFLICT (content): Merge conflict in circ/circulation.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 10212: Columns configuration for tables
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-10212-Columns-configuration-for-tables-o21Nap.patch
Comment 53 Jonathan Druart 2014-07-07 11:17:22 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2014-07-07 11:17:30 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2014-07-07 11:17:35 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2014-07-07 11:17:40 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2014-07-07 11:17:45 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2014-07-07 11:21:05 UTC
I add some significant changes in the last patch set :
1/ Use DBIC
2/ Reorganize patches for a better readability
3/ Remove changes in circ/circulation.pl|tt. I now use admin/currency.tt. Hopefully, this will be easier to maintain (less conflict).

This new patch set needs a (quick?) new signoff please.
Comment 59 Katrin Fischer 2014-07-12 21:35:45 UTC
Hi Jonathan, 

thx for providing new patches, this is a fun feature!

Starting with a code review:

1) QA script
I had this error before I ran the database update:
 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/admin/currency.tt
   OK	  forbidden patterns
   FAIL	  valid_template
		DBIx::Class::Storage::DBI::_dbh_execute(): Table 'koha.columns_settings' doesn't exist at /home/katrin/kohaclone/C4/Utils/DataTables/ColumnsSettings.pm line 34

After the database update QA script was ok.

2) Copyright - update?
+# Copyright BibLibre 2013

3) Add new library to About page?
jquery.dataTables.colvis.js

4) Capitalization
Is Hidden by default

Now onto testing:

5) Configuration page
Nice accordeon :) Changes save correctly.
a) After saving a new configuration, it would be nice if the section you worked in remained open, instead of it jumping back to the first - Acquisitions.
b) I am not sure why currency is displayed twice in different font size?

6) Currencies page
The table showed according to what I had configured. Changes made with the options on the page were remembered during my session.
Table shows as normal with Javascript deactivated.

a) The options to toggle/hide could be positioned a bit closer to the table, maybe we could even use an icon later? Just something we could try to improve a bit more.
Comment 60 Owen Leonard 2014-07-14 15:07:40 UTC
Overall this looks good to me. I only have a few minor comments:

- The plugin should be added to /intranet-tmpl/lib/jquery/plugins instead of to the language-specific directory

- The page <title> for columns_settings.tt is "Tables" instead of "Columns settings."

- Spelling: Use American English spellings "Catalog" and "Cataloging" on columns_settings.tt
Comment 61 Owen Leonard 2014-07-14 15:09:15 UTC
One more thing: A link to columns configuration should be added to admin-menu.inc.
Comment 62 Jonathan Druart 2014-07-15 15:56:00 UTC Comment hidden (obsolete)
Comment 63 Jonathan Druart 2014-07-15 15:56:05 UTC Comment hidden (obsolete)
Comment 64 Jonathan Druart 2014-07-15 15:56:12 UTC Comment hidden (obsolete)
Comment 65 Jonathan Druart 2014-07-15 15:56:20 UTC Comment hidden (obsolete)
Comment 66 Jonathan Druart 2014-07-15 15:56:26 UTC Comment hidden (obsolete)
Comment 67 Jonathan Druart 2014-07-15 16:01:31 UTC
Katrin and Owen,

Thanks for your feedbacks!

(In reply to Katrin Fischer from comment #59)
> 2) Copyright - update?
> +# Copyright BibLibre 2013

Done.

> 3) Add new library to About page?
> jquery.dataTables.colvis.js

Oops, forgot. Will be done soon.

> 4) Capitalization
> Is Hidden by default

Done.

> 5) Configuration page
> a) After saving a new configuration, it would be nice if the section you
> worked in remained open, instead of it jumping back to the first -
> Acquisitions.

Done.

> b) I am not sure why currency is displayed twice in different font size?

The first one is the "page name", the second one is the "table name".
It will often be the same, but in some case, there will be 2 tables per page.

> 6) Currencies page
> a) The options to toggle/hide could be positioned a bit closer to the table,
> maybe we could even use an icon later? Just something we could try to
> improve a bit more.

Not sure I am a really bad designer and I will certainly break something if I change the datatable css file. I would prefer to let this job to a css expert.

(In reply to Owen Leonard from comment #60)
> - The plugin should be added to /intranet-tmpl/lib/jquery/plugins instead of
> to the language-specific directory

Done.

> - The page <title> for columns_settings.tt is "Tables" instead of "Columns
> settings."

Done.

> - Spelling: Use American English spellings "Catalog" and "Cataloging" on
> columns_settings.tt

Done.

(In reply to Owen Leonard from comment #61)
> One more thing: A link to columns configuration should be added to
> admin-menu.inc.

Done.
Comment 68 Jonathan Druart 2014-07-15 16:08:33 UTC Comment hidden (obsolete)
Comment 69 Jonathan Druart 2014-07-18 13:22:41 UTC Comment hidden (obsolete)
Comment 70 Owen Leonard 2014-07-18 13:32:06 UTC Comment hidden (obsolete)
Comment 71 Owen Leonard 2014-07-18 13:32:17 UTC Comment hidden (obsolete)
Comment 72 Owen Leonard 2014-07-18 13:32:28 UTC Comment hidden (obsolete)
Comment 73 Owen Leonard 2014-07-18 13:32:37 UTC Comment hidden (obsolete)
Comment 74 Owen Leonard 2014-07-18 13:32:45 UTC Comment hidden (obsolete)
Comment 75 Owen Leonard 2014-07-18 13:32:53 UTC Comment hidden (obsolete)
Comment 76 Owen Leonard 2014-07-18 13:33:02 UTC Comment hidden (obsolete)
Comment 77 Owen Leonard 2014-07-18 13:34:45 UTC
(In reply to Katrin Fischer from comment #59)
> a) The options to toggle/hide could be positioned a bit closer to the table,
> maybe we could even use an icon later?

I think this could be improved but I don't have a good suggestion right now. I'm going to keep thinking about it.
Comment 78 Katrin Fischer 2014-07-20 19:56:27 UTC
Created attachment 29882 [details] [review]
[PASSED QA] Bug 10212: Columns configuration for tables

This development introduces ColVis into Koha and provides a configuration
page for columns visibility.

ColVis is a plugin for DataTables. It allows to change the visibility of
the columns in the table.

* This development adds:
  - the js and css file for ColVis
  - a new DB table 'columns_settings'
  - a new template plugin 'ColumnsSettings'
  - a new package C4::Utils::DataTables::ColumnsSettings
  - a new admin page admin/columns_settings.pl

* How it works:
A yaml file is created (admin/columns_settings.yml) in order to take an
inventory of all tables where ColVis is implemented. This file is read
to create the list of modules, pages and tables in the configuration
page.
There are 3 possible keys in the yml:
 - is_hidden: default is 0
The column will be hidden.
 - cannot_be_toggled: default is 0.
ColVis will allow to hide/show the column.
 - cannot_be_modified: default is 0
Default values (in the yml) won't be modifiable.

When a user changes (or saves) the configuration for one module, all
columns are added to the DB table. The values in the DB get the upper hand
on the yaml values.

* Humm, strange?
It seems weird to have 2 storages for the same values. But I
think it will be easy to add an entry and maintain the yaml rather than
adding a new row (and new entry in updatedatabase script) in the DB.

* To go further: We can imagine that the configuration is saved for each
user (and not globally like it is made with this patch).

This patch cannot be tested as it, you need to apply the "POC" patch.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script, more comments on last patch.
Comment 79 Katrin Fischer 2014-07-20 19:56:33 UTC
Created attachment 29883 [details] [review]
[PASSED QA] Bug 10212: Columns configuration for tables - DB changes

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 80 Katrin Fischer 2014-07-20 19:56:38 UTC
Created attachment 29884 [details] [review]
[PASSED QA] Bug 10212: Columns configuration for tables - Unit tests

test plan:
Verify the
  prove t/db_dependent/ColumnsSettings.t
returns green.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 81 Katrin Fischer 2014-07-20 19:56:44 UTC
Created attachment 29885 [details] [review]
[PASSED QA] Bug 10212: Columns configuration for tables - Add Colvis files

This patch only adds Colvis js and css files.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 82 Katrin Fischer 2014-07-20 19:56:49 UTC
Created attachment 29886 [details] [review]
[PASSED QA] Bug 10212: Columns configuration for tables - POC

This patch proves that this feature is useful with a concret use case.

Test plan:
1/ Execute the updatedatabase in order to create the new table.
2/ Take a look to the yml structure.
3/ Go on the admin currency page (admin/currency.pl).
4/ Check that you cannot hide the 2 first columns on the table.
5/ Try to hide/show columns.
6/ Go on the columns configuration page (admin/columns_settings.pl).
7/ Only the admin tab are filled with data. Check/uncheck checkboxes and
save.
8/ Go on the admin currency page and check that the behavior is what
you expected.
9/ Give me some feedback :)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 83 Katrin Fischer 2014-07-20 19:56:53 UTC
Created attachment 29887 [details] [review]
[PASSED QA] Bug 10212: Add Colvis to the about page

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 84 Katrin Fischer 2014-07-20 19:56:56 UTC
Created attachment 29888 [details] [review]
[PASSED QA] Bug 10212: Move colvis files to the include file

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.

Implements the option to change the default columns that appear in
a table while giving the user the option to change the columns
individually for a session.

As an example, this feature is implemented for the currencies table.

Works well for all possible combinations in configuration.
Comment 85 David Cook 2014-07-22 00:04:22 UTC
Comment on attachment 29882 [details] [review]
[PASSED QA] Bug 10212: Columns configuration for tables

Review of attachment 29882 [details] [review]:
-----------------------------------------------------------------

::: C4/Utils/DataTables/ColumnsSettings.pm
@@ +66,5 @@
> +    for my $c (@$columns) {
> +        $c->{is_hidden}         //= 0;
> +        $c->{cannot_be_toggled} //= 0;
> +
> +        my $column = $schema->resultset('ColumnsSetting')->search(

Have you considered using "update_or_create" instead of these 3 methods?

Admittedly, I'm not sure what about the significance of "$column = $column->first;" below. However, if that's the case, I'd suggest "find_or_new" or "find_or_create". 

http://search.cpan.org/~ribasushi/DBIx-Class-0.08270/lib/DBIx/Class/ResultSet.pm#update_or_create

my $column = $schema->resultset('ColumnsSetting')->update_or_create({
    module            => $c->{module},    
    page              => $c->{page},
    tablename         => $c->{tablename},
    columnname        => $c->{columnname},
    is_hidden         => $c->{is_hidden},
    cannot_be_toggled => $c->{cannot_be_toggled},
  });

Regardless of whether you choose to go with this method, I think it would be a good idea to add a primary key constraint to your DbiC methods. I haven't looked at the proposed table structure, so I don't know if there are any, but probably a good idea.
Comment 86 David Cook 2014-07-22 00:12:29 UTC
I haven't tested any of these patches, but excited to have a mechanism for controlling column visibility. I thought about adding the ColVis files locally, so I'm happy to see a patch for them to go into Koha proper.
Comment 87 Jonathan Druart 2014-07-22 08:23:27 UTC
Created attachment 29947 [details] [review]
Bug 10212: Use the update_or_create DBIC method when possible

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 88 Jonathan Druart 2014-07-22 08:25:13 UTC
(In reply to David Cook from comment #85)
> ::: C4/Utils/DataTables/ColumnsSettings.pm
> @@ +66,5 @@
> > +    for my $c (@$columns) {
> > +        $c->{is_hidden}         //= 0;
> > +        $c->{cannot_be_toggled} //= 0;
> > +
> > +        my $column = $schema->resultset('ColumnsSetting')->search(
> 
> Have you considered using "update_or_create" instead of these 3 methods?

Hum... don't repeat what you saw ;)
Comment 89 David Cook 2014-07-22 22:58:00 UTC
(In reply to Jonathan Druart from comment #88)
> (In reply to David Cook from comment #85)
> > ::: C4/Utils/DataTables/ColumnsSettings.pm
> > @@ +66,5 @@
> > > +    for my $c (@$columns) {
> > > +        $c->{is_hidden}         //= 0;
> > > +        $c->{cannot_be_toggled} //= 0;
> > > +
> > > +        my $column = $schema->resultset('ColumnsSetting')->search(
> > 
> > Have you considered using "update_or_create" instead of these 3 methods?
> 
> Hum... don't repeat what you saw ;)

Hehe. No worries. I've been reading a lot about DBIC this week, and thought it might be relevant :).
Comment 90 Tomás Cohen Arazi 2014-08-26 13:01:43 UTC Comment hidden (obsolete)
Comment 91 Tomás Cohen Arazi 2014-08-26 13:20:32 UTC
Created attachment 31159 [details] [review]
Bug 10212: (qa followup) accordion collapsed by default

By default, the accordion shows the first element (Acquisitions) which
is empty, and missleading. This patch collapses it by default.

The expected behaviour is that if we are rendering because the user saved
its changes, the modified accordion should be expanded, so I added a test
for the $panel variable.

Template comment added for future references about it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 92 Tomás Cohen Arazi 2014-08-26 13:34:15 UTC
Patches pushed to master.

Thanks Jonathan!