Bug 16759 - Make OPAC holdings table configurable
Summary: Make OPAC holdings table configurable
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Nick Clemens
URL:
Whiteboard:
Keywords:
Depends on: 17277
Blocks: 19028 19038
  Show dependency treegraph
 
Reported: 2016-06-16 17:33 UTC by Nicole C. Engard
Modified: 2017-11-28 07:12 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 16759: Make OPAC holdings table configurable (11.72 KB, patch)
2017-08-01 17:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16759: (followup) Make it work with OpacSeparateHoldings (1.67 KB, patch)
2017-08-04 18:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16759: (followup) Make it work with OpacSeparateHoldings (1.83 KB, patch)
2017-08-04 19:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16759: Make OPAC holdings table configurable (11.79 KB, patch)
2017-08-07 14:34 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16759: (followup) Make it work with OpacSeparateHoldings (1.89 KB, patch)
2017-08-07 14:34 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16759: Make OPAC holdings table configurable (11.90 KB, patch)
2017-10-21 04:30 UTC, Alex Buckley
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16759: (followup) Make it work with OpacSeparateHoldings (2.02 KB, patch)
2017-10-21 04:33 UTC, Alex Buckley
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16759: Make OPAC holdings table configurable (11.91 KB, patch)
2017-10-21 04:47 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 16759: (followup) Make it work with OpacSeparateHoldings (2.00 KB, patch)
2017-10-21 04:48 UTC, Alex Buckley
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16759: (followup) Make it work with OpacSeparateHoldings (2.02 KB, patch)
2017-10-21 04:49 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 16759: Make OPAC holdings table configurable (11.96 KB, patch)
2017-10-24 13:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16759: (followup) Make it work with OpacSeparateHoldings (2.06 KB, patch)
2017-10-24 13:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16759: (Alternative followup) Handle otherholdings gracefuly (3.03 KB, patch)
2017-10-24 22:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16759: Followup (1.16 KB, patch)
2017-10-25 10:56 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2016-06-16 17:33:59 UTC
Work to be done:
* Make the holdings table in the OPAC configurable using the columns editor
* Add columns hidden by default for home branch, holding branch, copy number and shelving location
Comment 1 Tomás Cohen Arazi 2017-08-01 17:14:12 UTC
Created attachment 65401 [details] [review]
Bug 16759: Make OPAC holdings table configurable

This patch makes the holdings table in the OPAC biblio detail page
configurable using the Columns configuration tools.

This patch:
- Moves the holdingst table from plain DataTable to KohaTable
- Creates a sample yml colvis configuration for this table
  containing all the currently available columns

To test:
- Visit some record's detail page in the OPAC
- Apply this patch
- Reload
=> SUCCESS: The page shows the same information
- On the staff interface, go to:
  Home > Administration > Columns settings
=> SUCCESS: There's a new OPAC section containing the configuration for
   holdingst.
- Choose to hide some fields (both currently displayed and currently hidden).
- Reload the OPAC detail page
=> SUCCESS:
   - Required fields are hidden
   - It doesn't explode because of trying to hide stuff that is not there to start with
- Sign off :-D

Sponsored-by: DOVER
Comment 2 Tomás Cohen Arazi 2017-08-01 17:20:59 UTC
I've attached this preliminary patch, that makes the holdings table in the OPAC detail page easily configurable using the Columns settings configuration page. YAY!

A few remarks for testers and other devs taking a look:
- The original request includes 'shelving location' column. The shelving location is already displayed below the home/holding library. I'm looking forward for opinions on this.
- Some of the currently used sysprefs controlling what's displayed could be removed in favour of using this column configuration. For example ShowCourseReservesHeader, OPACShowBarcode and OpacLocationBranchToDisplay. I can implement that if required by QA. It would be a simple upgrade step setting the right options based on the sysprefs.
- No-js, should we completely remove the columns at template level so non-js browsers can render it properly?
Comment 3 Owen Leonard 2017-08-02 14:41:47 UTC
This patch works well for me and I'd be ready to sign off on it.

