Add filters and export button (iso2709 or csv) on issues table (ref biblibre: MT5671)
Created attachment 9269 [details] [review] Bug 7986: Export issues for patron Contrary to the description, this patch does not add filters on the issues table. In the circulation page, you can now export (as csv or iso2709) a list of items which are currently checked out by a borrower. 3 export types: - iso2709 with items: Export the items list in iso2709 format with item informations. - iso2709 without items: Export the items list in iso2709 format without item informations. - CSV: Export the items list based on a csv profil. 2 new system preferences: - DontExportFields: a list of fields not to be export - CsvProfileForExport: The Csv profil name used for the csv export Test plan: - Fill the CsvProfileForExport syspref - go on the borrower circulation page containing checkouts - Select one or more items and export them to the 3 different formats. - check if the result file is what you expected
Could you provide a bit more detail on the CSV profiles? I have no idea what to expect, so I can't test the bug. Also, what goes into the DontExportFields syspref? A comma-separated list of MARC tags? Something else?
I think negative preferences are often a bit confusing - perhaps it could be something like ExportFields where empty means all and else you can list those you want to export?
Katrin just clarified what csv profiles are for me. I think it would be better to eliminate the DontExportFields syspref, and just use a profile that matches what you want. Less confusing.
Well, I disagree. ;^) The DontExportFields is not related to CSV export only but also to iso2709 export. And I can't define any iso2709 profile... And in this case, it's relevant using this syspref. It's totally different from activating a syspref to not do something. It would be confusing. But here, the syspref talks for itself : DontExportFields = I dont want to export these fields. It does what it says! ;^) Moreover, if you use this marc field, very specific to your library that nobody will want. What you expect is to export every fields but this one. And it would be crazy to list every, maybe 30, fields you want when you can only say you don't want this one... ;^)
Hi Francois, I agree, there are too many MARC fields to do it with "ExportFields". I thought it was about the item fields.
Created attachment 9972 [details] [review] Bug 7986: Export issues for patron Rebased patch
Created attachment 10635 [details] [review] Bug 7986: Export issues for patron Rebased patch
Created attachment 10675 [details] [review] Bug 7986: Export issues for patron In the circulation page, you can now export (as csv or iso2709) a list of items which are currently checked out by a borrower. 3 export types: - iso2709 with items: Export the items list in iso2709 format with item informations. - iso2709 without items: Export the items list in iso2709 format without item informations. - CSV: Export the items list based on a csv profil. 2 new system preferences: - DontExportFields: a list of fields not to be export - CsvProfileForExport: The Csv profile name used for the csv export Test plan: - Fill the CsvProfileForExport syspref - go on the borrower circulation page containing checkouts - Select one or more items and export them to the 3 different formats. - check if the result file is what you expected Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Works as advertised, and the export tool still works after the patch is applied: both bibliographic and authority records are exported properly, and respect the DontExportFields (and, reading francharb's response to my comments on DontExportFields, I agree that it is named correctly and is needed).
Created attachment 11002 [details] [review] Bug 7986: Export issues for patron In the circulation page, you can now export (as csv or iso2709) a list of items which are currently checked out by a borrower. 3 export types: - iso2709 with items: Export the items list in iso2709 format with item informations. - iso2709 without items: Export the items list in iso2709 format without item informations. - CSV: Export the items list based on a csv profil. 2 new system preferences: - DontExportFields: a list of fields not to be export - CsvProfileForExport: The Csv profile name used for the csv export Test plan: - Fill the CsvProfileForExport syspref - go on the borrower circulation page containing checkouts - Select one or more items and export them to the 3 different formats. - check if the result file is what you expected - Test there is no regression with the export authority - Test there is no regression using tools/export.pl with the command line interface Jared, I remove your signoff :-( Indeed, I rewrite a big part of this patch (caused by Bug 5600). /!\ This patch must be applied after the patch proposed for Bug 5600.
Created attachment 11388 [details] [review] Bug 7986: Export issues for patron Rebased patch.
The good news is no regressions, the bad news is, the checkboxes to select items are not showing, so I cant test that you can export from circulation.
Ah ha The checkboxes are only there for the previssues loop, but they dont show for items issued today (in the todayissues loop) Do you want to fix or I can do a follow up.
Created attachment 11407 [details] [review] Bug 7986: Followup: FIX display checkboxes for the today checkouts Hi Chris, Good catch ! :) This followup should solve the issue. Thanks for testing
Created attachment 11408 [details] [review] Bug 7986: Export issues for patron In the circulation page, you can now export (as csv or iso2709) a list of items which are currently checked out by a borrower. 3 export types: - iso2709 with items: Export the items list in iso2709 format with item informations. - iso2709 without items: Export the items list in iso2709 format without item informations. - CSV: Export the items list based on a csv profil. 2 new system preferences: - DontExportFields: a list of fields not to be export - CsvProfileForExport: The Csv profile name used for the csv export Test plan: - Fill the CsvProfileForExport syspref - go on the borrower circulation page containing checkouts - Select one or more items and export them to the 3 different formats. - check if the result file is what you expected - Test there is no regression with the export authority - Test there is no regression using tools/export.pl with the command line interface Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 11409 [details] [review] Bug 7986: Followup: FIX display checkboxes for the today checkouts Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
QA Comment: Code looks good to me. Not tested, refer to signoff. Just a teasing remark :) + EmbedItemsInMarcBiblio($marc, $biblionumber, $itemnumbers); Not clearly documented that itemnumbers should be an arrayref. Same for sub marc2csv, marcrecord2csv Passed QA
I'm like others: I don't like "negative" system preferences. What about renaming "DontExportFields" to "ExportRemoveFields" ? (and also rename CsvProfileForExport to ExportWithCsvProfile, so both export related sysprefs are starting with "Export") The "DontExportFields" syspref is used only 3 times in your patches, should be very easy to rename it. So please provide a follow-up to rename the field and I'll push & test everything.
Created attachment 11972 [details] [review] Bug 7986: Followup: Replace DontExportFields and CsvProfileForExport Replace DontExportFieldsi by ExportRemoveFields and CsvProfileForExport by ExportWithCsvProfile
Created attachment 11998 [details] [review] Bug 7986: Export issues for patron In the circulation page, you can now export (as csv or iso2709) a list of items which are currently checked out by a borrower. 3 export types: - iso2709 with items: Export the items list in iso2709 format with item informations. - iso2709 without items: Export the items list in iso2709 format without item informations. - CSV: Export the items list based on a csv profil. 2 new system preferences: - DontExportFields: a list of fields not to be export - CsvProfileForExport: The Csv profile name used for the csv export Test plan: - Fill the CsvProfileForExport syspref - go on the borrower circulation page containing checkouts - Select one or more items and export them to the 3 different formats. - check if the result file is what you expected - Test there is no regression with the export authority - Test there is no regression using tools/export.pl with the command line interface Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 11999 [details] [review] Bug 7986: Followup: FIX display checkboxes for the today checkouts Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 12000 [details] [review] Bug 7986: Followup: Replace DontExportFields and CsvProfileForExport Replace DontExportFields by ExportRemoveFields and CsvProfileForExport by ExportWithCsvProfile
rebased patches. The last one is rewritten. Now all occurrences are replaced.
The last patch does not apply Applying: Bug 7986: Followup: Replace DontExportFields and CsvProfileForExport fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 7986: Followup: Replace DontExportFields and CsvProfileForExport When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/Bug-7986-Followup-Replace-DontExportFields-and-Csv-kNu3V9.patch Jonathan, note that you can also squash the 3 patches in one, that will be easier to solve updatedatabase conflict !
Created attachment 12168 [details] [review] Bug 7986: Export issues for patron Rebased and squashed patch
Patch pushed to master with 2 tiny string changes to fix English & a better translatability Note that I'm not a fan of adding a new information to the circulation screen. I had preferred something less visible. I've no counter-proposal, so I've pushed, but some ergonomic improvement would be cool.
(In reply to comment #27) > Note that I'm not a fan of adding a new information to the circulation > screen. I had preferred something less visible. I agree, and I'm sorry I didn't see this until now. I would definitely like to come up with an alternative solution. We already have an "export checkins" option under the "More" menu (at least on some pages--it hasn't been added to all the necessary includes for it to be consistent), and now we have a new export option in a different place. What if we put both export options into a new tab on the "Circulation history" page? Both are directly related to circulation history. Are they operations which happen so frequently that putting them onto another page would cause problems?
Maybe we could also hide the tab/the feature when both system preferences are empty? I think this is not a feature that is interesting for all libraries, so hiding it would make sense to me. I also like the idea of a separate tab.
(In reply to comment #28) > I agree, and I'm sorry I didn't see this until now. I would definitely like > to come up with an alternative solution. We already have an "export > checkins" option under the "More" menu (at least on some pages--it hasn't > been added to all the necessary includes for it to be consistent), and now > we have a new export option in a different place. > > What if we put both export options into a new tab on the "Circulation > history" page? Both are directly related to circulation history. Are they > operations which happen so frequently that putting them onto another page > would cause problems? The export link in the "More" menu only exports barcodes of the exports of the day. The "export checkouts" link generates a csv or a iso2709 file with the checkout you have checked. It is not the same feature at all. So to add a new tab, a new table with checkouts will have to be added. Theses information already exist (under different format) in the "Check out", "Details" and "Circulation history" tabs. I don't know if we want to add them a fourth times. Don't you agree with this ?
Hm, I think you are right about the duplicated information. Maybe we could have a checkbox or option that would make the checkboxes for exporting and other things show up? Similar to how you have to activate the option for overriding renewals first.
(In reply to comment #29) > Maybe we could also hide the tab/the feature when both system preferences > are empty? Great idea = if the syspref is empty, then it probably mean the library will never use this feature (and, if the librarian tries, there will be an error message) I think it is also easy to code ! > I think this is not a feature that is interesting for all > libraries, so hiding it would make sense to me. I also like the idea of a > separate tab. The problem is that libraries interested are using this feature all the day, so it must be as easy as possible to access. A (french) Use case = we have regional-wide libraries (100 all over France) that issues books to small libraries, in small villages. The local library comes, pick-up 100+ books. They are checked-out, and, once they're done, the regional-wide library gives them the list in iso2709 on a USB key, so they can put them in a few seconds in their local ILS (which is not always Koha, unfortunately ;-) ) Some libraries even don't have an ILS, in this case they just get a CSV file for import into their spreadsheat.
Created attachment 12212 [details] [review] Bug 7986: Followup: Hide the export link unless sysprefs are set If at least one of both sysprefs (ExportRemoveFields and ExportWithCsvProfile) is filled, the export link is displayed
Thanks for the explanation Paul. I'm going to test an interface idea which I think will be a good solution for all.
(In reply to comment #33) > If at least one of both sysprefs (ExportRemoveFields and > ExportWithCsvProfile) is filled, the export link is displayed The column of checkboxes for export should also be shown only if one of the two system preferences is filled.
Created attachment 12296 [details] [review] Bug 7986: Followup: Hide the export link unless sysprefs are set If at least one of both sysprefs (ExportRemoveFields and ExportWithCsvProfile) is filled, the export link is displayed
Created attachment 12457 [details] [review] [SIGNED-OFF] Bug 7986: Followup: Hide the export link unless sysprefs are set If at least one of both sysprefs (ExportRemoveFields and ExportWithCsvProfile) is filled, the export link is displayed Signed-off-by: Marc Veron <veron@veron.ch> Tested with both sysprefs filled/not filled, patch behaves as expected.
I'm trying to document this feature, but see no examples at all in this thread on what the preferences are looking for as values. Is it a comma separated list? If so, what does it look like? 100a, 245a or 100$a, 952$8 ?? Please provide me with an example so I can add these preferences to the manual. Nicole
I also don't know what to put in the ExportWithCsvProfile preference ... tips for the manual are needed before I can document these. Nicole
(In reply to comment #39) > I also don't know what to put in the ExportWithCsvProfile preference ... > tips for the manual are needed before I can document these. > > Nicole Nicole, - CsvProfileForExport: The Csv profile name used for the csv export => it must contain any of CSV profile name defined in /cgi-bin/koha/tools/csv-profiles.pl HTH QA comment for the follow-up: tiny HTML only follow-up, passes koha-qa.pl, passed QA
(In reply to comment #38) > I'm trying to document this feature, but see no examples at all in this > thread on what the preferences are looking for as values. > > Is it a comma separated list? If so, what does it look like? > > 100a, 245a > > or > > 100$a, 952$8 > > ?? Please provide me with an example so I can add these preferences to the > manual. > > Nicole Hi Nicole. The format for the syspref is a list a field separated by space. e.g. 100a 200b 300c It is the format for the tools/export.pl script.
Created attachment 12819 [details] [review] Bug 7986 Follow Up: Update ExportWithCsvProfile The ExportWithCsvProfile preference was on it's own file (tools.pref) when in reality it belonged on the circulation tab. This patch removes the tools.pref file and moves the ExportWithCsvProfile preference and updates the text around it to make it clearer.
Follow up needs sign off. Also, When I look at circulation.pref I see this: - - pref: ExportRemoveFields - choices: yes: Export no: "Don't export" - fields for csv or iso2709 export But when I look at the screen I don't see a pull down for this preference, I see a text box. Is it supposed to be a pull down or a text box?
updating to get sign off on follow up
Comment on attachment 12457 [details] [review] [SIGNED-OFF] Bug 7986: Followup: Hide the export link unless sysprefs are set pushed to master already
Created attachment 12821 [details] [review] Bug 7986 Follow Up: Update ExportWithCsvProfile The ExportWithCsvProfile preference was on it's own file (tools.pref) when in reality it belonged on the circulation tab. This patch removes the tools.pref file and moves the ExportWithCsvProfile preference and updates the text around it to make it clearer.
Created attachment 12822 [details] [review] [SIGNED-OFF] Bug 7986 Follow Up: Update ExportWithCsvProfile The ExportWithCsvProfile preference was on its own file (tools.pref) when in reality it belonged on the circulation tab. This patch removes the tools.pref file and moves the ExportWithCsvProfile preference and updates the text around it to make it clearer. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 12824 [details] ExportRemoveFields ExportRemoveFields preference without pull down
Created attachment 12840 [details] [review] Bug 7986 Follow Up: Update ExportWithCsvProfile The ExportWithCsvProfile preference was on its own file (tools.pref) when in reality it belonged on the circulation tab. This patch removes the tools.pref file and moves the ExportWithCsvProfile preference and updates the text around it to make it clearer. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 12841 [details] [review] Bug 7986 Followup: Update ExportRemoveFields
Created attachment 12850 [details] [review] Bug 7986 Follow Up: Fix ExportRemoveFields This preference was in the code as a choice but should be a list of fields to exclude. This patch makes that change and updates the description to fit more with the standard set by other preferences.
Jonathan, I hope you don't mind, but I updated your patch to show the preference in a bit clearer of a sentence (to match the format of other similar preferences). I added the info about this being the patron checkout export only - not all CSV exports. Nicole
Created attachment 12882 [details] [review] Bug 7986 Follow Up: Fix ExportRemoveFields This preference was in the code as a choice but should be a list of fields to exclude. This patch makes that change and updates the description to fit more with the standard set by other preferences. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
(In reply to comment #52) > I hope you don't mind, but I updated your patch to show the preference in a > bit clearer of a sentence Quite the opposite ! Thank you to correct me :)
Two string follow ups need QA before the string freeze please. Thanks Nicole
(In reply to comment #55) > Two string follow ups need QA before the string freeze please. > > Thanks > Nicole passing QA on these last 2 patches... $ koha-qa.pl -c 2 testing 2 commit(s) (applied to commit 32fab74) * c1fa07f Bug 7986 Follow Up: Fix ExportRemoveFields koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref * 9bafa74 Bug 7986 Follow Up: Update ExportWithCsvProfile koha-tmpl/intranet-tmpl/prog/en/includes/prefs-menu.inc koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/tools.pref * koha-tmpl/intranet-tmpl/prog/en/includes/prefs-menu.inc OK
Patch pushed to master