Bug 15451 - Move the CSV related code to Koha::CsvProfile[s]
Summary: Move the CSV related code to Koha::CsvProfile[s]
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 15446
Blocks: 15449 18312 15475 15498
  Show dependency treegraph
 
Reported: 2015-12-31 10:04 UTC by Jonathan Druart
Modified: 2017-04-23 23:03 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 15451: Add the 2 new modules Koha::CsvProfile[s] (5.31 KB, patch)
2015-12-31 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Rewrite the csv profile tool script (39.14 KB, patch)
2015-12-31 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfilesLoop (6.63 KB, patch)
2015-12-31 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfiles (4.10 KB, patch)
2015-12-31 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfileId (5.21 KB, patch)
2015-12-31 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfile (4.12 KB, patch)
2015-12-31 11:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove the residue (7.62 KB, patch)
2015-12-31 11:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Do not considered a Resultset as a Result (3.24 KB, patch)
2016-01-06 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Add the 2 new modules Koha::CsvProfile[s] (5.31 KB, patch)
2016-04-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Rewrite the csv profile tool script (39.14 KB, patch)
2016-04-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfilesLoop (6.63 KB, patch)
2016-04-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfiles (4.09 KB, patch)
2016-04-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfileId (5.21 KB, patch)
2016-04-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfile (4.12 KB, patch)
2016-04-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove the residue (7.57 KB, patch)
2016-04-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Do not considered a Resultset as a Result (3.24 KB, patch)
2016-04-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15451: Add the 2 new modules Koha::CsvProfile[s] (5.41 KB, patch)
2016-04-28 02:16 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15451: Rewrite the csv profile tool script (39.26 KB, patch)
2016-04-28 02:16 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15451: Koha::CsvProfiles - Remove GetCsvProfilesLoop (6.81 KB, patch)
2016-04-28 02:16 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15451: Koha::CsvProfiles - Remove GetCsvProfiles (4.20 KB, patch)
2016-04-28 02:16 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15451: Koha::CsvProfiles - Remove GetCsvProfileId (5.35 KB, patch)
2016-04-28 02:16 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15451: Koha::CsvProfiles - Remove GetCsvProfile (4.20 KB, patch)
2016-04-28 02:16 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15451: Koha::CsvProfiles - Remove the residue (7.74 KB, patch)
2016-04-28 02:16 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15451: Do not considered a Resultset as a Result (3.41 KB, patch)
2016-04-28 02:17 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15451: (followup) fix filename extension for csv file (1.58 KB, patch)
2016-04-28 02:21 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15451: (followup) fix filename extension for csv file (1.65 KB, patch)
2016-04-29 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15451: Add the 2 new modules Koha::CsvProfile[s] (5.49 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: Rewrite the csv profile tool script (39.35 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfilesLoop (6.91 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfiles (4.30 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfileId (5.44 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfile (4.29 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: Koha::CsvProfiles - Remove the residue (7.85 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: Do not considered a Resultset as a Result (3.50 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: (followup) fix filename extension for csv file (1.75 KB, patch)
2016-07-22 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15451: Better error handling (2.94 KB, patch)
2016-07-22 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-12-31 10:04:30 UTC
The code from C4::Csv can easily be replaced by an OO modules in the Koha namespace, using Koha::Object.
On the way, the tool script will be rewritten to use this module.
Comment 1 Jonathan Druart 2015-12-31 11:07:47 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-12-31 11:07:50 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-12-31 11:07:52 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-12-31 11:07:55 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-12-31 11:07:58 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-12-31 11:08:00 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-12-31 11:08:03 UTC Comment hidden (obsolete)
Comment 8 Bernardo Gonzalez Kriegel 2016-01-04 23:50:26 UTC
Hi Jonathan and Happy new year :)

With all patches applied Tools > CSVprofiles fails with

Software error:
DBIx::Class::Schema::resultset(): resultset() expects a source name at /home/bgkriegel/kohaclone/Koha/Objects.pm line 237

Test fails with similar error.
Comment 9 Jonathan Druart 2016-01-05 07:12:44 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #8)
> Hi Jonathan and Happy new year :)
> 
> With all patches applied Tools > CSVprofiles fails with
> 
> Software error:
> DBIx::Class::Schema::resultset(): resultset() expects a source name at
> /home/bgkriegel/kohaclone/Koha/Objects.pm line 237
> 
> Test fails with similar error.

Thanks for testing Bernardo, happy new year too ;)
You should apply bug 15446 before.
Comment 10 Bernardo Gonzalez Kriegel 2016-01-05 14:12:10 UTC
(In reply to Jonathan Druart from comment #9)
> You should apply bug 15446 before.
Done, now works more.

I'm testing in order, all patches applied. First 3 Ok.
When testing 4th, /cgi-bin/koha/tools/export.pl gives the same/similar error of comment #8,

Software error:

DBIx::Class::Schema::resultset(): resultset() expects a source name at /home/bgkriegel/kohaclone/Koha/Objects.pm line 235
Comment 11 Jonathan Druart 2016-01-05 14:41:11 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #10)
> I'm testing in order, all patches applied. First 3 Ok.

I add a test plan on each patch to explain what I do, but to test them, you have to apply the entire patch set (well, in a dream world you should not, but I am not perfect :p).
Comment 12 Bernardo Gonzalez Kriegel 2016-01-05 14:59:57 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Bernardo Gonzalez Kriegel from comment #10)
> > I'm testing in order, all patches applied. First 3 Ok.
> 
> I add a test plan on each patch to explain what I do, but to test them, you
> have to apply the entire patch set (well, in a dream world you should not,
> but I am not perfect :p).

Hehe :)
I applied all patches, and follow each test plan before sign.

But on current master (e2a3b93860740fd498baad35fc8a4dc859527cb9), on top of 15446
$ git status
On branch bug_15451
Your branch is ahead of 'origin/master' by 10 commits

Go to Tools > Export data and got the error described on comment #10
Comment 13 Jonathan Druart 2016-01-05 15:28:08 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #12)

Sorry Bernardo, I tested on my local branch, which was not updated.
I have attached new patch on bug 15446, some occurrences of 'sub type' have been pushed in the meanwhile.
Comment 14 Bernardo Gonzalez Kriegel 2016-01-05 16:02:27 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Bernardo Gonzalez Kriegel from comment #12)
> 
> Sorry Bernardo, I tested on my local branch, which was not updated.
> I have attached new patch on bug 15446, some occurrences of 'sub type' have
> been pushed in the meanwhile.

With all patches from 15446 and this, exporting a range of biblios to marc/xml/cvs, with/without ExportWithCsvProfile

Inside the file (marc/xml/csv)

<h1>Software error:</h1>
<pre>Can't locate object method &quot;export_format_id&quot; via package &quot;Koha::CsvProfiles&quot; at /home/bgkriegel/kohaclone/tools/export.pl line 196.
</pre>

On logs
export.pl: Can't locate object method "export_format_id" via package "Koha::CsvProfiles"
Comment 15 Jonathan Druart 2016-01-06 09:00:26 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-01-06 09:03:10 UTC
(In reply to Jonathan Druart from comment #15)
> Created attachment 46306 [details] [review] [review]
> Bug 15451: Do not considered a Resultset as a Result
> 
> The previous calls were wrong, but there is something bad with the DB
> structure: export_format.profile should be a unique key.
> This patch fixes the previous calls and add a FIXME not to forget to fix
> the DB structure.

I have opened bug 15475.
Comment 17 Jonathan Druart 2016-04-13 08:37:12 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-04-13 08:37:16 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-04-13 08:37:19 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-04-13 08:37:22 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2016-04-13 08:37:25 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2016-04-13 08:37:29 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-04-13 08:37:32 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2016-04-13 08:37:35 UTC Comment hidden (obsolete)
Comment 25 Bernardo Gonzalez Kriegel 2016-04-28 02:16:01 UTC Comment hidden (obsolete)
Comment 26 Bernardo Gonzalez Kriegel 2016-04-28 02:16:10 UTC Comment hidden (obsolete)
Comment 27 Bernardo Gonzalez Kriegel 2016-04-28 02:16:17 UTC Comment hidden (obsolete)
Comment 28 Bernardo Gonzalez Kriegel 2016-04-28 02:16:25 UTC Comment hidden (obsolete)
Comment 29 Bernardo Gonzalez Kriegel 2016-04-28 02:16:35 UTC Comment hidden (obsolete)
Comment 30 Bernardo Gonzalez Kriegel 2016-04-28 02:16:46 UTC Comment hidden (obsolete)
Comment 31 Bernardo Gonzalez Kriegel 2016-04-28 02:16:55 UTC Comment hidden (obsolete)
Comment 32 Bernardo Gonzalez Kriegel 2016-04-28 02:17:06 UTC Comment hidden (obsolete)
Comment 33 Bernardo Gonzalez Kriegel 2016-04-28 02:21:10 UTC Comment hidden (obsolete)
Comment 34 Bernardo Gonzalez Kriegel 2016-04-28 02:23:33 UTC
Hi Jonathan,
all is signed but I found a little problem, added a possible solution.
Comment 35 Jonathan Druart 2016-04-29 12:41:22 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2016-04-29 12:41:57 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #34)
> Hi Jonathan,
> all is signed but I found a little problem, added a possible solution.

Good catch, thanks!
Comment 37 Marcel de Rooy 2016-07-22 06:22:49 UTC
QA: Looking here now..
Comment 38 Marcel de Rooy 2016-07-22 07:26:15 UTC
Created attachment 53623 [details] [review]
Bug 15451: Add the 2 new modules Koha::CsvProfile[s]

There are based on Koha::Objets. Tests provided.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass, no errors.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2016-07-22 07:26:19 UTC
Created attachment 53624 [details] [review]
Bug 15451: Rewrite the csv profile tool script

This page was inconsistent with the other admin and tool pages.
The 2 tabs 'New profile' and 'Edit existing profile' were useless, the
ergonomic needs to be revisited.
This patch applies the same script/page structure as others: by default
a table containing all csv profiles is displayed with 2 action links:
edit and delete.

Test plan:
1/ Create 1+ CSV profiles, with different types (marc and sql)
2/ Update some values using the Edit link
3/ Delete a CSV profile

Note: When deleting a CSV profile, it would be great to warn the user if it is
used.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
No problems on create, update and delete.
No errors

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2016-07-22 07:26:23 UTC
Created attachment 53625 [details] [review]
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfilesLoop

This subroutine returned the csv profiles for a given type.
This could be done easily with the new Koha::CsvProfiles->search method.

Test plan:
To do at the OPAC and staff interface!
1/ Export a list using a CSV profile
2/ Export your CART using a CSV profile
Note that only MARC profiles should be available.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested on staff/opac & cart/list
Small problem on filename extension fixed in followup.
No errors

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2016-07-22 07:26:27 UTC
Created attachment 53626 [details] [review]
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfiles

This subroutine did the same job as GetCsvProfilesLoop, so this patch
applies the same changes as the previous patch.

Test plan:
1/ Claim some serials, sql profiles should be listed
2/ Export records using the export tool. MARC profiles should be listed.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Listed sql & marc profiles
No errors

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2016-07-22 07:26:31 UTC
Created attachment 53627 [details] [review]
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfileId

This subroutine returned the export_format_id for a given profile name.
This can be done easily with the Koha::CsvProfiles->search method.

Test plan:
Export records using the misc/export_records.pl script and the
export tool.
If you are exporting using the MARC format, the profile filled in the pref
ExportWithCsvProfile will be used (or the one passed in parameter of
misc/export_records.pl).
If you are exporting using the CSV format, you can choose a profile in
the dropdown list.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Exported using tool & cmd, marc & csv. Pref is used.
No errors

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2016-07-22 07:26:34 UTC
Created attachment 53628 [details] [review]
Bug 15451: Koha::CsvProfiles - Remove GetCsvProfile

This subroutine just returned a csv profile for a given id.
It is replaced in this patch by a call to Koha::CsvProfiles->find.

There is nothing to test here, these changes have been tested in
previous patches.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2016-07-22 07:26:38 UTC
Created attachment 53629 [details] [review]
Bug 15451: Koha::CsvProfiles - Remove the residue

This patch erase all traces of C4::Csv since it's not used anymore.
All occurrences have been replaced by previous patches to use
Koha::CsvProfiles.

Note that GetMarcFieldsForCsv was not used prior this patch set.

Test plan:
  git grep 'C4::Csv'
should not return any result.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
No more traces of the file.
This produces a koha-qa fail, due to the missing file.
No other errors

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2016-07-22 07:26:42 UTC
Created attachment 53630 [details] [review]
Bug 15451: Do not considered a Resultset as a Result

The previous calls were wrong, but there is something bad with the DB
structure: export_format.profile should be a unique key.
This patch fixes the previous calls and add a FIXME not to forget to fix
the DB structure.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Previous test where done with all patches applied,
including this one, and all worked.
No errors

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2016-07-22 07:26:46 UTC
Created attachment 53631 [details] [review]
Bug 15451: (followup) fix filename extension for csv file

This patch does the same as basket/downloadcart.pl
to set '.csv' as filename extension for downloadshelf.pl

To test:
1) Define a CSV MARC profile
2) On staff download a list, extension is '.NN'
with 'NN' the CSV profile id.
3) Apply the patch
4) Download again, check extension is now '.csv'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2016-07-22 07:28:58 UTC
QA Comment (by patch number):

0002
Delete CSV Profile "Test01?"
Yes, delete
No, do not Delete
I would suggest to change this into Yes and No. This looks weird. 

0005
If you provide a wrong csv id to export_records.pl, it will generate this error:
Can't call method "encoding" on an undefined value at /usr/share/koha/masterclone/C4/Record.pm line 481.
This is more or less outside the scope of this report.
As a side note, I think it is kind of strange to pass record id's to a script. I would have expected the profile name here.
At least the validity of this csv argument could be tested somewhere.
If I choose the format csv, I think it is wrong to output to a file called koha.mrc. (Yes, I did not add the filename parameter. Should be obvious here.) 

0006
my $profile = Koha::CsvProfiles->find($id);
This is actually the line that I refer to in my former comment. 
What if this find does not have any results? You cannot assume that you will find a record here. You should test.
Not a blocker, but please provide a follow-up.

Passed QA
Comment 48 Jonathan Druart 2016-07-22 09:28:28 UTC
Created attachment 53641 [details] [review]
Bug 15451: Better error handling

1/ If a librarian edit (add_validate) a non-existing csv profile, we
explicitely die
2/ If you try to delete a non-existing csv profile, you will now get a
nice alert box
Comment 49 Kyle M Hall 2016-07-22 17:22:33 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 50 Mason James 2017-04-23 23:02:51 UTC
Enhancement, skipping for 16.05.x