This feature may make it more desirable to have shelving location in a separate location, but I don't have a strong opinion about it.

I think it would be nice to transition from the system preferences, but I think it could be handled in separate follow-up bugs.

I think that we should not use js-based column configuration to hide anything which is sensitive information (OPACShowCheckoutName is the one that comes to mind), and let no-js users see the full table.
Comment 4 Tomás Cohen Arazi 2017-08-03 17:46:58 UTC
(In reply to Owen Leonard from comment #3)
> This patch works well for me and I'd be ready to sign off on it.

That's great!

> This feature may make it more desirable to have shelving location in a
> separate location, but I don't have a strong opinion about it.

I filed a separate bug for it to simplify testing. Please check bug 19028.

> I think it would be nice to transition from the system preferences, but I
> think it could be handled in separate follow-up bugs.
> 
> I think that we should not use js-based column configuration to hide
> anything which is sensitive information (OPACShowCheckoutName is the one
> that comes to mind), and let no-js users see the full table.

I agree, that was the main idea behind the approach. I'm about to add a followup for OPACShowBarcode. And am all ears for other sysprefs removal.
Comment 5 Owen Leonard 2017-08-04 14:35:35 UTC
Turning on OpacSeparateHoldings and setting OpacSeparateHoldingsBranch breaks this for logged-in users. The second tab of holdings doesn't get its contents correctly filtered.
Comment 6 Tomás Cohen Arazi 2017-08-04 18:41:52 UTC
Created attachment 65512 [details] [review]
Bug 16759: (followup) Make it work with OpacSeparateHoldings

This patch fixes columns_settings.inc so it works for all objects when
the selector returns more than one object.

To test:
- Have the first patch applied
- Verify it works correctly
- Enable OpacSeparateHoldings
- Reload
=> FAIL: Notice the second holdings tab doesn't have the visualization
rules applied.
- Apply this patch
- Reload
=> SUCCESS: Rules applied correctly!
- Sign off :-D

Sponsored-by: Dover
Comment 7 Tomás Cohen Arazi 2017-08-04 19:15:08 UTC
Created attachment 65513 [details] [review]
Bug 16759: (followup) Make it work with OpacSeparateHoldings

This patch fixes columns_settings.inc so it works for all objects when
the selector returns more than one object.

To test:
- Have the first patch applied
- Verify it works correctly
- Enable OpacSeparateHoldings
- Reload
=> FAIL: Notice the second holdings tab doesn't have the visualization
rules applied.
- Apply this patch
- Reload
=> SUCCESS: Rules applied correctly!
- Sign off :-D

Edit: amended so it preserves the original semantic, returning the
resulting table objects.

Sponsored-by: Dover
Comment 8 Owen Leonard 2017-08-07 14:34:47 UTC
Created attachment 65563 [details] [review]
[SIGNED-OFF] Bug 16759: Make OPAC holdings table configurable

This patch makes the holdings table in the OPAC biblio detail page
configurable using the Columns configuration tools.

This patch:
- Moves the holdingst table from plain DataTable to KohaTable
- Creates a sample yml colvis configuration for this table
  containing all the currently available columns

To test:
- Visit some record's detail page in the OPAC
- Apply this patch
- Reload
=> SUCCESS: The page shows the same information
- On the staff interface, go to:
  Home > Administration > Columns settings
=> SUCCESS: There's a new OPAC section containing the configuration for
   holdingst.
- Choose to hide some fields (both currently displayed and currently hidden).
- Reload the OPAC detail page
=> SUCCESS:
   - Required fields are hidden
   - It doesn't explode because of trying to hide stuff that is not there to start with
- Sign off :-D

Sponsored-by: DOVER

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Owen Leonard 2017-08-07 14:34:50 UTC
Created attachment 65564 [details] [review]
[SIGNED-OFF] Bug 16759: (followup) Make it work with OpacSeparateHoldings

This patch fixes columns_settings.inc so it works for all objects when
the selector returns more than one object.

To test:
- Have the first patch applied
- Verify it works correctly
- Enable OpacSeparateHoldings
- Reload
=> FAIL: Notice the second holdings tab doesn't have the visualization
rules applied.
- Apply this patch
- Reload
=> SUCCESS: Rules applied correctly!
- Sign off :-D

Edit: amended so it preserves the original semantic, returning the
resulting table objects.

Sponsored-by: Dover

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Alex Buckley 2017-10-21 04:30:48 UTC
Created attachment 68342 [details] [review]
Bug 16759: Make OPAC holdings table configurable

This patch makes the holdings table in the OPAC biblio detail page
configurable using the Columns configuration tools.

This patch:
- Moves the holdingst table from plain DataTable to KohaTable
- Creates a sample yml colvis configuration for this table
  containing all the currently available columns

To test:
- Visit some record's detail page in the OPAC
- Apply this patch
- Reload
=> SUCCESS: The page shows the same information
- On the staff interface, go to:
  Home > Administration > Columns settings
=> SUCCESS: There's a new OPAC section containing the configuration for
   holdingst.
- Choose to hide some fields (both currently displayed and currently hidden).
- Reload the OPAC detail page
=> SUCCESS:
   - Required fields are hidden
   - It doesn't explode because of trying to hide stuff that is not there to start with
- Sign off :-D

Sponsored-by: DOVER

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 11 Alex Buckley 2017-10-21 04:33:22 UTC
Created attachment 68343 [details] [review]
[SIGNED-OFF] Bug 16759: (followup) Make it work with OpacSeparateHoldings

This patch fixes columns_settings.inc so it works for all objects when
the selector returns more than one object.

To test:
- Have the first patch applied
- Verify it works correctly
- Enable OpacSeparateHoldings
- Reload
=> FAIL: Notice the second holdings tab doesn't have the visualization
rules applied.
- Apply this patch
- Reload
=> SUCCESS: Rules applied correctly!
- Sign off :-D

Edit: amended so it preserves the original semantic, returning the
resulting table objects.

Sponsored-by: Dover

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Followed test plan, patch worked as described. Also passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 12 Alex Buckley 2017-10-21 04:47:06 UTC
Created attachment 68344 [details] [review]
[SIGNED-OFF] Bug 16759: Make OPAC holdings table configurable

This patch makes the holdings table in the OPAC biblio detail page
configurable using the Columns configuration tools.

This patch:
- Moves the holdingst table from plain DataTable to KohaTable
- Creates a sample yml colvis configuration for this table
  containing all the currently available columns

To test:
- Visit some record's detail page in the OPAC
- Apply this patch
- Reload
=> SUCCESS: The page shows the same information
- On the staff interface, go to:
  Home > Administration > Columns settings
=> SUCCESS: There's a new OPAC section containing the configuration for
   holdingst.
- Choose to hide some fields (both currently displayed and currently hidden).
- Reload the OPAC detail page
=> SUCCESS:
   - Required fields are hidden
   - It doesn't explode because of trying to hide stuff that is not there to start with
- Sign off :-D

Sponsored-by: DOVER

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 13 Alex Buckley 2017-10-21 04:48:16 UTC
Created attachment 68345 [details] [review]
Bug 16759: (followup) Make it work with OpacSeparateHoldings

This patch fixes columns_settings.inc so it works for all objects when
the selector returns more than one object.

To test:
- Have the first patch applied
- Verify it works correctly
- Enable OpacSeparateHoldings
- Reload
=> FAIL: Notice the second holdings tab doesn't have the visualization
rules applied.
- Apply this patch
- Reload
=> SUCCESS: Rules applied correctly!
- Sign off :-D

Edit: amended so it preserves the original semantic, returning the
resulting table objects.

Sponsored-by: Dover

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Followed test plan, patch worked as described. Also passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 14 Alex Buckley 2017-10-21 04:49:55 UTC
Created attachment 68346 [details] [review]
[SIGNED-OFF] Bug 16759: (followup) Make it work with OpacSeparateHoldings

This patch fixes columns_settings.inc so it works for all objects when
the selector returns more than one object.

To test:
- Have the first patch applied
- Verify it works correctly
- Enable OpacSeparateHoldings
- Reload
=> FAIL: Notice the second holdings tab doesn't have the visualization
rules applied.
- Apply this patch
- Reload
=> SUCCESS: Rules applied correctly!
- Sign off :-D

Edit: amended so it preserves the original semantic, returning the
resulting table objects.

Sponsored-by: Dover

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Followed test plan, patch worked as described. Also passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 15 Alex Buckley 2017-10-21 04:52:46 UTC
Hi 

I have taken a second look at these patches for Tomás, and they work well and pass the QA test tool so I have signed off on them as well
Comment 16 Nick Clemens 2017-10-24 13:07:54 UTC
Created attachment 68464 [details] [review]
Bug 16759: Make OPAC holdings table configurable

This patch makes the holdings table in the OPAC biblio detail page
configurable using the Columns configuration tools.

This patch:
- Moves the holdingst table from plain DataTable to KohaTable
- Creates a sample yml colvis configuration for this table
  containing all the currently available columns

To test:
- Visit some record's detail page in the OPAC
- Apply this patch
- Reload
=> SUCCESS: The page shows the same information
- On the staff interface, go to:
  Home > Administration > Columns settings
=> SUCCESS: There's a new OPAC section containing the configuration for
   holdingst.
- Choose to hide some fields (both currently displayed and currently hidden).
- Reload the OPAC detail page
=> SUCCESS:
   - Required fields are hidden
   - It doesn't explode because of trying to hide stuff that is not there to start with
- Sign off :-D

Sponsored-by: DOVER

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Followed test plan, patch worked as described. Also passed QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2017-10-24 13:07:58 UTC
Created attachment 68465 [details] [review]
Bug 16759: (followup) Make it work with OpacSeparateHoldings

This patch fixes columns_settings.inc so it works for all objects when
the selector returns more than one object.

To test:
- Have the first patch applied
- Verify it works correctly
- Enable OpacSeparateHoldings
- Reload
=> FAIL: Notice the second holdings tab doesn't have the visualization
rules applied.
- Apply this patch
- Reload
=> SUCCESS: Rules applied correctly!
- Sign off :-D

Edit: amended so it preserves the original semantic, returning the
resulting table objects.

Sponsored-by: Dover

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Followed test plan, patch worked as described. Also passed QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Jonathan Druart 2017-10-24 18:13:31 UTC
(In reply to Nick Clemens from comment #17)
> Created attachment 68465 [details] [review] [review]
> Bug 16759: (followup) Make it work with OpacSeparateHoldings

There is something wrong in this patch:
1. columns_settings.inc of opac and intranet will differ
2. KohaTable will not return an array, that's weird, it should return a table

I think a better way to do that is that add different ids to the table and instantiate two KohaTable
Comment 19 Tomás Cohen Arazi 2017-10-24 22:19:34 UTC
Created attachment 68507 [details] [review]
Bug 16759: (Alternative followup) Handle otherholdings gracefuly

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2017-10-24 22:20:50 UTC
I've submitted this alternative followup, that's supposed to do the job, but it isn't happening. I can't find a way to solve it yet. I post it so other people can spot the problem with it.
It is supposed to replace the previous followup.
Comment 21 Nick Clemens 2017-10-25 10:56:54 UTC
Created attachment 68528 [details] [review]
Bug 16759: Followup

Use index of elements in specific table
Comment 22 Nick Clemens 2017-10-25 10:58:31 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> I've submitted this alternative followup, that's supposed to do the job, but
> it isn't happening. I can't find a way to solve it yet. I post it so other
> people can spot the problem with it.
> It is supposed to replace the previous followup.

It looks like the problem was the index, we were getting the index of 'th' in all tables - if we specify the index of "selector + ' th'" we get the correct value and columns are hidden.

This should be a safe enough change it could be added to the intranet file too if we need to maintain consistency
Comment 23 Jonathan Druart 2017-10-25 15:17:46 UTC
Pushed to master for 17.11, thanks to everybody involved!