Bug 17385 - Add custom export formats for bibliographic records
Summary: Add custom export formats for bibliographic records
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Target Milestone: ---
Assignee: Charles Farmer
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-30 15:17 UTC by Bouzid Fergani
Modified: 2019-03-22 09:53 UTC (History)
11 users (show)

See Also:
Change 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 17385 - Add custom export notices (32.51 KB, patch)
2016-09-30 15:36 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 17385 - Add custom export notices (32.33 KB, patch)
2016-12-20 17:04 UTC, Bouzid Fergani
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17385 - Add custom export notices (32.53 KB, patch)
2017-06-06 10:50 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17385 - Fix pod (608 bytes, patch)
2017-06-06 10:50 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17385 - Add custom export XSLT (25.66 KB, patch)
2017-09-26 14:18 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 17385 - Add custom export XSLT (25.79 KB, patch)
2017-10-03 23:56 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17385: Add custom export XSLT (25.84 KB, patch)
2018-04-20 20:09 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 17385: Add tests for CustomXSLTExportList (8.64 KB, patch)
2018-04-20 20:09 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 17385: Add custom export XSLT (26.12 KB, patch)
2018-09-18 15:46 UTC, Blou
Details | Diff | Splinter Review
Bug 17385: Add tests for CustomXSLTExportList (8.66 KB, patch)
2018-09-18 15:47 UTC, Blou
Details | Diff | Splinter Review
Bug 17385: Add tests for CustomXSLTExportList (8.98 KB, patch)
2018-10-16 18:07 UTC, Cori Lynn Arnold
Details | Diff | Splinter Review
Bug 17385: Add custom export XSLT (26.28 KB, patch)
2018-10-28 09:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17385: Add tests for CustomXSLTExportList (8.98 KB, patch)
2018-10-28 09:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17385: Add custom export XSLT (26.15 KB, patch)
2019-01-24 19:33 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 17385: Add custom export XSLT (26.15 KB, patch)
2019-01-24 19:35 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 17385: Add tests for CustomXSLTExportList (8.95 KB, patch)
2019-01-24 19:35 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 17385: Add custom export XSLT (26.86 KB, patch)
2019-03-22 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17385: Add tests for CustomXSLTExportList (9.08 KB, patch)
2019-03-22 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17385: (QA follow-up) Do not needlessly scan biblioexport folder (1.92 KB, patch)
2019-03-22 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17385: (QA follow-up) Add caching (2.84 KB, patch)
2019-03-22 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17385: (QA follow-up) Do not needlessly scan biblioexport folder (2.09 KB, patch)
2019-03-22 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17385: (QA follow-up) Add caching (2.97 KB, patch)
2019-03-22 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bouzid Fergani 2016-09-30 15:17:50 UTC
this patch, add custom export notices using xslt.
Comment 1 Bouzid Fergani 2016-09-30 15:36:34 UTC
Created attachment 55961 [details] [review]
Bug 17385 - Add custom export notices

  test case:
        - Apply patch
        - Create folder /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport for intranet
        - Create folder /koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport for opac
        - In the folder /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport-samples, you found sample xslt
        - Copie this file in /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport for intranet and in /koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport for opac.
        - Open intranet or opac interface and submit any search, open any notices found.
        - Click save and you found Simple Export.
        - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
        - It work correctly in basket and lists.
        - The name 'Simple Export' it's extract in the xslt file, curently the name is MARC21_simple_export.xsl.
Comment 2 Michael Kuhn 2016-10-16 14:15:12 UTC
I applied the patch. The following is as you describe:

* Patch can be applied
* The folder /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport for intranet is there
* The folder /koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport for opac is there
* In the folder /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport-samples there is the sample file "MARC21_simple_export.xsl"
* This file can also be found in /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport for intranet and in /koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport for opac

As the next point you write: "Open intranet or opac interface and submit any search, open any notices found."

I opened both Intranet and OPAC and searched for the only book in my test catalog. But there were no "notices" found so I couldn't open any. What do you mean with "notices"?
Comment 3 Katrin Fischer 2016-10-16 14:23:15 UTC
notice = record (French?)
Comment 4 Bouzid Fergani 2016-10-17 13:00:35 UTC
Hi Michael,
sorry, i not write correctly
    - Apply patch
    - Open intranet or opac interface and submit any search, click title of one item found.
    - Click Save and you found Simple Export.
    - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
    - You can use it in basket and lists.
    - The name 'Simple Export' it's extract in the xslt file, curently the name is MARC21_simple_export.xsl.
    
