Bug 12561

Summary: Remove non-XSLT views
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: TemplatesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: david, dcook, fridolin.somers, gaetan.boisson, jonathan.druart, julian.maurice, marjorie.barry-vila, martin.renvoize, mtj, mtompset, nick, tomascohen
Version: masterKeywords: Manual
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23663
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This removes the non-XSLT views feature that was deprecated from July 2014. As part of this change: - system preferences HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich are removed - a warning is added to the about page if a default XSLT file was removed, or if a file referenced in one of the system preferences does not exist.
Version(s) released in:
21.11.00
Bug Depends on: 2619, 10571, 11426, 12574, 12900, 12901, 13327, 21891, 22695, 25871    
Bug Blocks: 20396, 5229, 29669    
Attachments: Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich
Bug 12561: Remove non-XSLT views
Bug 12561: Change prototype of XSLTParse4Display
Bug 12561: Add warning on about page
Bug 12561: Change prototype of XSLTParse4Display
Bug 12561: Add warning on about page
Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich
Bug 12561: Remove non-XSLT views
Bug 12561: Change prototype of XSLTParse4Display
Bug 12561: Add warning on about page
Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich
Bug 12561: Remove non-XSLT views
Bug 12561: Change prototype of XSLTParse4Display
Bug 12561: Add warning on about page
Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich
Bug 12561: Remove non-XSLT views
Bug 12561: Change prototype of XSLTParse4Display
Bug 12561: Add warning on about page
Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich
Bug 12561: Remove non-XSLT views
Bug 12561: Change prototype of XSLTParse4Display
Bug 12561: Add warning on about page
Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich
Bug 12561: Remove non-XSLT views
Bug 12561: Change prototype of XSLTParse4Display
Bug 12561: Add warning on about page

Description Katrin Fischer 2014-07-12 11:00:43 UTC
The deprecation of the non-XSLT views for staff and OPAC were discussed at the Developer meetings on 2 July:

http://wiki.koha-community.org/wiki/Development_IRC_meeting,_2_July_2014

