|
Description
Nicole C. Engard
2016-06-16 17:33:59 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 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? 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. (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. Turning on OpacSeparateHoldings and setting OpacSeparateHoldingsBranch breaks this for logged-in users. The second tab of holdings doesn't get its contents correctly filtered. 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 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 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> 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> 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> 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> 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> 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> 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> 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 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> 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> (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 Created attachment 68507 [details] [review] Bug 16759: (Alternative followup) Handle otherholdings gracefuly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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. Created attachment 68528 [details] [review] Bug 16759: Followup Use index of elements in specific table (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 Pushed to master for 17.11, thanks to everybody involved! |