Bouzid.
Comment 5 Michael Kuhn 2016-10-19 20:34:49 UTC
I applied the patch again. The following is as you describe:

* Patch can be applied
* The folder /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport for intranet is there
* The folder /koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport for opac is there
* In the folder /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport-samples there is the sample file "MARC21_simple_export.xsl"
* This file can also be found in /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport for intranet and in /koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport for opac
* I open the Intranet interface, submit a search, click the title of a found item

Then I click the button "Save", but I can only see formats BIBTEX, Dublin Core, MARCXML, three kinds of MARC, MODS and RIS - but no "Simple Export".

In the OPAC interface it is almost the same. There I click the button "Save record" but there are only the formats already given above, plus one more (ISBD). But no "Simple Export".
Comment 6 Bouzid Fergani 2016-10-25 14:58:13 UTC
Are you sure MARC21_simple_export.xsl exist in folders /koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport and /koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport, because for me, when i apply this patch in actual master, it work correctly.
Comment 7 Michael Kuhn 2016-10-25 21:54:24 UTC
I have set up a completely new kohadevbox.

vagrant@kohadevbox:~$ cd kohaclone
vagrant@kohadevbox:kohaclone(master)$ git pull
Already up-to-date.
vagrant@kohadevbox:kohaclone(master)$ git checkout -b 001
Switched to a new branch '001'

Then I tried to apply patch 17385 again, but I was not able to do it:

vagrant@kohadevbox:kohaclone(001)$ git bz apply 17385
Bug 17385 - Add custom export notices

