The way the export options are displayed at the bottom of the checkouts table is not consistent. At the moment (prior to this patch set), they are display if ExportRemoveFields or ExportWithCsvProfile is set. It does not make any sense, the user could want to export the checkouts in iso2709 format without having to define a csv profile and fill the pref. Moreover the behavior of this pref does not match its description: it's used as a default CSV profile when exporting records from the export tools or the command line.
Created attachment 46338 [details] [review] Bug 15498: Let the user choose the CSV profile to export circ history The way the export options are displayed at the bottom of the checkouts table was not consistent. Prior to this patch set, they are display if ExportRemoveFields or ExportWithCsvProfile is set. It does not make any sense, the user could want to export the checkouts in iso2709 format without having to define a csv profile and fill the pref. Moreover the behavior of this pref did not match its description: it's used as a default CSV profile when exporting records from the export tools or the command line. This patch set adds a new pref ExportCircHistory and remove ExportWithCsvProfile. The new pref is set if ExportWithCsvProfile or ExportRemoveFields were set. A new dropdown list with the CSV profile list will be displayed in the export area, at the bottom of the checkouts table. Note that now --csv_profile_id is mandatory for the export command line (misc/export_records.pl) if the export format is csv. Test plan: 0/ Do not execute the DB entry 1/ Clear both ExportWithCsvProfile and ExportRemoveFields prefs 2/ Execute the DB entry 3/ ExportCircHistory should not be set and the export options should not be displayed at the bottom of the checkouts table. 4/ Remove the pref DELETE FROM systempreferences WHERE variable='ExportCircHistory'; and reinsert the previous one, with a value: INSERT INTO systempreferences (variable, value) VALUES ('ExportWithCsvProfile', 'something'); Execute the DB entry again => The now pref should be now set 5/ Export some checkouts using the CSV entry 6/ Note that the export tool and commandline script still work using the csv format. You have to provide a --csv_profile_id option to make it work.
Created attachment 46339 [details] [review] Bug 15498: Add ExportCircHistory and remove ExportWithCsvProfile
>Wende an: Bug 15498: Let the user choose the CSV profile to export circ history >fatal: sha1 information is lacking or useless (Koha/Exporter/Record.pm). >Dem Repository fehlen notwendige Blobs um auf einen 3-Wege-Merge zurückzufallen. >Kann nicht zu 3-Wege-Merge zurückfallen. >Anwendung des Patches fehlgeschlagen bei 0001 Bug 15498: Let the user choose the CSV profile to export circ history >Die Kopie des fehlgeschlagenen Patches befindet sich in: > /home/mirko/koha/.git/rebase-apply/patch
Created attachment 50257 [details] [review] Bug 15498: Let the user choose the CSV profile to export circ history The way the export options are displayed at the bottom of the checkouts table was not consistent. Prior to this patch set, they are display if ExportRemoveFields or ExportWithCsvProfile is set. It does not make any sense, the user could want to export the checkouts in iso2709 format without having to define a csv profile and fill the pref. Moreover the behavior of this pref did not match its description: it's used as a default CSV profile when exporting records from the export tools or the command line. This patch set adds a new pref ExportCircHistory and remove ExportWithCsvProfile. The new pref is set if ExportWithCsvProfile or ExportRemoveFields were set. A new dropdown list with the CSV profile list will be displayed in the export area, at the bottom of the checkouts table. Note that now --csv_profile_id is mandatory for the export command line (misc/export_records.pl) if the export format is csv. Test plan: 0/ Do not execute the DB entry 1/ Clear both ExportWithCsvProfile and ExportRemoveFields prefs 2/ Execute the DB entry 3/ ExportCircHistory should not be set and the export options should not be displayed at the bottom of the checkouts table. 4/ Remove the pref DELETE FROM systempreferences WHERE variable='ExportCircHistory'; and reinsert the previous one, with a value: INSERT INTO systempreferences (variable, value) VALUES ('ExportWithCsvProfile', 'something'); Execute the DB entry again => The now pref should be now set 5/ Export some checkouts using the CSV entry 6/ Note that the export tool and commandline script still work using the csv format. You have to provide a --csv_profile_id option to make it work.
Created attachment 50258 [details] [review] Bug 15498: Add ExportCircHistory and remove ExportWithCsvProfile
I'd like to sign-off, but patch doesn't apṕlied cleanly... Also, I like the ability to select from all available profiles, but I think it should offer only 'marc' type of profile, because with 'sql' type exporting of checkouts doesn't work...
Created attachment 57064 [details] [review] Bug 15498: Let the user choose the CSV profile to export circ history The way the export options are displayed at the bottom of the checkouts table was not consistent. Prior to this patch set, they are display if ExportRemoveFields or ExportWithCsvProfile is set. It does not make any sense, the user could want to export the checkouts in iso2709 format without having to define a csv profile and fill the pref. Moreover the behavior of this pref did not match its description: it's used as a default CSV profile when exporting records from the export tools or the command line. This patch set adds a new pref ExportCircHistory and remove ExportWithCsvProfile. The new pref is set if ExportWithCsvProfile or ExportRemoveFields were set. A new dropdown list with the CSV profile list will be displayed in the export area, at the bottom of the checkouts table. Note that now --csv_profile_id is mandatory for the export command line (misc/export_records.pl) if the export format is csv. Test plan: 0/ Do not execute the DB entry 1/ Clear both ExportWithCsvProfile and ExportRemoveFields prefs 2/ Execute the DB entry 3/ ExportCircHistory should not be set and the export options should not be displayed at the bottom of the checkouts table. 4/ Remove the pref DELETE FROM systempreferences WHERE variable='ExportCircHistory'; and reinsert the previous one, with a value: INSERT INTO systempreferences (variable, value) VALUES ('ExportWithCsvProfile', 'something'); Execute the DB entry again => The now pref should be now set 5/ Export some checkouts using the CSV entry 6/ Note that the export tool and commandline script still work using the csv format. You have to provide a --csv_profile_id option to make it work.
Created attachment 57065 [details] [review] Bug 15498: Add ExportCircHistory and remove ExportWithCsvProfile
Created attachment 57066 [details] [review] Bug 15498: Do not display sql csv profiles
(In reply to Josef Moravec from comment #6) > I'd like to sign-off, but patch doesn't apṕlied cleanly... > > Also, I like the ability to select from all available profiles, but I think > it should offer only 'marc' type of profile, because with 'sql' type > exporting of checkouts doesn't work... Indeed, done. Thanks for testing Josef!
Jonathan, everything works perfectly, I have just small doubt about UI - the purpose of 2nd select is not 100% obvious from my point of view... could you try to Merge these to select into one like this? - ISO2709 with items - ISO2709 without - CSV - profile1 - CSV - profile2 - ... OR Show/hide the second select dynamically based on actually selected format in first select?
Created attachment 57070 [details] [review] Bug 15498: Fix show/hide csv profile list
(In reply to Josef Moravec from comment #11) > Show/hide the second select dynamically based on actually selected format in > first select? Yes, that's how it is supposed to work, but I broke it during the rebase.
Created attachment 57072 [details] [review] [SIGNED-OFF] Bug 15498: Let the user choose the CSV profile to export circ history The way the export options are displayed at the bottom of the checkouts table was not consistent. Prior to this patch set, they are display if ExportRemoveFields or ExportWithCsvProfile is set. It does not make any sense, the user could want to export the checkouts in iso2709 format without having to define a csv profile and fill the pref. Moreover the behavior of this pref did not match its description: it's used as a default CSV profile when exporting records from the export tools or the command line. This patch set adds a new pref ExportCircHistory and remove ExportWithCsvProfile. The new pref is set if ExportWithCsvProfile or ExportRemoveFields were set. A new dropdown list with the CSV profile list will be displayed in the export area, at the bottom of the checkouts table. Note that now --csv_profile_id is mandatory for the export command line (misc/export_records.pl) if the export format is csv. Test plan: 0/ Do not execute the DB entry 1/ Clear both ExportWithCsvProfile and ExportRemoveFields prefs 2/ Execute the DB entry 3/ ExportCircHistory should not be set and the export options should not be displayed at the bottom of the checkouts table. 4/ Remove the pref DELETE FROM systempreferences WHERE variable='ExportCircHistory'; and reinsert the previous one, with a value: INSERT INTO systempreferences (variable, value) VALUES ('ExportWithCsvProfile', 'something'); Execute the DB entry again => The now pref should be now set 5/ Export some checkouts using the CSV entry 6/ Note that the export tool and commandline script still work using the csv format. You have to provide a --csv_profile_id option to make it work. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57073 [details] [review] [SIGNED-OFF] Bug 15498: Add ExportCircHistory and remove ExportWithCsvProfile Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57074 [details] [review] [SIGNED-OFF] Bug 15498: Do not display sql csv profiles Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57075 [details] [review] [SIGNED-OFF] Bug 15498: Fix show/hide csv profile list Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
QA Comment: It would have been helpful if I had a more clear description of what this patch set wants to accomplish. [1] Going through the code changes, you remove ExportWithCsvProfile and you add ExportCircHistory. You also touch ExportRemoveFields. ExportRemoveFields is still found in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref. But it is not in sysprefs.sql. Still in use in tools/export.pl. But I would also expect these fields to pop up at the circulation screen in the Dont export fields box. [2] I do not see the combo (select name="csv_profile_id"). I guess it is hidden by js in checkouts.js. But also if I select csv, it does not show up. Refreshed, restarted, flushed, etc. No results. (I expected a combo with two csv profiles.) Failed QA
Created attachment 60372 [details] [review] Bug 15498: Let the user choose the CSV profile to export circ history The way the export options are displayed at the bottom of the checkouts table was not consistent. Prior to this patch set, they are display if ExportRemoveFields or ExportWithCsvProfile is set. It does not make any sense, the user could want to export the checkouts in iso2709 format without having to define a csv profile and fill the pref. Moreover the behavior of this pref did not match its description: it's used as a default CSV profile when exporting records from the export tools or the command line. This patch set adds a new pref ExportCircHistory and remove ExportWithCsvProfile. The new pref is set if ExportWithCsvProfile or ExportRemoveFields were set. A new dropdown list with the CSV profile list will be displayed in the export area, at the bottom of the checkouts table. Note that now --csv_profile_id is mandatory for the export command line (misc/export_records.pl) if the export format is csv. Test plan: 0/ Do not execute the DB entry 1/ Clear both ExportWithCsvProfile and ExportRemoveFields prefs 2/ Execute the DB entry 3/ ExportCircHistory should not be set and the export options should not be displayed at the bottom of the checkouts table. 4/ Remove the pref DELETE FROM systempreferences WHERE variable='ExportCircHistory'; and reinsert the previous one, with a value: INSERT INTO systempreferences (variable, value) VALUES ('ExportWithCsvProfile', 'something'); Execute the DB entry again => The now pref should be now set 5/ Export some checkouts using the CSV entry 6/ Note that the export tool and commandline script still work using the csv format. You have to provide a --csv_profile_id option to make it work. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60373 [details] [review] Bug 15498: Add ExportCircHistory and remove ExportWithCsvProfile Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60374 [details] [review] Bug 15498: Do not display sql csv profiles Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60375 [details] [review] Bug 15498: Fix show/hide csv profile list Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60376 [details] [review] Bug 15498: [QA Follow-up] Resolves: forbidden pattern: trailing space char (line 334)
Trivial rebase
Created attachment 60385 [details] [review] Bug 15498: Let the user choose the CSV profile to export circ history The way the export options are displayed at the bottom of the checkouts table was not consistent. Prior to this patch set, they are display if ExportRemoveFields or ExportWithCsvProfile is set. It does not make any sense, the user could want to export the checkouts in iso2709 format without having to define a csv profile and fill the pref. Moreover the behavior of this pref did not match its description: it's used as a default CSV profile when exporting records from the export tools or the command line. This patch set adds a new pref ExportCircHistory and remove ExportWithCsvProfile. The new pref is set if ExportWithCsvProfile or ExportRemoveFields were set. A new dropdown list with the CSV profile list will be displayed in the export area, at the bottom of the checkouts table. Note that now --csv_profile_id is mandatory for the export command line (misc/export_records.pl) if the export format is csv. Test plan: 0/ Do not execute the DB entry 1/ Clear both ExportWithCsvProfile and ExportRemoveFields prefs 2/ Execute the DB entry 3/ ExportCircHistory should not be set and the export options should not be displayed at the bottom of the checkouts table. 4/ Remove the pref DELETE FROM systempreferences WHERE variable='ExportCircHistory'; and reinsert the previous one, with a value: INSERT INTO systempreferences (variable, value) VALUES ('ExportWithCsvProfile', 'something'); Execute the DB entry again => The now pref should be now set 5/ Export some checkouts using the CSV entry 6/ Note that the export tool and commandline script still work using the csv format. You have to provide a --csv_profile_id option to make it work. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60386 [details] [review] Bug 15498: Add ExportCircHistory and remove ExportWithCsvProfile Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60387 [details] [review] Bug 15498: Do not display sql csv profiles Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60388 [details] [review] Bug 15498: Fix show/hide csv profile list Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60389 [details] [review] Bug 15498: [QA Follow-up] Resolves: forbidden pattern: trailing space char (line 334)
Created attachment 60390 [details] [review] Bug 15498: Fix conflict with bug 17394
(In reply to Marcel de Rooy from comment #18) > QA Comment: > It would have been helpful if I had a more clear description of what this > patch set wants to accomplish. Sorry, I thought everything was clear enough. > [1] Going through the code changes, you remove ExportWithCsvProfile and you > add ExportCircHistory. You also touch ExportRemoveFields. ExportRemoveFields > is still found in > koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref. > But it is not in sysprefs.sql. Still in use in tools/export.pl. But I would > also expect these fields to pop up at the circulation screen in the Dont > export fields box. It's a bug from master, opened bug 18136. > [2] I do not see the combo (select name="csv_profile_id"). I guess it is > hidden by js in checkouts.js. But also if I select csv, it does not show up. > Refreshed, restarted, flushed, etc. No results. (I expected a combo with two > csv profiles.) Caused by a changed of the element's id by bug 17394.
Comment on attachment 60389 [details] [review] Bug 15498: [QA Follow-up] Somehow this is useless. Space gets away when committing..
Created attachment 60411 [details] [review] Bug 15498: Let the user choose the CSV profile to export circ history The way the export options are displayed at the bottom of the checkouts table was not consistent. Prior to this patch set, they are display if ExportRemoveFields or ExportWithCsvProfile is set. It does not make any sense, the user could want to export the checkouts in iso2709 format without having to define a csv profile and fill the pref. Moreover the behavior of this pref did not match its description: it's used as a default CSV profile when exporting records from the export tools or the command line. This patch set adds a new pref ExportCircHistory and remove ExportWithCsvProfile. The new pref is set if ExportWithCsvProfile or ExportRemoveFields were set. A new dropdown list with the CSV profile list will be displayed in the export area, at the bottom of the checkouts table. Note that now --csv_profile_id is mandatory for the export command line (misc/export_records.pl) if the export format is csv. Test plan: 0/ Do not execute the DB entry 1/ Clear both ExportWithCsvProfile and ExportRemoveFields prefs 2/ Execute the DB entry 3/ ExportCircHistory should not be set and the export options should not be displayed at the bottom of the checkouts table. 4/ Remove the pref DELETE FROM systempreferences WHERE variable='ExportCircHistory'; and reinsert the previous one, with a value: INSERT INTO systempreferences (variable, value) VALUES ('ExportWithCsvProfile', 'something'); Execute the DB entry again => The now pref should be now set 5/ Export some checkouts using the CSV entry 6/ Note that the export tool and commandline script still work using the csv format. You have to provide a --csv_profile_id option to make it work. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60412 [details] [review] Bug 15498: Add ExportCircHistory and remove ExportWithCsvProfile Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60413 [details] [review] Bug 15498: Do not display sql csv profiles Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60414 [details] [review] Bug 15498: Fix show/hide csv profile list Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60415 [details] [review] Bug 15498: Fix conflict with bug 17394 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60416 [details] [review] Bug 15498: [QA Follow-up] Additional changes Adds ExportRemoveFields to sysprefs.sql Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 17.05, thanks Jonathan, Marcel!
This won't get ported back to 16.11.x as it is an enhancement.