Bug 27859

When writing tests for this patch also discovered and fixed a bug in_array_to_marc($data) where $data is unintentionally modified by replacing the value of hash-ref in that strucure. $data is mutated when it should not which becomes apparent if calling _array_to_marc with the same argument multiple times.

Rebased against master.

Sorry David, patch needs rebase :/

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 27859: marc search result export
Using index info to reconstruct a base tree...
M	Koha/SearchEngine/Elasticsearch/Search.pm
M	catalogue/search.pl
M	installer/data/mysql/mandatory/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging catalogue/search.pl
Auto-merging Koha/SearchEngine/Elasticsearch/Search.pm
CONFLICT (content): Merge conflict in Koha/SearchEngine/Elasticsearch/Search.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 27859: marc search result export
hint: Use 'git am --show-current-patch=diff' to see the failed patch

Ok! Rebased against master.

Hi David, 

the QA test tools turn up some issues with this patch, can you please fix?

1) QA test tools

 FAIL	Koha/SearchEngine/Elasticsearch.pm
   FAIL	  spelling
		 prefered ==> preferred

 OK	Koha/SearchEngine/Elasticsearch/Search.pm
 FAIL	catalogue/search.pl
   FAIL	  critic
		# ControlStructures::ProhibitMutatingListFunctions: Got 1 violation(s).  
		# Variables::ProhibitUnusedVariables: Got 1 violation(s).  

 OK	installer/data/mysql/atomicupdate/bug_27859-add_enable_search_result_marc_export_sysprefs.pl
 OK	installer/data/mysql/mandatory/sysprefs.sql
 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
   FAIL	  filters
		wrong_html_filter at line 325 (                                            <li><a href="/cgi-bin/koha/catalogue/search.pl?count=[% results_per_page | uri %]&export=1&export_format=[% id %][% PROCESS sort_search_query %]">[% custom_export_formats.$id.name | html %]</a></li>)
		missing_filter at line 325 (                                            <li><a href="/cgi-bin/koha/catalogue/search.pl?count=[% results_per_page | uri %]&export=1&export_format=[% id %][% PROCESS sort_search_query %]">[% custom_export_formats.$id.name | html %]</a></li>)
		missing_filter at line 360 (                    <div class="dialog message">[% export_message %]</div>)
		missing_filter at line 363 (                    <div class="dialog error">[% export_error %]</div>)

 FAIL	t/db_dependent/Koha/SearchEngine/Elasticsearch.t
   FAIL	  spelling
		 indentical ==> identical
		 indentical ==> identical


2) Translatability

This will not be translatable and should be moved to templates, also the other places where export_links_html is set. HTML should not be generated in .pl files:

+            $export_links_html = $links_count > 1 ?
+            "<p>Some records exceeded the maximum size supported by ISO2709 and was exported as MARCXML instead.</p>" . $export_links_html : $export_links_html;

Also here:
$message = "<p>The export finished successfully:</p>"


3) System preferences

I am a little in doubt about SearchResultMARCExportFromAddress. The from address can not always be set to be a library specific email, especially for installations hosted centrally by a support provider. We usually tend to use the reply-to for individual settings.

But it should for sure be type = email in the .prefs file (activates validation).

Thanks for the review! Issues should now be fixed. Now using background job when performing the export so removed the feature for sending results in email.

Just one item from testing. 

When I click on the "View detail of the enqueued job" (link is http://127.0.0.1:8081/cgi-bin/koha/admin/background_jobs.pl?op=view&id=1) I get an error trace page:

Template process failed: file error - background_jobs/search_result_marc_export.inc: not found at /kohadevbox/koha/C4/Templates.pm line 127

    in C4::Templates::output at /kohadevbox/koha/C4/Templates.pm line 127

      124: 
      125:     my $data;
      126:     binmode( STDOUT, ":encoding(UTF-8)" );
      127:     $template->process( $self->filename, $vars, \$data )
      128:       || die "Template process failed: ", $template->error();
      129:     return $data;
      130: }

....

I get the same error if I try to view the job details from Administration > Jobs > Manage jobs