55961 - Bug 17385 - Add custom export notices

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17385 - Add custom export notices
fatal: sha1 information is lacking or useless (opac/opac-detail.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 17385 - Add custom export notices
The copy of the patch that failed is found in:
   /home/vagrant/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-17385---Add-custom-export-notices-0lLL49.patch
Comment 8 Bouzid Fergani 2016-12-20 17:04:29 UTC
Created attachment 58308 [details] [review]
Bug 17385 - Add custom export notices

test case:
        - Apply patch
        - Open intranet or opac interface and submit any search.
        - Click title of found item
        - Click save and you found Simple Export.
        - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
        - It work correctly in basket and lists.
        - The name 'Simple Export' it's extract in the xslt file, curently the name is MARC21_simple_export.xsl.
    for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
    for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl
Comment 9 Bouzid Fergani 2016-12-20 17:05:09 UTC
Rebase patch
Comment 10 Josef Moravec 2017-06-06 10:50:07 UTC
Created attachment 64033 [details] [review]
[SIGNED-OFF] Bug 17385 - Add custom export notices

test case:
        - Apply patch
        - Open intranet or opac interface and submit any search.
        - Click title of found item
        - Click save and you found Simple Export.
        - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
        - It work correctly in basket and lists.
        - The name 'Simple Export' it's extract in the xslt file, curently the name is MARC21_simple_export.xsl.
    for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
    for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Josef Moravec 2017-06-06 10:50:10 UTC
Created attachment 64034 [details] [review]
[SIGNED-OFF] Bug 17385 - Fix pod

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Marcel de Rooy 2017-09-01 08:28:06 UTC
QA: Looking here now
Comment 13 Marcel de Rooy 2017-09-01 08:50:18 UTC
QA Comment:
Thanks for your work. I think this is not yet ready for inclusion.

 sub XSLTParse4Display {
-    my ( $biblionumber, $orig_record, $xslsyspref, $fixamps, $hidden_items, $sysxml, $xslfilename, $lang ) = @_;
+    my ( $biblionumber, $orig_record, $xslsyspref, $fixamps, $hidden_items, $sysxml, $xslfilename, $lang, $xslFile, $opac ) = @_;
We already have $xslfilename. You add $xslFile ? Does not look good. 
The added code seems repetitive.

You add a new routine CustomXSLTExportList. It needs unit tests.
In that routine you are parsing the xsl files for something like +<!-- xml title="Simple Export" -->. I am not sure if that is a good idea. Another QA pov is welcome too.
Note that you are doing it each time again too.
If you provide clear xslt names, that should probably be sufficient. Please think about another way to enrich that information if needed.

--- a/Koha/XSLT_Handler.pm
+++ b/Koha/XSLT_Handler.pm
@@ -168,7 +168,6 @@ sub transform {
         ( $xml, $filename, $format ) = @_;
         $format ||= 'chars';
     }
-
     #Initialized yet?
     if ( !$self->{xslt_hash} ) {
         $self->_init;
No need to remove an empty line if you are not touching this module anyway.

diff --git a/koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/README b/koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/README
new file mode 100644
Empty README ?
And another further on.

koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport-samples/MARC21_simple_export.xsl koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl
What is the use of adding two identical files here ?
Actually, all files here are sort of sample files.
Comment 14 David Bourgault 2017-09-26 14:18:02 UTC
Created attachment 67357 [details] [review]
Bug 17385 - Add custom export XSLT

Patch was corrected and rebased after QA feedback. Previous patches where combined into this one.

- Empty READMEs have been deleted
- Whitespaces changes to unrelated code have been undone
* CustomXSLTExportList now uses LibXML to figure out the title and output
    * The custom export XSLT's title should be an attribute to the root node, otherwise it will default to filename
- Duplicate $xslFile param was removed from XSLTParse4Display
* Improvment to code structure in XSLTParse4Display
* Some fixes to calls to AddBiblio()

Test case:
  - Apply patch
  - Visit an item's detail page in the OPAC or the staff interface
  - Click save and you found Simple Export.
  - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
  - It work correctly in basket and lists.

The patch comes with sample XSLT files
  for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
  for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl
Comment 15 Alex Buckley 2017-10-03 23:56:43 UTC
Created attachment 67579 [details] [review]
Bug 17385 - Add custom export XSLT

Patch was corrected and rebased after QA feedback. Previous patches where combined into this one.

- Empty READMEs have been deleted
- Whitespaces changes to unrelated code have been undone
* CustomXSLTExportList now uses LibXML to figure out the title and output
    * The custom export XSLT's title should be an attribute to the root node, otherwise it will default to filename
- Duplicate $xslFile param was removed from XSLTParse4Display
* Improvment to code structure in XSLTParse4Display
* Some fixes to calls to AddBiblio()

Test case:
  - Apply patch
  - Visit an item's detail page in the OPAC or the staff interface
  - Click save and you found Simple Export.
  - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
  - It work correctly in basket and lists.

The patch comes with sample XSLT files
  for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
  for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl

Followed test plan, patch works as described. Also ran QA test tools and
modified files pass

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 16 Marcel de Rooy 2018-01-12 07:19:04 UTC
Thx for your adjustments! Looks good to me. Final comments:

This is not a blocker for me, but OpacExportOptions controls the options listed by detail sidebar. Your custom exports are listed no matter what. It might be better to let OpacExportOptions also control if the custom exports should be added to the list somehow. Maybe not one by one, but just on directory level (show them all or not)?
But do not make big adjustments here. It could be handled on a new report or we could simply state: remove the xsl files from biblioexport.

XSLTParse4Display is not yet tested, but your change should officially be tested ;)
    Also I recommend to remove the opac parameter. It is not consistent with the approach followed in the sub. Not changing the parameters could help you here.
    Please send XSLTCustomExport and OPACXSLTCustomExport to the routine; this is consistent and does the same thing.
    Since your change only deals with the $xslfilename, I could live without a test script (but I am not sure about the RM).

CustomXSLTExportList is new and should really have its own test (should not be hard)
    Mock opachtdocs and create the dir in a tempdir. Check on returned title and method.
Comment 17 David Bourgault 2018-04-20 20:09:28 UTC
Created attachment 74660 [details] [review]
Bug 17385: Add custom export XSLT

Patch was corrected and rebased after QA feedback. Previous patches where combined into this one.

- Empty READMEs have been deleted
- Whitespaces changes to unrelated code have been undone
* CustomXSLTExportList now uses LibXML to figure out the title and output
    * The custom export XSLT's title should be an attribute to the root node, otherwise it will default to filename
- Duplicate $xslFile param was removed from XSLTParse4Display
* Improvment to code structure in XSLTParse4Display
* Some fixes to calls to AddBiblio()

Test case:
  - Apply patch
  - Visit an item's detail page in the OPAC or the staff interface
  - Click save and you found Simple Export.
  - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
  - It work correctly in basket and lists.

The patch comes with sample XSLT files
  for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
  for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl

Followed test plan, patch works as described. Also ran QA test tools and
modified files pass

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

https://bugs.koha-community.org/show_bug.cgi?id=17835
Comment 18 David Bourgault 2018-04-20 20:09:40 UTC
Created attachment 74661 [details] [review]
Bug 17385: Add tests for CustomXSLTExportList

+ Added tests for CustomXSLTExportList to C4::XSLT
+ Added an eval block in CustomXSLTExportList to handle invalid XML.

* Removed Opac parameter, but now handle "OPACXSLTCustomExport" as equivalent

+ Added "Custom" option to OPACExportOptions preference.
Comment 19 Martin Renvoize 2018-06-13 09:54:25 UTC
Comment on attachment 74660 [details] [review]
Bug 17385: Add custom export XSLT

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

Generally looks good with a few minor code comments added for review.

::: C4/XSLT.pm
@@ +197,5 @@
>      $xslfilename ||= C4::Context->preference($xslsyspref);
>      $lang ||= C4::Languages::getlanguage();
>  
> +    if ($xslsyspref eq "XSLTCustomExport") {
> +        my $dir;

The code used to defined $dir in CustomXSLTExportList which does the same thing is cleaner.. why not use that here too... or even factor it out and use the same factored out routine for both to keep it DRY?

@@ +370,5 @@
>  }
>  
> +=head2 CustomXSLTExportList
> +
> + Returns list of file for custom xslt conversion

Please add accepted parameters (i.e. $opac) to the POD section and note their required status and what they accept (string, int, etc)
Comment 20 Martin Renvoize 2018-06-13 10:03:37 UTC
Ignore the first comment in that review.. you've already changed it in the followup.
Comment 21 Katrin Fischer 2018-08-23 06:31:44 UTC
I am sorry, patch doesn't apply with a lot of conflicts. Can you please rebase?
Comment 22 Blou 2018-09-18 15:46:25 UTC
Created attachment 79055 [details] [review]
Bug 17385: Add custom export XSLT

Patch was corrected and rebased after QA feedback. Previous patches where combined into this one.

- Empty READMEs have been deleted
- Whitespaces changes to unrelated code have been undone
* CustomXSLTExportList now uses LibXML to figure out the title and output
    * The custom export XSLT's title should be an attribute to the root node, otherwise it will default to filename
- Duplicate $xslFile param was removed from XSLTParse4Display
* Improvment to code structure in XSLTParse4Display
* Some fixes to calls to AddBiblio()

Test case:
  - Apply patch
  - Visit an item's detail page in the OPAC or the staff interface
  - Click save and you found Simple Export.
  - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
  - It work correctly in basket and lists.

The patch comes with sample XSLT files
  for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
  for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl

Followed test plan, patch works as described. Also ran QA test tools and
modified files pass

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

https://bugs.koha-community.org/show_bug.cgi?id=17835
Comment 23 Blou 2018-09-18 15:47:59 UTC
Created attachment 79056 [details] [review]
Bug 17385: Add tests for CustomXSLTExportList

+ Added tests for CustomXSLTExportList to C4::XSLT
+ Added an eval block in CustomXSLTExportList to handle invalid XML.

* Removed Opac parameter, but now handle "OPACXSLTCustomExport" as equivalent

+ Added "Custom" option to OPACExportOptions preference.
Comment 24 Cori Lynn Arnold 2018-10-16 18:07:01 UTC
Created attachment 80675 [details] [review]
Bug 17385: Add tests for CustomXSLTExportList

+ Added tests for CustomXSLTExportList to C4::XSLT
+ Added an eval block in CustomXSLTExportList to handle invalid XML.

* Removed Opac parameter, but now handle "OPACXSLTCustomExport" as equivalent

+ Added "Custom" option to OPACExportOptions preference.

Tested with invalid XML (not originally on purpose, but...) and then
added a new field to add to the simple export.

Walked through the testing steps.

Question: should this also apply to the cataloging page as an export
option? It doesn't now.

Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>
Comment 25 Katrin Fischer 2018-10-28 09:46:45 UTC
I've attached rebased patches and added Cori Lynn's sign-off to the first patch as well. 

1) Can you please add POD to CustomXSLTExportList (see comment #19)

2) There is lots of minor (fast to fix) fails in the QA script:

FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc
   FAIL	  filters
		wrong_html_filter at line 100 (            <li><a href="/cgi-bin/koha/catalogue/export.pl?format=[% filesOption.format %]&amp;op=exportxsl&amp;file=[% filesOption.value %]&amp;bib=[% biblionumber | html %]">[% filesOption.filename %]</a></li>)
		missing_filter at line 100 (            <li><a href="/cgi-bin/koha/catalogue/export.pl?format=[% filesOption.format %]&amp;op=exportxsl&amp;file=[% filesOption.value %]&amp;bib=[% biblionumber | html %]">[% filesOption.filename %]</a></li>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/virtualshelves-toolbar.inc
   FAIL	  filters
		wrong_html_filter at line 27 (                        <li><a href="/cgi-bin/koha/virtualshelves/downloadshelf.pl?format=xsl.[% xsl_export.value %]&amp;shelfid=[% shelf.shelfnumber %]">XSL - [% xsl_export.filename %]</a></li>)
		missing_filter at line 27 (                        <li><a href="/cgi-bin/koha/virtualshelves/downloadshelf.pl?format=xsl.[% xsl_export.value %]&amp;shelfid=[% shelf.shelfnumber %]">XSL - [% xsl_export.filename %]</a></li>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/basket/basket.tt
   FAIL	  filters
		missing_filter at line 60 (                                <li><a href="/cgi-bin/koha/basket/downloadcart.pl?format=xsl.[% xsl_export.value %]&amp;bib_list=[% bib_list %]">XSL - [% xsl_export.filename %]</a></li>)
		wrong_html_filter at line 60 (                                <li><a href="/cgi-bin/koha/basket/downloadcart.pl?format=xsl.[% xsl_export.value %]&amp;bib_list=[% bib_list %]">XSL - [% xsl_export.filename %]</a></li>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
   FAIL	  filters
		missing_filter at line 860 (            <option value="[% filesOption.value %]">[% filesOption.filename %]</option>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/downloadshelf.tt
   FAIL	  filters
		missing_filter at line 40 (            <option value="xsl.[% xsl_export.value %]">XSL - [% xsl_export.filename %]</option>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
   OK	  git manipulation
   OK	  xml_valid

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-detail-sidebar.inc
   FAIL	  filters
		missing_filter at line 71 (                                        <li><a role="menuitem" href="/cgi-bin/koha/opac-export.pl?format=[% filesOption.format %]&amp;op=exportxsl&amp;file=[% filesOption.value %]&amp;bib=[% biblionumber %]">[% filesOption.filename %]</a></li>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-downloadcart.tt
   FAIL	  filters
		missing_filter at line 24 (                                    <option value="xsl.[% xsl_export.value %]">XSL - [% xsl_export.filename %]</option>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-downloadshelf.tt
   FAIL	  filters
		missing_filter at line 63 (                                                <option value="xsl.[% xsl_export.value %]">XSL - [% xsl_export.filename %]</option>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

3) t/XSLT.t passes, but there is a warning that should be fixed:
t/XSLT.t .. 2/12 Use of uninitialized value $outputformat in string eq at /home/vagrant/kohaclone/C4/XSLT.pm line 404.

4) I think we should leave the example out of production. You only provide an example for MARC21 that will not work for UNIMARC and NORMARC users and that can't be removed from display unless you delete the file (and will come back on each update using packages).
Comment 26 Katrin Fischer 2018-10-28 09:48:54 UTC
Created attachment 81385 [details] [review]
Bug 17385: Add custom export XSLT

Patch was corrected and rebased after QA feedback. Previous patches where combined into this one.

- Empty READMEs have been deleted
- Whitespaces changes to unrelated code have been undone
* CustomXSLTExportList now uses LibXML to figure out the title and output
    * The custom export XSLT's title should be an attribute to the root node, otherwise it will default to filename
- Duplicate $xslFile param was removed from XSLTParse4Display
* Improvment to code structure in XSLTParse4Display
* Some fixes to calls to AddBiblio()

Test case:
  - Apply patch
  - Visit an item's detail page in the OPAC or the staff interface
  - Click save and you found Simple Export.
  - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
  - It work correctly in basket and lists.

The patch comes with sample XSLT files
  for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
  for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl

Followed test plan, patch works as described. Also ran QA test tools and
modified files pass

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>
https://bugs.koha-community.org/show_bug.cgi?id=17835
Comment 27 Katrin Fischer 2018-10-28 09:48:59 UTC
Created attachment 81386 [details] [review]
Bug 17385: Add tests for CustomXSLTExportList

+ Added tests for CustomXSLTExportList to C4::XSLT
+ Added an eval block in CustomXSLTExportList to handle invalid XML.

* Removed Opac parameter, but now handle "OPACXSLTCustomExport" as equivalent

+ Added "Custom" option to OPACExportOptions preference.

Tested with invalid XML (not originally on purpose, but...) and then
added a new field to add to the simple export.

Walked through the testing steps.

Question: should this also apply to the cataloging page as an export
option? It doesn't now.

Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>
Comment 28 Katrin Fischer 2018-10-28 09:49:35 UTC
comment#25!
Comment 29 Mikaël Olangcay Brisebois 2019-01-24 19:33:38 UTC
Created attachment 84388 [details] [review]
Bug 17385: Add custom export XSLT

Patch was corrected and rebased after QA feedback. Previous patches where combined into this one.

- Empty READMEs have been deleted
- Whitespaces changes to unrelated code have been undone
* CustomXSLTExportList now uses LibXML to figure out the title and output
    * The custom export XSLT's title should be an attribute to the root node, otherwise it will default to filename
- Duplicate $xslFile param was removed from XSLTParse4Display
* Improvment to code structure in XSLTParse4Display
* Some fixes to calls to AddBiblio()

Test case:
  - Apply patch
  - Visit an item's detail page in the OPAC or the staff interface
  - Click save and you found Simple Export.
  - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
  - It work correctly in basket and lists.

The patch comes with sample XSLT files
  for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
  for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl

Followed test plan, patch works as described. Also ran QA test tools and
modified files pass

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>
https://bugs.koha-community.org/show_bug.cgi?id=17835
Comment 30 Mikaël Olangcay Brisebois 2019-01-24 19:35:17 UTC
Created attachment 84389 [details] [review]
Bug 17385: Add custom export XSLT

Patch was corrected and rebased after QA feedback. Previous patches where combined into this one.

- Empty READMEs have been deleted
- Whitespaces changes to unrelated code have been undone
* CustomXSLTExportList now uses LibXML to figure out the title and output
    * The custom export XSLT's title should be an attribute to the root node, otherwise it will default to filename
- Duplicate $xslFile param was removed from XSLTParse4Display
* Improvment to code structure in XSLTParse4Display
* Some fixes to calls to AddBiblio()

Test case:
  - Apply patch
  - Visit an item's detail page in the OPAC or the staff interface
  - Click save and you found Simple Export.
  - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
  - It work correctly in basket and lists.

The patch comes with sample XSLT files
  for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
  for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl

Followed test plan, patch works as described. Also ran QA test tools and
modified files pass

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>
https://bugs.koha-community.org/show_bug.cgi?id=17835
Comment 31 Mikaël Olangcay Brisebois 2019-01-24 19:35:30 UTC
Created attachment 84390 [details] [review]
Bug 17385: Add tests for CustomXSLTExportList

+ Added tests for CustomXSLTExportList to C4::XSLT
+ Added an eval block in CustomXSLTExportList to handle invalid XML.

* Removed Opac parameter, but now handle "OPACXSLTCustomExport" as equivalent

+ Added "Custom" option to OPACExportOptions preference.

Tested with invalid XML (not originally on purpose, but...) and then
added a new field to add to the simple export.

Walked through the testing steps.

Question: should this also apply to the cataloging page as an export
option? It doesn't now.

Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>
Comment 32 Mikaël Olangcay Brisebois 2019-01-24 19:37:50 UTC
Rebased on current master.
Comment 33 Marcel de Rooy 2019-03-22 07:42:58 UTC
QA: Looking here now
Comment 34 Marcel de Rooy 2019-03-22 09:02:40 UTC
Created attachment 86875 [details] [review]
Bug 17385: Add custom export XSLT

Patch was corrected and rebased after QA feedback. Previous patches where combined into this one.

- Empty READMEs have been deleted
- Whitespaces changes to unrelated code have been undone
* CustomXSLTExportList now uses LibXML to figure out the title and output
    * The custom export XSLT's title should be an attribute to the root node, otherwise it will default to filename
- Duplicate $xslFile param was removed from XSLTParse4Display
* Improvment to code structure in XSLTParse4Display
* Some fixes to calls to AddBiblio()

Test case:
  - Apply patch
  - Visit an item's detail page in the OPAC or the staff interface
  - Click save and you found Simple Export.
  - Click Simple Export and you have a file.html, it contain informations returned by the xslt file.
  - It work correctly in basket and lists.

The patch comes with sample XSLT files
  for Intranet the xsl file is : koha-tmpl/intranet-tmpl/prog/en/xslt/biblioexport/MARC21_simple_export.xsl
  for opac the xsl file is : koha-tmpl/opac-tmpl/bootstrap/en/xslt/biblioexport/MARC21_simple_export.xsl

Followed test plan, patch works as described. Also ran QA test tools and
modified files pass

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>
https://bugs.koha-community.org/show_bug.cgi?id=17835

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2019-03-22 09:02:46 UTC
Created attachment 86876 [details] [review]
Bug 17385: Add tests for CustomXSLTExportList

+ Added tests for CustomXSLTExportList to C4::XSLT
+ Added an eval block in CustomXSLTExportList to handle invalid XML.

* Removed Opac parameter, but now handle "OPACXSLTCustomExport" as equivalent

+ Added "Custom" option to OPACExportOptions preference.

Tested with invalid XML (not originally on purpose, but...) and then
added a new field to add to the simple export.

Walked through the testing steps.

Question: should this also apply to the cataloging page as an export
option? It doesn't now.

Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2019-03-22 09:02:51 UTC
Created attachment 86877 [details] [review]
Bug 17385: (QA follow-up) Do not needlessly scan biblioexport folder

Performance: We need to return an empty list if OpacExportOptions does not contain Custom.  No need to scan the directory for custom files, if we are not using them at all.

Test plan:
Run t/db_dependent/XSLT.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2019-03-22 09:02:56 UTC
Created attachment 86878 [details] [review]
Bug 17385: (QA follow-up) Add caching

Performance: We should add caching in CustomXSLTExportList.

Test plan:
Run t/db_dependent/XSLT.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2019-03-22 09:11:27 UTC
QA Comment:
Thanks for your perseverance, guys. This took quite some time.
Since the Custom option is not checked by default, I do not see a problem pushing this in its current form. 

[1] We need to return an empty list if OpacExportOptions does not contain Custom.  No need to scan the directory for custom files, if we are not using them at all.
=> See follow-up.
[2] We should add caching in CustomXSLTExportList.
=> See second follow-up.
[3] The example xslt perhaps should be renamed (as mentioned before earlier). If you would only check for the .xsl extension, we could add a README and rename the extension of the simple export sample. Since it is pref controlled now, this may be done later imo. No blocker for me.
[4] t/XSLT.t depends on a pref, so on the database. Should imo be moved to dependent. Removing Test::DBIx::Class :) No blocker, maybe later ?
[5] I personally do not like using LibXML to parse the filename and format from the xsl file. And was just thinking about adding a small json or yaml file to do the same? No blocker, maybe later..

Passed QA
Comment 39 Marcel de Rooy 2019-03-22 09:14:22 UTC
Hmm. One small thing still..
Comment 40 Marcel de Rooy 2019-03-22 09:52:57 UTC
Created attachment 86879 [details] [review]
Bug 17385: (QA follow-up) Do not needlessly scan biblioexport folder

Performance: We need to return an empty list if OpacExportOptions does not contain Custom.  No need to scan the directory for custom files, if we are not using them at all.

Note: The test only pertains to OPAC now, since the pref should not control intranet behavior. We have no intranet counterpart. See further next follow-up.

Test plan:
Run t/db_dependent/XSLT.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2019-03-22 09:53:03 UTC
Created attachment 86880 [details] [review]
Bug 17385: (QA follow-up) Add caching

Performance: We should add caching in CustomXSLTExportList.

Note: This resolves the lack of an intranet test in the former patch too.

Test plan:
Run t/db_dependent/XSLT.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2019-03-22 09:53:39 UTC
Obviously, we needed two cache keys.