Bug 7986

Summary: Export issues
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: chris, francois.charbonnier, gmcharlt, katrin.fischer, m.de.rooy, mtj, nengard, paul.poulain, veron
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 5600, 8202    
Bug Blocks: 10482    
Attachments: Bug 7986: Export issues for patron
Bug 7986: Export issues for patron
Bug 7986: Export issues for patron
Bug 7986: Export issues for patron
Bug 7986: Export issues for patron
Bug 7986: Export issues for patron
Bug 7986: Followup: FIX display checkboxes for the today checkouts
Bug 7986: Export issues for patron
Bug 7986: Followup: FIX display checkboxes for the today checkouts
Bug 7986: Followup: Replace DontExportFields and CsvProfileForExport
Bug 7986: Export issues for patron
Bug 7986: Followup: FIX display checkboxes for the today checkouts
Bug 7986: Followup: Replace DontExportFields and CsvProfileForExport
Bug 7986: Export issues for patron
Bug 7986: Followup: Hide the export link unless sysprefs are set
Bug 7986: Followup: Hide the export link unless sysprefs are set
[SIGNED-OFF] Bug 7986: Followup: Hide the export link unless sysprefs are set
Bug 7986 Follow Up: Update ExportWithCsvProfile
Bug 7986 Follow Up: Update ExportWithCsvProfile
[SIGNED-OFF] Bug 7986 Follow Up: Update ExportWithCsvProfile
ExportRemoveFields
Bug 7986 Follow Up: Update ExportWithCsvProfile
Bug 7986 Followup: Update ExportRemoveFields
Bug 7986 Follow Up: Fix ExportRemoveFields
Bug 7986 Follow Up: Fix ExportRemoveFields

Description Jonathan Druart 2012-04-20 10:04:28 UTC
Add filters and export button (iso2709 or csv) on issues table

(ref biblibre: MT5671)
Comment 1 Jonathan Druart 2012-04-20 13:48:33 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-05-13 14:22:05 UTC
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?
Comment 3 Katrin Fischer 2012-05-13 17:32:58 UTC
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?
Comment 4 Jared Camins-Esakov 2012-05-13 17:42:10 UTC
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.
Comment 5 Francois Charbonnier 2012-05-14 08:21:39 UTC
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... ;^)
Comment 6 Katrin Fischer 2012-05-14 08:23:41 UTC
Hi Francois,

I agree, there are too many MARC fields to do it with "ExportFields". I thought it was about the item fields.
Comment 7 Jonathan Druart 2012-06-07 11:53:27 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2012-07-05 08:55:45 UTC Comment hidden (obsolete)
Comment 9 Jared Camins-Esakov 2012-07-06 14:55:53 UTC Comment hidden (obsolete)
Comment 10 Jared Camins-Esakov 2012-07-06 15:03:41 UTC
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).
Comment 11 Jonathan Druart 2012-07-19 11:36:55 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-08-06 15:10:46 UTC Comment hidden (obsolete)
Comment 13 Chris Cormack 2012-08-07 08:00:59 UTC
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.
Comment 14 Chris Cormack 2012-08-07 08:06:50 UTC
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.
Comment 15 Jonathan Druart 2012-08-07 08:44:46 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2012-08-07 09:02:14 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2012-08-07 09:02:29 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2012-08-30 10:31:54 UTC
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
Comment 19 Paul Poulain 2012-09-05 08:11:19 UTC
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.
Comment 20 Jonathan Druart 2012-09-05 09:03:02 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2012-09-06 08:42:45 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2012-09-06 08:43:00 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2012-09-06 08:43:06 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2012-09-06 08:45:55 UTC
rebased patches.
The last one is rewritten. Now all occurrences are replaced.
Comment 25 Paul Poulain 2012-09-12 13:38:59 UTC
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 !
Comment 26 Jonathan Druart 2012-09-12 13:46:12 UTC Comment hidden (obsolete)
Comment 27 Paul Poulain 2012-09-13 16:10:49 UTC
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.
Comment 28 Owen Leonard 2012-09-14 00:53:51 UTC
(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?
Comment 29 Katrin Fischer 2012-09-14 08:00:24 UTC
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.
Comment 30 Jonathan Druart 2012-09-14 08:00:33 UTC
(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 ?
Comment 31 Katrin Fischer 2012-09-14 08:26:06 UTC
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.
Comment 32 Paul Poulain 2012-09-14 09:15:39 UTC
(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.
Comment 33 Jonathan Druart 2012-09-14 09:40:46 UTC Comment hidden (obsolete)
Comment 34 Owen Leonard 2012-09-14 09:46:35 UTC
Thanks for the explanation Paul. I'm going to test an interface idea which I think will be a good solution for all.
Comment 35 Owen Leonard 2012-09-17 16:20:55 UTC
(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.
Comment 36 Jonathan Druart 2012-09-18 07:38:38 UTC Comment hidden (obsolete)
Comment 37 Marc Véron 2012-09-23 16:19:38 UTC Comment hidden (obsolete)
Comment 38 Nicole C. Engard 2012-09-24 15:40:22 UTC
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
Comment 39 Nicole C. Engard 2012-09-28 14:27:39 UTC
I also don't know what to put in the ExportWithCsvProfile preference ... tips for the manual are needed before I can document these.

Nicole
Comment 40 Paul Poulain 2012-10-04 16:11:54 UTC
(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
Comment 41 Jonathan Druart 2012-10-15 14:44:01 UTC
(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.
Comment 42 Nicole C. Engard 2012-10-15 15:45:50 UTC Comment hidden (obsolete)
Comment 43 Nicole C. Engard 2012-10-15 15:46:44 UTC
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?
Comment 44 Nicole C. Engard 2012-10-15 15:47:06 UTC
updating to get sign off on follow up
Comment 45 Nicole C. Engard 2012-10-15 15:47:28 UTC
Comment on attachment 12457 [details] [review]
[SIGNED-OFF] Bug 7986: Followup: Hide the export link unless sysprefs are set

pushed to master already
Comment 46 Nicole C. Engard 2012-10-15 15:54:59 UTC Comment hidden (obsolete)
Comment 47 Owen Leonard 2012-10-15 15:59:37 UTC Comment hidden (obsolete)
Comment 48 Nicole C. Engard 2012-10-15 16:21:01 UTC
Created attachment 12824 [details]
ExportRemoveFields

 ExportRemoveFields  preference without pull down
Comment 49 Jonathan Druart 2012-10-16 07:54:31 UTC
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>
Comment 50 Jonathan Druart 2012-10-16 07:54:40 UTC Comment hidden (obsolete)
Comment 51 Nicole C. Engard 2012-10-16 14:43:39 UTC Comment hidden (obsolete)
Comment 52 Nicole C. Engard 2012-10-16 14:44:38 UTC
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
Comment 53 Jonathan Druart 2012-10-17 09:59:35 UTC
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>
Comment 54 Jonathan Druart 2012-10-17 10:03:30 UTC
(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 :)
Comment 55 Nicole C. Engard 2012-10-18 18:33:20 UTC
Two string follow ups need QA before the string freeze please.

Thanks
Nicole
Comment 56 Mason James 2012-10-19 03:40:39 UTC
(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
Comment 57 Paul Poulain 2012-10-19 15:55:32 UTC
Patch pushed to master