(In reply to David Nind from comment #19)
> Just one item from testing. 
> 
> When I click on the "View detail of the enqueued job" (link is
> http://127.0.0.1:8081/cgi-bin/koha/admin/background_jobs.pl?op=view&id=1) I
> get an error trace page:
> 
> Template process failed: file error -
> background_jobs/search_result_marc_export.inc: not found at
> /kohadevbox/koha/C4/Templates.pm line 127
> 
>     in C4::Templates::output at /kohadevbox/koha/C4/Templates.pm line 127
> 
>       124: 
>       125:     my $data;
>       126:     binmode( STDOUT, ":encoding(UTF-8)" );
>       127:     $template->process( $self->filename, $vars, \$data )
>       128:       || die "Template process failed: ", $template->error();
>       129:     return $data;
>       130: }
> 
> ....
> 
> I get the same error if I try to view the job details from Administration >
> Jobs > Manage jobs

Ok! Forgot to include that file in the commit, have now fixed this.

(In reply to David Gustafsson from comment #21)
> Ok! Forgot to include that file in the commit, have now fixed this.

Thanks David! Now signed off.

Tested with ES7.

Testing...

1) QA test tools

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/search_result_marc_export.inc
   FAIL	  filters
		missing_filter at line 14 (                <li>[% format %]: <a href="[% report.export_links.$format | $raw %]">[% report.export_links.$format | html %]</a></li

2) Database update

It would be better to use the new template for system preferences, but the output should include the names of the new preferences.

3) System preference definitions

a) This needs to be 0/1 instead of yes/no:
+            - pref: EnableSearchResultMARCExport
+              type: boolean
+              default: yes
+              choices:
+                  yes: Enable
+                  no: Disable


b) This doesn't read quite right:
"A format have the required properties \"<strong>name</strong>\", \"<strong>fields</strong>\" and an optional \"<strong>multiple</strong>\".<br />"

c) SearchResultMARCExportCustomFormats
It says MARC export... but do we actually export MARC? Maybe a note where to find the filed names would help here. From the name, I had expected MARC fields.

d) ElasticSearch should be Elasticsearch

e) As this appears to be an Elasticserach only feature, all relating prefs should say so. Also you might want to add some links from one pref to the others involved.

I got to stop here now since Elasticsearch doesn't want to cooperate in my koha-testing-docker environment right now :(

Created attachment 147936 [details] [review]
Bug 27859: marc search result export

Enable export of staff interface search results in different marc formats.
This feature is only supported when using Elasticsearch.

To test:
1) Apply patch
2) Run installer/data/mysql/updatedatabase.pl
3) Make sure the syspref EnableSearchResultMARCExport is enabled
4) Make sure the current user has the tools -> export_catalog permission
5) Perform a search
6) Export the serach result by choosing a format under the
   "Export results" drop down
7) Verify that link(s) with exported data appear when export is
   completed
8) Revoke the permission in 3) and ensure exporting is no longer
   possible
9) Run tests in t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Sponsored-by: Gothenburg University Library