This bug is to collect feature differences - features available for non-XSLT views but not for XSLT and other issues that need to be resolved in order to be able to remove the non-XSLT views from Koha.
Comment 1 Gaetan Boisson 2015-04-02 18:18:49 UTC
This is actually not a feature difference, but i just noticed that results display is much slower (twice as slow it seems) when going through xslt. Which is probably not a surprise, but maybe a good thing to mention here.
Comment 2 David Cook 2021-05-04 03:50:24 UTC
I feel like the non-XSLT view was deprecated just not removed?
Comment 3 Owen Leonard 2021-05-04 11:15:44 UTC
(In reply to David Cook from comment #2)
> I feel like the non-XSLT view was deprecated just not removed?

I don't understand what you're saying.
Comment 4 David Cook 2021-05-04 23:39:23 UTC
(In reply to Owen Leonard from comment #3)
> (In reply to David Cook from comment #2)
> > I feel like the non-XSLT view was deprecated just not removed?
> 
> I don't understand what you're saying.

Deprecated just means that it's not endorsed/recommended anymore but it is still available in the system. The XSLT view has been the default and the recommended view for quite a few years now. Even if we never explicitly said it was deprecated it has been deprecated in practice. 

If you look at ./koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt, there are about 152 lines of non-XSLT view. I think at this point that we could safely remove those lines.
Comment 5 David Cook 2021-05-04 23:41:42 UTC
Is the idea that Bug 11426 is the blocker for this?
Comment 6 Katrin Fischer 2021-05-05 18:29:24 UTC
(In reply to David Cook from comment #4)
> (In reply to Owen Leonard from comment #3)
> > (In reply to David Cook from comment #2)
> > > I feel like the non-XSLT view was deprecated just not removed?
> > 
> > I don't understand what you're saying.
> 
> Deprecated just means that it's not endorsed/recommended anymore but it is
> still available in the system. The XSLT view has been the default and the
> recommended view for quite a few years now. Even if we never explicitly said
> it was deprecated it has been deprecated in practice. 
> 
> If you look at ./koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt,
> there are about 152 lines of non-XSLT view. I think at this point that we
> could safely remove those lines.

We did officially deprecate it, check the coding guidelines:
https://wiki.koha-community.org/wiki/Coding_Guidelines#DEPR1:_non-XSLT_search_results_and_bib_display_templates_deprecated

The problems are features not implemented for the non-XSLT view that have been considered as blockers for final removal.
Comment 7 Jonathan Druart 2021-06-18 14:22:37 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2021-06-18 14:22:42 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2021-06-18 14:22:46 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2021-06-18 14:22:51 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2021-06-18 14:25:10 UTC
After 7 years, we are going to remove them, yeah \o/

Test plan:
Play with the different sysprefs and confirm that:
- empty or "default" means the same => the default xslt file is used
- Have a look at the about page, if you are pointing to a file that has been removed you should see a warning there

QA will take care of catching inconsistencies in the code, and eventually code that was used for the non-XSLT views only and must be removed.
Comment 12 Martin Renvoize 2021-06-29 10:36:41 UTC
Sorry Jonathan.. I get 'sha1 information is lacking or useless' on the third patch.

They look good but I can't SO without testing ;)
Comment 13 Jonathan Druart 2021-07-06 08:25:24 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2021-07-06 08:25:31 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2021-07-06 09:16:29 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2021-07-06 09:16:37 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2021-07-06 09:16:42 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2021-07-06 09:16:47 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2021-07-28 15:32:49 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2021-07-28 15:32:57 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2021-07-28 15:33:05 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2021-07-28 15:33:11 UTC Comment hidden (obsolete)
Comment 23 Owen Leonard 2021-07-28 17:47:26 UTC Comment hidden (obsolete)
Comment 24 Owen Leonard 2021-07-28 17:47:31 UTC Comment hidden (obsolete)
Comment 25 Owen Leonard 2021-07-28 17:47:35 UTC Comment hidden (obsolete)
Comment 26 Owen Leonard 2021-07-28 17:47:39 UTC Comment hidden (obsolete)
Comment 27 Tomás Cohen Arazi 2021-07-30 11:42:46 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2021-07-30 11:42:52 UTC Comment hidden (obsolete)
Comment 29 Tomás Cohen Arazi 2021-07-30 11:42:57 UTC Comment hidden (obsolete)
Comment 30 Tomás Cohen Arazi 2021-07-30 11:43:03 UTC Comment hidden (obsolete)
Comment 31 Tomás Cohen Arazi 2021-07-30 11:47:59 UTC
Nice job. There's a QA failure about POD.

Tests still pass:
$ prove t/db_dependent/Search.t \
        t/db_dependent/XSLT.t \
        t/XSLT.t \ 
        t/db_dependent/UsageStats.t \
        t/db_dependent/Auth.t

Overall, the XSLTParse4Display method is well covered by tests so I'm not worried about the lack of tests on the splited subs.
Comment 32 Nick Clemens 2021-07-30 12:09:04 UTC
Created attachment 123305 [details] [review]
Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich

This is the only remaining non-XSLT only feature. We should remove the
non-XSLT views that have been deprecated for a while now, and remove
this feature.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Nick Clemens 2021-07-30 12:09:08 UTC
Created attachment 123306 [details] [review]
Bug 12561: Remove non-XSLT views

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 34 Nick Clemens 2021-07-30 12:09:12 UTC
Created attachment 123307 [details] [review]
Bug 12561: Change prototype of XSLTParse4Display

Use a hash for better readability and reusability.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 35 Nick Clemens 2021-07-30 12:09:16 UTC
Created attachment 123308 [details] [review]
Bug 12561: Add warning on about page

If a default xslt file has been removed or if the one filled in one of
the syspref does not exist.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 36 Nick Clemens 2021-07-30 12:09:50 UTC
(In reply to Tomás Cohen Arazi from comment #31)
> Nice job. There's a QA failure about POD.
> 
> Tests still pass:
> $ prove t/db_dependent/Search.t \
>         t/db_dependent/XSLT.t \
>         t/XSLT.t \ 
>         t/db_dependent/UsageStats.t \
>         t/db_dependent/Auth.t
> 
> Overall, the XSLTParse4Display method is well covered by tests so I'm not
> worried about the lack of tests on the splited subs.

What he said :-) we overlapped
Comment 37 Jonathan Druart 2021-08-16 10:04:17 UTC
Pushed to master for 21.11, thanks to everybody involved!