Bug 5771 - Better error handling for import borrowers - Provide a download file of errors
Summary: Better error handling for import borrowers - Provide a download file of errors
Status: BLOCKED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P3 enhancement (vote)
Assignee: claire.hernandez@biblibre.com
QA Contact: Bugs List
URL:
Keywords:
: 8291 (view as bug list)
Depends on: 11317
Blocks: 5575
  Show dependency treegraph
 
Reported: 2011-02-17 22:14 UTC by Chris Cormack
Modified: 2017-01-17 15:33 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (7.11 KB, patch)
2011-02-17 22:18 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5771 : MT2894 Adds a download link for big error reports to import_borrowers (7.05 KB, patch)
2011-07-08 02:42 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5771 - Follow up MT2894 Adds a download link for big error reports to import_borrowers (3.04 KB, patch)
2012-06-09 13:17 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 5771 : Adds a download link for big error reports to import_borrowers (18.89 KB, patch)
2013-06-27 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Screenshot of downloaded report (68.25 KB, image/png)
2014-01-29 10:49 UTC, Holger Meißner
Details
Bug 5771 : Adds a download link for big error reports to import_borrowers (18.83 KB, patch)
2014-02-03 11:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5771 : Adds a download link for big error reports to import_borrowers (18.97 KB, patch)
2014-02-04 10:55 UTC, Holger Meißner
Details | Diff | Splinter Review
[PASSED QA] Bug 5771 : Adds a download link for big error reports to import_borrowers (19.12 KB, patch)
2014-03-12 16:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 5771 : Adds a download link for big error reports to import_borrowers (19.33 KB, patch)
2014-04-18 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5771: Set the directory path to /tmp (1.62 KB, patch)
2014-04-22 15:07 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 Chris Cormack 2011-02-17 22:14:16 UTC

    
Comment 1 Chris Cormack 2011-02-17 22:18:38 UTC Comment hidden (obsolete)
Comment 2 MJ Ray (software.coop) 2011-03-03 08:36:58 UTC
This bug is mentioned in:
Bug 5771 : MT2894 Adds a download link for	big error reports to import_borrowers http://lists.koha-community.org/pipermail/koha-patches/2011-February/013889.html
Comment 3 Katrin Fischer 2011-06-15 14:09:31 UTC
Needs to be reformatted for T:T

     <h5>Error analysis:</h5>
+    <!-- TMPL_IF NAME="download_errors" -->
+    <form method="post">
+	<input type="hidden" name="errors_filename" value="<!-- TMPL_VAR NAME="errors_filename" -->" />
+	<input type="hidden" name="report" value="report" />
+	<input type="submit" value="Download report" />
+    </form>
+    <!-- TMPL_ELSE -->
Comment 4 Chris Cormack 2011-07-08 02:42:53 UTC Comment hidden (obsolete)
Comment 5 Nicole C. Engard 2011-07-09 12:11:33 UTC
Can someone attach a patron file that will generate errors for testing?

Nicole
Comment 6 Owen Leonard 2011-08-16 12:13:38 UTC
I get errors when I try to open tools/import_borrowers.pl:

Global symbol "$commandline" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 77.
Global symbol "$commandline" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 82.
Global symbol "$commandline" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 316.
Global symbol "$commandline" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 318.
Global symbol "$lastalreadyindb" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 329.
Global symbol "$lastalreadyindb" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 329.
Global symbol "$lastinvalid" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 331.
Global symbol "$lastinvalid" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 331.
Global symbol "$commandline" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 355.
Global symbol "$commandline" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 360.
Global symbol "$commandline" requires explicit package name at /home/oleonard/kohaclone/tools/import_borrowers.pl line 367.
Comment 7 Alex Arnaud 2012-06-09 13:17:41 UTC Comment hidden (obsolete)
Comment 8 Alex Arnaud 2012-06-09 13:18:27 UTC
This patch is a follow up. I have fixed errors reported by Owen and reformatted errors_filename template variable for T:T.
Comment 9 Katrin Fischer 2012-08-17 05:29:59 UTC
I tried several combinations of files with format errors and I never got an option to download a file. I am not sure if I am looking in the wrong place? 

My import results looked like this:

0 imported records
0 overwritten
2 not imported because already in borrowers table and overwrite disabled (last was Anja / 53)
2 not imported because they are not in the expected format
4 records parsed


    Line 4 Critical field "categorycode" has unrecognized value "STU" (borrowernumber: 202; surname: Dora).
    202,"Test2","Dora","Mustermann","STU","MPL"
    Line 5 Critical field "categorycode" has unrecognized value "STB" (borrowernumber: 203; surname: Diana).
    203,"Test3","Diana","Mustermann","STB","MPL"