(In reply to Katrin Fischer from comment #24)
> Testing...
> 
> 1) QA test tools
> 
>  FAIL
> koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/
> search_result_marc_export.inc
>    FAIL	  filters
> 		missing_filter at line 14 (                <li>[% format %]: <a href="[%
> report.export_links.$format | $raw %]">[% report.export_links.$format | html
> %]</a></li
> 
> 2) Database update
> 
> It would be better to use the new template for system preferences, but the
> output should include the names of the new preferences.
> 
> 3) System preference definitions
> 
> a) This needs to be 0/1 instead of yes/no:
> +            - pref: EnableSearchResultMARCExport
> +              type: boolean
> +              default: yes
> +              choices:
> +                  yes: Enable
> +                  no: Disable
> 
> 
> b) This doesn't read quite right:
> "A format have the required properties \"<strong>name</strong>\",
> \"<strong>fields</strong>\" and an optional
> \"<strong>multiple</strong>\".<br />"
> 
> c) SearchResultMARCExportCustomFormats
> It says MARC export... but do we actually export MARC? Maybe a note where to
> find the filed names would help here. From the name, I had expected MARC
> fields.
> 
> d) ElasticSearch should be Elasticsearch
> 
> e) As this appears to be an Elasticserach only feature, all relating prefs
> should say so. Also you might want to add some links from one pref to the
> others involved.
> 
> I got to stop here now since Elasticsearch doesn't want to cooperate in my
> koha-testing-docker environment right now :(

Thanks for the feedback! The issues should now be fixed.

Rebased against master.

David, unfortunately your tests no longer apply cleanly. ( t/db_dependent/Koha/SearchEngine/Elasticsearch.t ) It looks like a simple fix where you just need to adjust the number of tests. 

QA Tool:
 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
   FAIL	  valid_template
		str/job_progess.inc: not found


I don't see an include in Koha called this. 

Also, don't forget to tidy!

Created attachment 162342 [details] [review]
Bug 27859: marc search result export

Enable export of staff interface search results in different marc formats.
This feature is only supported when using Elasticsearch.

To test:
1) Apply patch
2) Run installer/data/mysql/updatedatabase.pl
3) Make sure the syspref EnableSearchResultMARCExport is enabled
4) Make sure the current user has the tools -> export_catalog permission
5) Perform a search
6) Export the serach result by choosing a format under the
   "Export results" drop down
7) Verify that link(s) with exported data appear when export is
   completed
8) Revoke the permission in 3) and ensure exporting is no longer
   possible
9) Run tests in t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Sponsored-by: Gothenburg University Library

The job_progress-files where misspelled in previous version of Koha, that's why the includes now fail and this is now fixed. The qa script complains about POD coverage being greater before for Koha/BackgroundJob/SearchResultExport.pm, but don't really understand why. It's the same as the other background job classes, and a coverage of 1 should be fine?

Created attachment 162343 [details] [review]
Bug 27859: marc search result export

Enable export of staff interface search results in different marc formats.
This feature is only supported when using Elasticsearch.

To test:
1) Apply patch
2) Run installer/data/mysql/updatedatabase.pl
3) Make sure the syspref EnableSearchResultMARCExport is enabled
4) Make sure the current user has the tools -> export_catalog permission
5) Perform a search
6) Export the serach result by choosing a format under the
   "Export results" drop down
7) Verify that link(s) with exported data appear when export is
   completed
8) Revoke the permission in 3) and ensure exporting is no longer
   possible
9) Run tests in t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Sponsored-by: Gothenburg University Library

Created attachment 163761 [details] [review]
Bug 27859: Export staff interface search results when using Elasticsearch

Enable export of staff interface search results in different marc formats.
This feature is only supported when using Elasticsearch.

To test:
1) Apply patch
2) Run installer/data/mysql/updatedatabase.pl
3) Make sure the EnableElasticsearchSearchResultExport system
   preference is enabled
4) Make sure the current user has the export_catalog permission
   (Use all tools > Export bibliographic and holdings data)
5) Perform a search
6) Export the serach result by choosing a format under the
   "Export all results" drop down
7) Verify that link(s) with exported data appear on the
   background job detail page when the export is completed
8) Revoke the permission in step 4 and ensure exporting
   is no longer possible
9) Run tests in t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Sponsored-by: Gothenburg University Library
Signed-off-by: David Nind <david@davidnind.com>

Created attachment 163762 [details] [review]
Bug 27859: (follow-up) Update system preference descriptions

Format and edit the descriptions for the new
EnableElasticsearchSearchResultExport and
ElasticsearchSearchResultExportCustomFormats system
preferences.

Signed-off-by: David Nind <david@davidnind.com>

I have:
1. Amended the title and test plan for the patch (to update to the new system preference name)
2. Added a follow-up with some edits to the system preference descriptions.

Testing notes using KTD:
1. Start up KTD using Elasticsearch: ktd --es8 up
2. The new system preference EnableElasticsearchSearchResultExport is enabled by default.
3. Test exporting using custom export formats:
   . Test the example custom export formats in the ElasticsearchSearchResultExportCustomFormats system preference description