Bug 27569 - marc-framework import function doesn't accept LibreOffice csv/ods file formats
Summary: marc-framework import function doesn't accept LibreOffice csv/ods file formats
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: PC Linux
: P1 - high critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 18143
Blocks:
  Show dependency treegraph
 
Reported: 2021-01-28 19:19 UTC by braun.kamilla
Modified: 2021-03-22 04:26 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09,19.11.16


Attachments
Bug 27569: Remove non mysql code (3.87 KB, patch)
2021-02-01 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27569: Don't hide errors! (780 bytes, patch)
2021-02-01 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27569: Add tests (4.93 KB, patch)
2021-02-01 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27569: Fix CSV export for biblio frameworks (6.55 KB, patch)
2021-02-01 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27569: Add tests for ODS export (21.74 KB, patch)
2021-02-01 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27569: Change av for 100$2 (23.12 KB, patch)
2021-02-01 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27569: Fix ODS export - take empty string into account (1.97 KB, patch)
2021-02-01 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27569: Remove non mysql code (3.93 KB, patch)
2021-02-15 04:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 27569: Don't hide errors! (837 bytes, patch)
2021-02-15 04:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 27569: Add tests (5.00 KB, patch)
2021-02-15 04:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 27569: Fix CSV export for biblio frameworks (6.62 KB, patch)
2021-02-15 04:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 27569: Add tests for ODS export (19.68 KB, patch)
2021-02-15 04:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 27569: Change av for 100$2 (21.98 KB, patch)
2021-02-15 04:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 27569: Fix ODS export - take empty string into account (2.03 KB, patch)
2021-02-15 04:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 27569: Remove non mysql code (3.99 KB, patch)
2021-02-15 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27569: Don't hide errors! (895 bytes, patch)
2021-02-15 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27569: Add tests (5.06 KB, patch)
2021-02-15 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27569: Fix CSV export for biblio frameworks (6.67 KB, patch)
2021-02-15 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27569: Add tests for ODS export (19.73 KB, patch)
2021-02-15 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27569: Change av for 100$2 (22.04 KB, patch)
2021-02-15 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27569: Fix ODS export - take empty string into account (2.09 KB, patch)
2021-02-15 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description braun.kamilla 2021-01-28 19:19:20 UTC
The biblographic marc-framework import function does not except any of the ods-file format standards (i.e. 1.0/1.1; 1.2; 1.2 extended (compatibility-mode); 1.2 extended) used by LibreOffice. The csv-format stored by LibreOffice is also not usable.
This makes the marc-framework export/import functions unusable.
Comment 1 Jonathan Druart 2021-01-29 09:11:00 UTC
What error do you get?

Can you detail the different steps to recreate the problem?