Looking at the template changes I don't see where the link should show up?
Comment 10 Jonathan Druart 2013-06-27 14:46:47 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-06-27 14:47:54 UTC
I removed the $commandline variable. Script in tools are only for the Koha interface, not command line.
Comment 12 I'm just a bot 2013-09-29 04:59:00 UTC
Patch applied cleanly, go forth and signoff
Comment 13 I'm just a bot 2013-10-24 19:34:49 UTC
Patch applied cleanly, go forth and signoff
Comment 14 Holger Meißner 2014-01-29 10:49:38 UTC
Created attachment 24858 [details]
Screenshot of downloaded report

The download button appears and a text file can be downloaded. But it only contains some kind of reference? See screenshot.
Comment 15 Jonathan Druart 2014-02-03 11:36:29 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-02-03 11:38:07 UTC
(In reply to Holger Meißner from comment #14)
> The download button appears and a text file can be downloaded. But it only
> contains some kind of reference? See screenshot.

Thanks for testing! The last patch should fix that.
Comment 17 Holger Meißner 2014-02-04 10:55:32 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2014-03-12 16:20:06 UTC
I'm happy that this all seems to work, and so far in my testing doesn't cause any regressions.  However, I'm still working on some profiling to ensure that super large imports don't casue issues.  Bare with me whilst I complete these tests, but with any luck this'll get past QA soon.

As a side note, I've been working on a re-write of the borrower imports code, bug 11916 and would appreciate comments upon it once there is somthing to test.
Comment 19 Martin Renvoize 2014-03-12 16:47:22 UTC Comment hidden (obsolete)
Comment 20 Martin Renvoize 2014-03-12 16:49:18 UTC
Comment on attachment 25019 [details] [review]
Bug 5771 : Adds a download link for big error reports to import_borrowers

I'm happy that this causes no regressions, passes all qa tests and after some minimal profiling and happy that it does not break with larger record sets.

Happy to pass QA.
Comment 21 Katrin Fischer 2014-04-18 09:55:28 UTC
I missed, that Martin already had passed this and ran some additional tests. All seems to work ok.
Comment 22 Katrin Fischer 2014-04-18 09:57:25 UTC
Created attachment 27278 [details] [review]
[PASSED QA] Bug 5771 : Adds a download link for big error reports to import_borrowers

If the import patrons tools produced a lot of errors(e.g. 1000), the
browser will crash or not be glad.

This patch allow to download an error file for report producing at least
25 errors.

Test plan:
Import patrons with at least 25 errors and check that a "Download
report" button appears and produce a text file to download.

Signed-off-by: Holger Meißner <h.meissner.82@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes tests and QA script.
Tested files with different problems, created from my Koha database
using the ./misc/export_borrowers.pl script and modifying it.
Comment 23 Galen Charlton 2014-04-18 15:09:10 UTC
Failed QA for a major security flaw:

/cgi-bin/koha/tools/import_borrowers.pl?report=1&errors_filename=/etc/passwd

Either more infrastructure is needed to maintain a secure area for storing and retrieving such files or (and this is possibly easier) errors from patron import runs should be stored in the database for later retrieval.

I also note that the patch as written introduces a bunch of untranslatable strings.
Comment 24 Katrin Fischer 2014-04-18 15:13:43 UTC
Hi Galen, glad you caught that! 

There is a bug that tries to work out something like that: bug 11317.
Comment 25 Katrin Fischer 2014-04-18 15:16:32 UTC
I noticed the untranslatable strings, but for generated files, I was not sure how to fix it. This still looked like an improvement as I have often been in a place where Koha only told me the last problematic patron, but I couldn't tell which where the other patrons that had failed to get imported.
Comment 26 Galen Charlton 2014-04-18 18:11:31 UTC
(In reply to Katrin Fischer from comment #25)
> I noticed the untranslatable strings, but for generated files, I was not
> sure how to fix it.

Broadly, one possibility is to use Template Toolkit, particularly since HTML output is reasonable for this particular report.
Comment 27 Jonathan Druart 2014-04-22 15:07:01 UTC
Created attachment 27405 [details] [review]
Bug 5771: Set the directory path to /tmp

The report filename should be written into /tmp
Comment 28 Jonathan Druart 2014-04-22 15:07:50 UTC
Galen,
Does something like the last patch could be acceptable?
Comment 29 Galen Charlton 2014-04-29 16:52:13 UTC
(In reply to Jonathan Druart from comment #28)
> Galen,
> Does something like the last patch could be acceptable?

It still makes me nervous.  I suggest another alternative: don't pass the filename around at all.  Instead, stick it in the user session, from where it can be retrieved if the user requests a download.
Comment 30 Martin Renvoize 2014-04-29 17:14:05 UTC
I'm working on a generalised file retrieval patch based upon the work initially in bug 11317.. maybe that might help here..

My idea is to add per file permissions based upon a db table of permissions. The default permission for any files would be inaccessible until confugred on a permissions page.

I'll post back here once i've got something more concrete to test.
Comment 31 Jonathan Druart 2015-09-22 10:59:32 UTC
Blocked by 11317.
Comment 32 Katrin Fischer 2016-08-14 18:54:51 UTC
*** Bug 8291 has been marked as a duplicate of this bug. ***