Created attachment 3177 [details] [review] Proposed Patch
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
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 -->
Created attachment 4589 [details] [review] Bug 5771 : MT2894 Adds a download link for big error reports to import_borrowers Reformated for template::toolkit
Can someone attach a patron file that will generate errors for testing? Nicole
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.
Created attachment 10023 [details] [review] Bug 5771 - Follow up MT2894 Adds a download link for big error reports to import_borrowers
This patch is a follow up. I have fixed errors reported by Owen and reformatted errors_filename template variable for T:T.
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?
Created attachment 19263 [details] [review] 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.
I removed the $commandline variable. Script in tools are only for the Koha interface, not command line.
Patch applied cleanly, go forth and signoff
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.
Created attachment 25004 [details] [review] 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.
(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.
Created attachment 25019 [details] [review] 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>
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.
Created attachment 26204 [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>
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.
I missed, that Martin already had passed this and ran some additional tests. All seems to work ok.
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.
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.
Hi Galen, glad you caught that! There is a bug that tries to work out something like that: bug 11317.
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.
(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.
Created attachment 27405 [details] [review] Bug 5771: Set the directory path to /tmp The report filename should be written into /tmp
Galen, Does something like the last patch could be acceptable?
(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.
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.
Blocked by 11317.
*** Bug 8291 has been marked as a duplicate of this bug. ***