I am lowering the severity, it's not a critical issue, you can still use CSV.
Comment 2 braun.kamilla 2021-01-29 11:47:47 UTC
(In reply to Jonathan Druart from comment #1)
> What error do you get?

there is no error report generated by koha, the marc-framework is empty after import, even though there is a lot of content in the ods/csv-file

> 
> Can you detail the different steps to recreate the problem?

You can recreate the problem by simply exporting an existing marc-framework to either csv- or ods-file, then opening it into LibreOffice, saving it with a different filename (still csv/ods-format), an then importing that file (without any changes made in the content) into koha.

> 
> I am lowering the severity, it's not a critical issue, you can still use CSV.

The CSV-import (saved by LibreOffice) is also not working. It suffers the same problem.
Comment 3 Jonathan Druart 2021-02-01 15:06:52 UTC
Created attachment 116164 [details] [review]
Bug 27569: Remove non mysql code

Remove a bit of the confusion by removing unless code (not related to
mysql)
Comment 4 Jonathan Druart 2021-02-01 15:06:55 UTC
Created attachment 116165 [details] [review]
Bug 27569: Don't hide errors!
Comment 5 Jonathan Druart 2021-02-01 15:06:58 UTC
Created attachment 116166 [details] [review]
Bug 27569: Add tests
Comment 6 Jonathan Druart 2021-02-01 15:07:01 UTC
Created attachment 116167 [details] [review]
Bug 27569: Fix CSV export for biblio frameworks

The CSV import of the biblio frameworks is broken.
A change in LibreOffice resulted in CSV not formatted how the
C4::ImportExportFramework code is expected.
This code is quite broken, it expects all the CSV cells to be quoted.

The "add tests" patch shows the different between what we expected
before this patch, and what is the new version generated by LibreOffice.

Test plan:
Export a biblio framework in CSV
Open it with LibreOffice and save it undef a different name
Create a new biblio framework and use the new file to construct it
Compare the 2 frameworks and make sure they are identical
Comment 7 Jonathan Druart 2021-02-01 15:07:04 UTC
Created attachment 116168 [details] [review]
Bug 27569: Add tests for ODS export

Looks ok so far
Comment 8 Jonathan Druart 2021-02-01 15:07:08 UTC
Created attachment 116169 [details] [review]
Bug 27569: Change av for 100$2

Switch from "#" to ""
Comment 9 Jonathan Druart 2021-02-01 15:07:11 UTC
Created attachment 116170 [details] [review]
Bug 27569: Fix ODS export - take empty string into account

If a cell contains an empty string it was not taken into account.

Test plan:
Same as CSV but with ODS
Comment 10 Katrin Fischer 2021-02-01 16:33:52 UTC
Hi Jonathan, 

can you add which LibreOffice version you have used for testing?

I have tested a bit and the issue seems to be that we used to export "empty cell" with # in it, but we don't seem to do that with the authorised_value and see_also columns consistently anymore? When I add back the #, the file loads appears to be loading ok.

I think making it work with empty cells would be better, but it could it have been a change in our export format causing this?

I tested with an .ods file with Excel 2016 or LibreOffice 6.2.5.2.
Comment 11 Jonathan Druart 2021-02-02 09:21:17 UTC
(In reply to Katrin Fischer from comment #10)
> Hi Jonathan, 
> 
> can you add which LibreOffice version you have used for testing?

Version: 6.1.5.2

> I have tested a bit and the issue seems to be that we used to export "empty
> cell" with # in it, but we don't seem to do that with the authorised_value
> and see_also columns consistently anymore? When I add back the #, the file
> loads appears to be loading ok.

It's actually exporting what we have in DB:

MariaDB [koha_kohadev]> select tagfield, authorised_value from marc_tag_structure where frameworkcode="" and tagfield in ("018", "020");
+----------+------------------+
| tagfield | authorised_value |
+----------+------------------+
| 018      |                  |
| 020      | NULL             |
+----------+------------------+
Comment 12 Jonathan Druart 2021-02-02 09:28:11 UTC
Ok, there was a change made by bug 18143 that introduced the export behaviour change. But it's from Feb 2017!
Comment 13 Jonathan Druart 2021-02-02 09:31:10 UTC
(In reply to Jonathan Druart from comment #12)
> Ok, there was a change made by bug 18143 that introduced the export
> behaviour change. But it's from Feb 2017!

But pushed in Feb 2019!
Comment 14 braun.kamilla 2021-02-04 08:31:55 UTC
Dear Jonathan,
Can you tell me, when the BUG-FIXED version of KOHA will be available for download. Thank you.
Best Regards
Comment 15 Katrin Fischer 2021-02-04 08:41:38 UTC
At the moment we cannot give you a version this will be released with yet. It needs to go through the QA process first.

The next step is that this needs to be tested by someone and changed to Signed Off. You can try and take on this step if you want, as this should work for testing on our sandboxes. The process is described here:
https://wiki.koha-community.org/wiki/Sandboxes

After the patch has been succesfully tested it will go to QA and someone from the QA team verifies if it meets our guidelines.

Then it can be pushed and included in the next monthly bugfx release.

As a workaroudn it might work if you replace empty cells with # to get your framework loaded.
Comment 16 ByWater Sandboxes 2021-02-15 04:14:37 UTC
Created attachment 116871 [details] [review]
Bug 27569: Remove non mysql code

Remove a bit of the confusion by removing unless code (not related to
mysql)

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Comment 17 ByWater Sandboxes 2021-02-15 04:14:39 UTC
Created attachment 116872 [details] [review]
Bug 27569: Don't hide errors!

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Comment 18 ByWater Sandboxes 2021-02-15 04:14:42 UTC
Created attachment 116873 [details] [review]
Bug 27569: Add tests

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Comment 19 ByWater Sandboxes 2021-02-15 04:14:44 UTC
Created attachment 116874 [details] [review]
Bug 27569: Fix CSV export for biblio frameworks

The CSV import of the biblio frameworks is broken.
A change in LibreOffice resulted in CSV not formatted how the
C4::ImportExportFramework code is expected.
This code is quite broken, it expects all the CSV cells to be quoted.

The "add tests" patch shows the different between what we expected
before this patch, and what is the new version generated by LibreOffice.

Test plan:
Export a biblio framework in CSV
Open it with LibreOffice and save it undef a different name
Create a new biblio framework and use the new file to construct it
Compare the 2 frameworks and make sure they are identical

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Comment 20 ByWater Sandboxes 2021-02-15 04:14:47 UTC
Created attachment 116875 [details] [review]
Bug 27569: Add tests for ODS export

Looks ok so far

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Comment 21 ByWater Sandboxes 2021-02-15 04:14:49 UTC
Created attachment 116876 [details] [review]
Bug 27569: Change av for 100$2

Switch from "#" to ""

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Comment 22 ByWater Sandboxes 2021-02-15 04:14:52 UTC
Created attachment 116877 [details] [review]
Bug 27569: Fix ODS export - take empty string into account

If a cell contains an empty string it was not taken into account.

Test plan:
Same as CSV but with ODS

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Comment 23 Ron Houk 2021-02-15 04:18:51 UTC
I went through the test plan listed in comment #6 using LibreOffice 7.0.4.2 and the new framework appeared to be identical to the framework that was exported.
Comment 24 Katrin Fischer 2021-02-15 21:01:03 UTC
Created attachment 116888 [details] [review]
Bug 27569: Remove non mysql code

Remove a bit of the confusion by removing unless code (not related to
mysql)

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2021-02-15 21:01:07 UTC
Created attachment 116889 [details] [review]
Bug 27569: Don't hide errors!

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2021-02-15 21:01:11 UTC
Created attachment 116890 [details] [review]
Bug 27569: Add tests

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2021-02-15 21:01:15 UTC
Created attachment 116891 [details] [review]
Bug 27569: Fix CSV export for biblio frameworks

The CSV import of the biblio frameworks is broken.
A change in LibreOffice resulted in CSV not formatted how the
C4::ImportExportFramework code is expected.
This code is quite broken, it expects all the CSV cells to be quoted.

The "add tests" patch shows the different between what we expected
before this patch, and what is the new version generated by LibreOffice.

Test plan:
Export a biblio framework in CSV
Open it with LibreOffice and save it undef a different name
Create a new biblio framework and use the new file to construct it
Compare the 2 frameworks and make sure they are identical

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2021-02-15 21:01:19 UTC
Created attachment 116892 [details] [review]
Bug 27569: Add tests for ODS export

Looks ok so far

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2021-02-15 21:01:23 UTC
Created attachment 116893 [details] [review]
Bug 27569: Change av for 100$2

Switch from "#" to ""

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2021-02-15 21:01:27 UTC
Created attachment 116894 [details] [review]
Bug 27569: Fix ODS export - take empty string into account

If a cell contains an empty string it was not taken into account.

Test plan:
Same as CSV but with ODS

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Jonathan Druart 2021-02-16 09:22:30 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 32 Fridolin Somers 2021-02-19 15:41:30 UTC
Pushed to 20.11.x for 20.11.03
Comment 33 Andrew Fuerste-Henry 2021-02-23 17:44:55 UTC
Pushed to 20.05.x for 20.05.09
Comment 34 Victor Grousset/tuxayo 2021-03-16 17:54:21 UTC
Without the patch, importing the CSV saved by LibreOffice crashes.

DBD::mysql::st execute failed: called with 4 bind variables when 3 are needed [for Statement "INSERT INTO marc_tag_structure (s,liblibrarian,libopac,repeatable,mandatory,authorised_value,ind1_defaultvalue,ind2_defaultvalue,frameworkcode,,,,,,,,,0,LEADER,LEADER,0,1,,,,,,,,,,,,,1,CONTROL [...]

And the ODS saved by LibreOffice only import 101 fields instead of 348

So this patch is a great fix :D
Comment 35 Victor Grousset/tuxayo 2021-03-16 17:55:03 UTC
Backported: Pushed to 19.11.x branch for 19.11.16
Comment 36 Victor Grousset/tuxayo 2021-03-16 17:55:56 UTC
When checking the functionality of the patch, other issues have been found, see bug 27968
Comment 37 Victor Grousset/tuxayo 2021-03-18 20:59:26 UTC
oops, I missed that "blocks 27968" was removed and I added it again thinking that I forgot in the first place.
Comment 38 Victor Grousset/tuxayo 2021-03-22 04:26:56 UTC
It turns out that the following SQL error has been caught by tests in 19.11

koha_1       | DBD::mysql::st execute failed: Unknown column 'important' in 'field list' [for Statement "INSERT INTO marc_tag_structure (tagfield,liblibrarian,libopac,repeatable,mandatory,important,authorised_value,ind1_defaultvalue,ind2_defaultvalue,frameworkcode) VALUES (?,?,?,?,?,?,?,?,?,?) ON DUPLICATE KEY UPDATE tagfield=?,liblibrarian=?,libopac=?,repeatable=?,mandatory=?,important=?,authorised_value=?,ind1_defaultvalue=?,ind2_defaultvalue=?,frameworkcode=?" with ParamValues: 0='000', 1='LEADER', 2='LEADER', 3='0', 4='1', 5='0', 6='', 7='', 8='', 9='h8aS', 10='000', 11='LEADER', 12='LEADER', 13='0', 14='1', 15='0', 16='', 17='', 18='', 19='h8aS'] at /kohadevbox/koha/C4/ImportExportFramework.pm line 860, <$dom> line 2.

https://jenkins.koha-community.org/job/Koha_19.11_D9/305/

Couldn't find where does it take the column "important" comes from though.

Wait, it actually comes from the test data.
t/db_dependent/data/frameworks/biblio_framework_no_quoted.csv

Add patch fixing the test data has been pushed to 19.11.x
https://git.koha-community.org/Koha-community/Koha/commit/eba92984ddf7fcc62a76bd28d32c42f92d434d84


Good thing that the previous patches made the import fail instead of silently doing something wrong when having a wrong structure to import.