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.
Created attachment 46108 [details] [review] Bug 15451: Add the 2 new modules Koha::CsvProfile[s] There are based on Koha::Objets. Tests provided.
Created attachment 46109 [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.
Created attachment 46110 [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.
Created attachment 46111 [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.
Created attachment 46112 [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.
Created attachment 46113 [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.
Created attachment 46114 [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.
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.
(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.
(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
(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).
(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
(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.
(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 "export_format_id" via package "Koha::CsvProfiles" 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"
Created attachment 46306 [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.
(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.
Created attachment 50178 [details] [review] Bug 15451: Add the 2 new modules Koha::CsvProfile[s] There are based on Koha::Objets. Tests provided.
Created attachment 50179 [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.
Created attachment 50180 [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.
Created attachment 50181 [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.
Created attachment 50182 [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.
Created attachment 50183 [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.
Created attachment 50184 [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.
Created attachment 50185 [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.
Created attachment 50860 [details] [review] [SIGNED-OFF] 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.
Created attachment 50861 [details] [review] [SIGNED-OFF] 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
Created attachment 50862 [details] [review] [SIGNED-OFF] 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
Created attachment 50863 [details] [review] [SIGNED-OFF] 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
Created attachment 50864 [details] [review] [SIGNED-OFF] 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
Created attachment 50865 [details] [review] [SIGNED-OFF] 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>
Created attachment 50866 [details] [review] [SIGNED-OFF] 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
Created attachment 50867 [details] [review] [SIGNED-OFF] 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
Created attachment 50868 [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'
Hi Jonathan, all is signed but I found a little problem, added a possible solution.
Created attachment 50985 [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>
(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!
QA: Looking here now..
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>
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>
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>
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>
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>
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>
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>
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>
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>
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
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
Pushed to master for 16.11, thanks Jonathan!
Enhancement, skipping for 16.05.x