Created attachment 23203 [details] [review] Adds a page to access log files on the server from the intranet. This patch adds a new report section (Report/log files) that lets you access log files in a specific directory on the server. The directory to use can be specified by the "publiclogdir" config in koha-conf.xml : <config> ... <publiclogdir>/var/log/koha-public</publiclogdir> ... </config> With this feature, you can configure cron scripts so they create files in the publiclogdir directory, and these files will then be accessible in the intranet. This is useful when using the html export of overdue_notices.pl or the invalid_856u.sh script as a cron. Sponsored-by: CCSR ( http://www.ccsr.qc.ca )
Frédérick, Could you provide a test plan and replace tablesorter with datatables please? Marked as Failed QA.
Created attachment 25387 [details] [review] New version of the patch, now using datatables Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This preference should be in the "config" section. For this test cas, we will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. Download the file and verify its content. It should contain the text "Hello world!".
Removing the "log" as I think the use case is a bit wider - it could be an interesting feature to use with printed notices (overdues, holds, etc.)
I like this patch... but, I'de like to see it implemented such that the files in question have perma-links as opposed to the current dynamically created ones which could change. I'de also love to see a followup that allowed files to require authentication or not.
I think I like the idea of this being 'required authentication' only and having another mostly separate feature for publicly available files. We have been thinking about how the library could get to printed holds and overdue notices. Moving them to an SFTP server or similar would be an option, but having a secure way in Koha to access and download the files would be better.
Created attachment 27492 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created.
Added a follow-up to create permalinks to files based upon filename only (We may want to link to a changing file in the case of logs). I'de still like to see this changed to reflect different permissions more specifically, to enable a more general approach, as opposed to being a reports only feature.
The second patch does not apply Applying: Bug 11317: Follow up - Update ID to allow for permalinking fatal: sha1 information is lacking or useless (reports/report_files.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
Created attachment 27579 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created.
Hmm, I found a minor bug in my code anyways, so I've uploaded a new patch.. However, I couldn't replicate your issue anyway Chris, are you still having issues?
Created attachment 27604 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created.
Thanks Joubu.. no idea why it wasn't applying.. but your new one applies fine for me still and hopefully also for everyone else too..
Created attachment 35938 [details] [review] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 35939 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 35940 [details] [review] Bug 11317: Rename publiclogdir with publicdir Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
I suggest to rename publiclogdir with publicdir. Feel free so obsolete the patch if you disagree.
FAIL reports/report_files.pl FAIL critic I/O layer ":utf8" used at line 78, column 9. Use ":encoding(UTF-8)" to get strict validation. Bareword file handle opened at line 78, column 9. See pages 202,204 of PBP. FAIL forbidden patterns forbidden pattern: trailing space char (line 61) forbidden pattern: trailing space char (line 8) forbidden pattern: tab char (line 45) forbidden pattern: trailing space char (line 60) forbidden pattern: tab char (line 52) forbidden pattern: tab char (line 41) forbidden pattern: tab char (line 42) forbidden pattern: Koha is now under the GPLv3 license (line 13) forbidden pattern: tab char (line 43) forbidden pattern: tab char (line 39) forbidden pattern: tab char (line 55) forbidden pattern: tab char (line 44) forbidden pattern: tab char (line 40) FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/report_files.tt FAIL forbidden patterns forbidden pattern: tab char (line 10) forbidden pattern: To include datatables files, please use the include file (see bug 10868) (line 5) forbidden pattern: tab char (line 31) forbidden pattern: tab char (line 33) forbidden pattern: tab char (line 19) forbidden pattern: tab char (line 30) forbidden pattern: tab char (line 18) forbidden pattern: tab char (line 9)
Created attachment 36844 [details] [review] Bug 11317: Correct critic, forbidden patterns issues when tested with koha-test-tools
How does this relate to something like bug 6874? :)
Created attachment 39783 [details] [review] [SIGNED-OFF] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 39784 [details] [review] [SIGNED-OFF] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 39785 [details] [review] [SIGNED-OFF] Bug 11317: Rename publiclogdir with publicdir Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 39786 [details] [review] [SIGNED-OFF] Bug 11317: Correct critic, forbidden patterns issues when tested with koha-test-tools Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, nice addition. No koha-qa errors
Please provide a test plan.
(In reply to Kyle M Hall from comment #24) > Please provide a test plan. I'm assuming they followed comment #2, comment #13, and/or comment #20.
(In reply to Kyle M Hall from comment #24) > Please provide a test plan. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!".
Could we have some documentation on what a sensible directory location would look like, and what permissions the files within it may require to be accessible by this route (and only this route)?
I am really keen on this feature, as Koha lacks a good way to access files like the ones generated for print notices. I have a few concerns tho: - The files should be stored in a directory on the server that is safe and cannot be accessed from outside as these files can and will most likely contain patron information in some way. How can we make this happen? I think the least we need to do is add a documentation saying that clearly. - Related to the first thing... can we please rename publicdir again to indicate something that should not be public/open? internal_files? privatefiledir? Not really a good idea here, but someone else might have.
A further enhancement I'd love to see is multiple directories. The was initially designed as a way to expose log files, and it shows. It's fast turning into a more generic solution for exposing notices, logs, whatever... but putting notices under a tool called 'reports' and combining files with different purposes into one directory both make little sense.. I'm thinking along the lines of a config block of the form: <staffdirs> <reports> <dir></dir> <permission></permission> </reports> <notices> <dir></dir> <permission></permission> </notices> </staffdirs> This would yield a much more comprehensive and viable feature
Some more notes: - +use strict; +use warnings; > Please use Modern::Perl; instead - The table should use a standard display format of Koha for the date and time information. The information form the patch doesn't translate and can't be configured with the dateformat/timeformat preferences: Thu Aug 2 16:41:30 2012 - I feel this would be better as a tool than as a link on the reports page - I don't really see a connection to reports so much, as it's a very general tool. - I am still in favor of renaming the directory to something less 'public' and we should have some information on how to use this feature safely without exposing data accidentally via your web server that should be kept safe. Possible enhancements: - As the kind of files presented can be quite different, it would be nice if this had its own permission. - Allow multiple directories - @Martin, maybe this could be added later?
We have a hard time figuring a good name for publicdir. Bouzid will do all the fixes you asked, but I expect someone will come with another patch just to change the config name, eventually. Regarding the multidir suggestion, we like it. But that's outside of the sponsored work, and I must say would be really easy to add once this is accepted. Making this harder to accept from the start is probably not where we want to be. Further hint as to what is looked for as for documentation would be a great help. Not sure where to start.
Created attachment 44859 [details] [review] Test case : Preparation : * Add the "filesaccess" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/files-access" directory : <config> ... <filesaccess>/tmp/files-access</filesaccess> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/files-access * Add a test file in the log directory : $ cd /tmp/files-access $ echo "Hello world!" > test.txt Test : * Go in the "tools/Additional tools" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!".
Created attachment 44860 [details] [review] Bug 11317: translate date with datetime format preference,replace in tools and rename preference in Koha-config.xml. Test case : Preparation : * Add the "filesaccess" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/files-access" directory : <config> ... <filesaccess>/tmp/files-access</filesaccess> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/files-access * Add a test file in the log directory : $ cd /tmp/files-access $ echo "Hello world!" > test.txt Test : * Go in the "tools/Additional tools" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!".
Hi Bouzid, thx for the new patches! For documentation I have following ideas: - Add a help page to the new page containing information on how to set this up. - Show a message on the About page of Koha and/or on the page itself, if the directory is not set up correctly - Show some kind of warning that people should set this up carefully because the files can include patron/sensitive information and what to look out for? Could we set up a safe (not accessible by URL) default directory for people to use? (normal installation... packages?) I am not sure how this could best be done technically, but someone else might be able to help.
Created attachment 48629 [details] [review] Bug 11317 - Rename config to 'accessdir' and fix qa Also translate date with datetime format preference and move to Tools. Test preparation : * Add the "accessdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/files-access" directory : <config> ... <accessdir>/tmp/files-access</accessdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/files-access * Add a test file in the log directory : $ cd /tmp/files-access $ echo "Hello world!" > test.txt Test : * Go in the "tools/Additional tools" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!".
The last patch introduced 'filesaccess', but didn't replace all 'publicdir' reference. I squashed the last three patches because I did not see the value of having a patch that changed to publicdir signed off followed by a patch to move it to accessdir then another patch to fix it... This feature does not warrant that many patches in Koha's history. An historian...
Created attachment 48635 [details] [review] Bug 11317 - Allows for multiple directories to be accessible It is now possible to add as many <accessdir>SOMEDIR</accessdir> as needed to the config file.
Created attachment 49941 [details] [review] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 49942 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 49943 [details] [review] Bug 11317 - Rename config to 'accessdir' and fix qa Also translate date with datetime format preference and move to Tools. Test preparation : * Add the "accessdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/files-access" directory : <config> ... <accessdir>/tmp/files-access</accessdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/files-access * Add a test file in the log directory : $ cd /tmp/files-access $ echo "Hello world!" > test.txt Test : * Go in the "tools/Additional tools" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 49944 [details] [review] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 49945 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 49946 [details] [review] Bug 11317 - Rename config to 'accessdir' and fix qa Also translate date with datetime format preference and move to Tools. Test preparation : * Add the "accessdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/files-access" directory : <config> ... <accessdir>/tmp/files-access</accessdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/files-access * Add a test file in the log directory : $ cd /tmp/files-access $ echo "Hello world!" > test.txt Test : * Go in the "tools/Additional tools" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 49947 [details] [review] Bug 11317 - Allows for multiple directories to be accessible It is now possible to add as many <accessdir>SOMEDIR</accessdir> as needed to the config file. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Very minor conflict from EDI cleared qa tool complains: FAIL reports/report_files.pl OK critic OK forbidden patterns OK pod OK spelling FAIL valid Name "Tie::Hash::FIELDS" used only once: possible typo Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo
Hi Nick, that qa test problem will be fixed by another patch soon. Is this otherwise ok?
(In reply to Katrin Fischer from comment #46) > Hi Nick, that qa test problem will be fixed by another patch soon. Is this > otherwise ok? Yes
Applying: Bug 11317 - Rename config to 'accessdir' and fix qa fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/reports/report_files.tt). Repository lacks necessary blobs to fall back on 3-way merge.
I can rebase the patches. But before that, I'd like to understand something: this was a report that became a tool, but it's still a report (reports/report_files.pl) and has report permissions. While being always available in the tools list... as long as you have access to other tools (because with 'reports' right you can run the report if you have the URL, but you can't view it in tools-home.tt unless you have some other tool access, which you don't necessarily want to give). My suggestion: - Make it fully a report OR a tool. Moving/renaming the files accordingly. - Add a specific permission to it if it's a tool. This is an original CCSR patch we've alreay put lot of hours on to make it acceptable, so I hate to pull the rug under so many Signed-off, and I fear the "In discussion", but the situation descrived above is live with a customer, so this is a real-world problem. If a QA would just tell me the standard acceptable solution, I'll code it quickly.
To me this feels more like a tool and I really like the idea of having a separate permission. Starting a little survey on #koha to get some more opinions.
(In reply to Blou from comment #49) I think from a usability/permissions/technical standpoint this should probably be considered a report. This script does not alter data, but only makes it accessible. This makes it technically a "report" to me. Furthermore, it seems likely that the librarians needing to access this data are the same ones who would run other reports, and not necessarily be the same ones running tools. This to me is another reason to move it back to the reports page.
Although viewiing the action logs is a tool, not a report. So I'm not sure this fits. Personnally, I view "reports" really as giving me a report. And reports do not have sub-permissions, really.
#koha voted for "tools". I will add new patch moving files under /tools, and add new permission for it.
Created attachment 50421 [details] [review] Bug 11317 - Move file access under Tools and add specific permission Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok.
THe little patch that could. Should cover all the points discussed EXCEPT the sha1 which I did not experience. I would have done a squash, but this little patch is easier to review. I *think* i covered all bases. Template, code, db... Hopefully. I hope...
Created attachment 50424 [details] [review] Bug 11317 - Move file access under Tools and add specific permission Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok. Sponsored by : CCSR
Created attachment 50425 [details] [review] Bug 11317 - Move file access under Tools and add specific permission Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok. Sponsored by : CCSR
Created attachment 50440 [details] [review] Bug 11317 - Allow sorting on date column in file access tool. For some reason, the table was blocking sorting based on timestamp, even though most users would probably want that since the feature is used primarily to access generated (daily) reports and finding the last ones is very important.
Created attachment 53446 [details] [review] Bug 11317 - Remove datatables-strings.inc from .tt
Tried to rebase it, but after a couple of apply --continue, I've lost track of where I was and where I should go :)
For easier testing, I'm resubmitting the whole shebang rebased on the current master. Patches to follow
Created attachment 58220 [details] [review] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 58221 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 58222 [details] [review] Bug 11317 - Rename config to 'accessdir' and fix qa Also translate date with datetime format preference and move to Tools. Test preparation : * Add the "accessdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/files-access" directory : <config> ... <accessdir>/tmp/files-access</accessdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/files-access * Add a test file in the log directory : $ cd /tmp/files-access $ echo "Hello world!" > test.txt Test : * Go in the "tools/Additional tools" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 58223 [details] [review] Bug 11317 - Allows for multiple directories to be accessible It is now possible to add as many <accessdir>SOMEDIR</accessdir> as needed to the config file. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 58224 [details] [review] Bug 11317 - Move file access under Tools and add specific permission Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok. Sponsored by : CCSR
Created attachment 58225 [details] [review] Bug 11317 - Allow sorting on date column in file access tool. For some reason, the table was blocking sorting based on timestamp, even though most users would probably want that since the feature is used primarily to access generated (daily) reports and finding the last ones is very important.
Created attachment 58226 [details] [review] Bug 11317 - Remove datatables-strings.inc from .tt
Sorry, this does no longer apply: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11317: Adds a page to access log files on the server from the intranet. Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. Applying: Bug 11317: Follow up - Update ID to allow for permalinking Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. Applying: Bug 11317 - Rename config to 'accessdir' and fix qa Applying: Bug 11317 - Allows for multiple directories to be accessible Applying: Bug 11317 - Move file access under Tools and add specific permission fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 11317 - Move file access under Tools and add specific permission The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-11317---Move-file-access-under-Tools-and-add-s-D7DJsU.patch
Created attachment 62971 [details] [review] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Sponsored by : CCSR Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62972 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62973 [details] [review] Bug 11317 - Rename config to 'accessdir' and fix qa Also translate date with datetime format preference and move to Tools. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62974 [details] [review] Bug 11317 - Allows for multiple directories to be accessible It is now possible to add as many <accessdir>SOMEDIR</accessdir> as needed to the config file. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62975 [details] [review] Bug 11317 - Move file access under Tools and add specific permission - Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. - Remove datatables-strings.inc from .tt - Allow sorting on date column in file access tool. For some reason, the table was blocking sorting based on timestamp, even though most users would probably want that since the feature is used primarily to access generated (daily) reports i and finding the last ones is very important. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok.
Rebased the (evil Sha1!!) patch . Squashed the unsigned patches (last three). Ready for take off.
Ok, trying to get this tested :) First: QA tools: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/report_files.tt FAIL git manipulation The file has been added and deleted in the same patchset FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt FAIL forbidden patterns forbidden pattern: tab char (line 55) forbidden pattern: tab char (line 68) forbidden pattern: tab char (line 63) forbidden pattern: tab char (line 61) forbidden pattern: tab char (line 64) forbidden pattern: tab char (line 58) forbidden pattern: tab char (line 65) forbidden pattern: tab char (line 73) forbidden pattern: tab char (line 59) forbidden pattern: tab char (line 57) forbidden pattern: tab char (line 60) FAIL reports/report_files.pl FAIL git manipulation The file has been added and deleted in the same patchset Easy to fix - as we got no sign-off yet, we'd just need to fix the tabs and squash the patches. Second: Some code review 1) I think as the permission is under Tools and the files live there too, it should also be shown as a tool on the tools page instead of under reports. I'd put it under "Additional tools", maybe under the log viewer. <li><a href="/cgi-bin/koha/reports/report_files.pl">Report/log files</a></li> 2) When I click on the link under Reports, I get a 'not found' error. What am I missing? 3) Looking at the history of this bug, are we missing 2 patches? Bug 11317 - Allow sorting on date column in file access tool. (1.29 KB, patch) Bug 11317 - Remove datatables-strings.inc from .tt (959 bytes, patch) 4) +use strict; +use warnings; Shoudl be: use Modern::Perl; Sorry, can't test further because of 2)!
Created attachment 64088 [details] [review] Bug 11317 - Update the link under reports
Created attachment 64089 [details] [review] Bug 11317 - Update the link under reports Signed-off-by: Paudet <philippe.audet-fortin@inlibro.com>
Should have been in Need Signoff for a month, now.
I am testing with a kohadevbox, but have problems to access the file, even though it is listed on the page. This is what I did: - Added the entry to koha-conf.xml, copied from the plugins/uploads default <accessdir>/var/lib/koha/kohadev/files</accessdir> - Created a file with runreport.pl and moved it to the files directory - The file is listed on the tool page - When I try to access the file, I get "internal server error" - The Plack error log shows: /home/vagrant/kohaclone/tools/access_files.pl line 119. I thought it might be permission related, but even with 777 the problem remains: -rwxrwxrwx 1 kohadev-koha kohadev-koha 592 Aug 8 2017 report1.csv While testing I've made a few little changes: - Fixed conflicts on reports-home.tt from adding/removing the link before moving it to tools - Slightly reworded second line on the tools page to be a bit more verbose. - Added entry to the tools-menu.inc. Attaching this as a follow-up for sign-off if you agree.
Created attachment 65691 [details] [review] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes - Fixed conflicts on reports-home.tt from adding/removing the link before moving it to tools - Slightly reworded second line on the tools page to be a bit more verbose. - Added entry to the tools-menu.inc.
Created attachment 65851 [details] [review] Bug 11317 - Move file access under Tools and add specific permission - Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. - Remove datatables-strings.inc from .tt - Allow sorting on date column in file access tool. For some reason, the table was blocking sorting based on timestamp, even though most users would probably want that since the feature is used primarily to access generated (daily) reports i and finding the last ones is very important. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok.
Philippe, can this be tested again?
Created attachment 65855 [details] [review] Bug 11317 - Update the link under reports
Created attachment 65856 [details] [review] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Created attachment 65857 [details] [review] Bug 11317 - Update the link under reports
The new attachment are there because I had sha1 error. I change nothing. However I don't see why you can't access files. Everything seem to work. I tried with different type of files and permissions and I can't replicate the error.
Philippe, you also killed my follow-up :(
Please: - Remove link from the tools page - Include link in the tools menu koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc
When you say remove the link from the tools page, do you mean report page?
Sorry, yes, that's what I meant!
Created attachment 65909 [details] [review] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Sponsored by : CCSR Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 65910 [details] [review] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 65911 [details] [review] Bug 11317 - Rename config to 'accessdir' and fix qa Also translate date with datetime format preference and move to Tools. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 65912 [details] [review] Bug 11317 - Allows for multiple directories to be accessible It is now possible to add as many <accessdir>SOMEDIR</accessdir> as needed to the config file. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 65913 [details] [review] Bug 11317 - Move file access under Tools and add specific permission - Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. - Remove datatables-strings.inc from .tt - Allow sorting on date column in file access tool. For some reason, the table was blocking sorting based on timestamp, even though most users would probably want that since the feature is used primarily to access generated (daily) reports i and finding the last ones is very important. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok.
Created attachment 65914 [details] [review] Bug 11317 - Update the link under reports
Created attachment 65915 [details] [review] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Created attachment 65918 [details] [review] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Sorry for the mess in the comment. I made the changes and now the patch apply without conflict.
Hi Phillipe, follow-up is looking good, but I still can't open or download the file. This is the error from the plack-error-log: exited nonzero: 1 at /home/vagrant/kohaclone/tools/access_files.pl line 120. Which is odd, as the file does end at line 107... :(
plack-error.log? Curious, do you have the issue when not using plack? Must be some kind of permission issue... :( Here it works like a charm. But Philippe will confirm himself.
I have tested with Plack deactivated and it does WORK. Using Plack is strongly recommende/standard setup now, so we really need it to work under Plack too. I am leaving this as Failed QA and hope that the new information will help you to fix it. We are close, please don't give up now. The QA tools have some complaints, maybe it would help to squash at least the patches by the same authors: OK etc/koha-conf.xml OK git manipulation OK xml_valid OK koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc OK forbidden patterns OK git manipulation OK spelling OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc OK forbidden patterns OK git manipulation OK spelling OK tt_valid OK valid_template FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/report_files.tt OK forbidden patterns FAIL git manipulation The file has been added and deleted in the same patchset OK spelling OK tt_valid OK valid_template FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt FAIL forbidden patterns forbidden pattern: tab char (line 55) forbidden pattern: tab char (line 59) forbidden pattern: tab char (line 65) forbidden pattern: tab char (line 63) forbidden pattern: tab char (line 61) forbidden pattern: tab char (line 64) forbidden pattern: tab char (line 57) forbidden pattern: tab char (line 60) forbidden pattern: tab char (line 58) forbidden pattern: tab char (line 68) forbidden pattern: tab char (line 72) OK git manipulation OK spelling OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/en/modules/tools/access_files.tt OK forbidden patterns OK git manipulation OK spelling OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt OK forbidden patterns OK git manipulation OK spelling OK tt_valid OK valid_template FAIL reports/report_files.pl OK critic OK forbidden patterns FAIL git manipulation The file has been added and deleted in the same patchset OK pod OK spelling OK valid OK tools/access_files.pl OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid
Global var in the BEGIN block might be the cause.
Created attachment 66091 [details] [review] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Created attachment 66092 [details] [review] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Created attachment 66093 [details] [review] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Hi Katrine, I found the problem. It didn't work with plack because when a file was download, access_files.pl exited with error flag even there had no error. It seem like plack doesn't love that.
Created attachment 66094 [details] [review] [SIGNED OFF] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Sponsored by : CCSR Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 66095 [details] [review] [SIGNED OFF] Bug 11317: Follow up - Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 66096 [details] [review] [SIGNED OFF] Bug 11317 - Rename config to 'accessdir' and fix qa Also translate date with datetime format preference and move to Tools. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 66097 [details] [review] [SIGNED OFF] Bug 11317 - Allows for multiple directories to be accessible It is now possible to add as many <accessdir>SOMEDIR</accessdir> as needed to the config file. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 66098 [details] [review] [SIGNED OFF] Bug 11317 - Move file access under Tools and add specific permission - Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. - Remove datatables-strings.inc from .tt - Allow sorting on date column in file access tool. For some reason, the table was blocking sorting based on timestamp, even though most users would probably want that since the feature is used primarily to access generated (daily) reports i and finding the last ones is very important. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 66099 [details] [review] [SIGNED OFF] Bug 11317 - Update the link under reports Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 66100 [details] [review] [SIGNED OFF] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Sorry, I see it is long process to get this to Koha, but I have one more concern: If there are files with the same name in multiple directories, they have the same hash! Than only one of them is always downloaded, although both (or more) are listed. The path of each file should be presented in the table too.
Created attachment 72097 [details] [review] Bug 11317 - File with same name in different directories now has distinct hash
Forgot to add test plan... Here we go : - Generate file hash based on fullpath + filename. This make sure all files are accessible, even if they have the same filename. Testing: 0. Make sure the requirements from the last patches are met 1. Add 2 accessdir in your koha-conf, i.e. "/tmp/test" and "/tmp/moretest". 2. Add a file in both directories using the same name for the file: - echo "small file" > /tmp/test/log - echo "the size of this file is quite huge" > /tmp/moretest/log 3. Go to Home › Tools › Report/log files and you should see both files with different size. 4. Download (or open) both files. They should be different. Thanks for testing
Sorry, still some issues found with this: 1) Please undo the changes of spaces to tabs in reports-home.tt - there is no reason to touch these lines and tab char is forbidden 2) Move javascript to the end of page - see JS12 rule: https://wiki.koha-community.org/wiki/Coding_Guidelines#JS12:_Include_javascript_at_the_end_of_template 3) Please add _[% KOHA_VERSION %] to css path, see bug 12904
Created attachment 72309 [details] [review] Bug 11317 - File with same name in different directories now has distinct hash + QA followup Testing: 0. Make sure the requirements from the last patches are met 1. Add 2 accessdir in your koha-conf, i.e. "/tmp/test" and "/tmp/moretest". 2. Add a file in both directories using the same name for the file: - echo "small file" > /tmp/test/log - echo "the size of this file is quite huge" > /tmp/moretest/log 3. Go to Home › Tools › Report/log files and you should see both files with different size. 4. Download (or open) both files. They should be different. Thanks for testing
Created attachment 72456 [details] [review] Bug 11317: Adds a page to access log files on the server from the intranet. The directory used is defined by the "publiclogdir" preference in koha-conf.xml. Test case : Preparation : * Add the "publiclogdir" preference to your "koha-conf.xml" file. This * preference should be in the "config" section. For this test cas, we * will use the "/tmp/koha-public" directory : <config> ... <publiclogdir>/tmp/koha-public</publiclogdir> ... </config> * Create the log directory on your server. Make sure this directory * is readable by your web server : $ mkdir /tmp/koha-public * Add a test file in the log directory : $ cd /tmp/koha-public $ echo "Hello world!" > test.txt Test : * Go in the "Reports" section in the intranet and select "Report/log * files" * The "test.txt" should appear in the files list. * Click on the "test.txt" file, a download dialog should appear. * Download the file and verify its content. It should contain the * text "Hello world!". Sponsored by : CCSR Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72457 [details] [review] Bug 11317: Update ID to allow for permalinking This follow up modifies the id parameter to use a digest of the filename to enable permalinking to files even if the array order changes due to new files being created. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72458 [details] [review] Bug 11317: Rename config to 'accessdir' and fix qa Also translate date with datetime format preference and move to Tools. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72459 [details] [review] Bug 11317: Allows for multiple directories to be accessible It is now possible to add as many <accessdir>SOMEDIR</accessdir> as needed to the config file. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72460 [details] [review] Bug 11317: Move file access under Tools and add specific permission - Moves the .pl and .tt under tools/ instead of reports/ and adds tools/access_files permission. - Remove datatables-strings.inc from .tt - Allow sorting on date column in file access tool. For some reason, the table was blocking sorting based on timestamp, even though most users would probably want that since the feature is used primarily to access generated (daily) reports i and finding the last ones is very important. Testing -1) Having done requirements of previous patches, like adding 'accessdir' to koha-conf... 0) apply patch, update database to add permission option. 1) Login as NON-superadmin. Go under tools, see no Report/Log under the third column 2) Go to add tools/access_file permission to user 3) See new entry under tools third column. 4) validate link is ok. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72461 [details] [review] Bug 11317: Update the link under reports Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72462 [details] [review] Bug 11317: (Follow-up) Fixing merge error and cosmetic changes Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72463 [details] [review] Bug 11317: (Follow-up) File with same name in different directories now has distinct hash Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72464 [details] [review] Bug 11317: (Follow-up) Fix tab chars and move javascript to the footer Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72465 [details] [review] Bug 11317: (QA follow-up) Fix datatable Fixes the link to datatables.css Fixes the footer javascript Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
1. Not unicode proof, create a file named "❤" in your accessdir and boom: Can't call method "mtime" on an undefined value at /home/vagrant/kohaclone/tools/access_files.pl line 61. 2. Please rename accessdir with access_dir 3. Can we get an up-to-date and complete test plan? 4. Why do not we have a default value for accessdir? 5. What is the main purpose of this development? How do we want to advert it? Still log files? 6. Did someone test with very big files (see bug 8437)? Should we put a limit?
The use case for us would be to allow access to files generated on the server - not logs, but print notices. They contain patron information, so you need to secure them behind login.
This is about to miss another release again :( Can we get some discussion going based on RM feedback?
Working on this and wondering what could be a good default value for accessdir? Also, renaming accessdir to access_dir make sense to me, I will include this change in my fix to make this feature unicode proof.
Created attachment 74119 [details] [review] Bug 11317: Make filename unicode-proof, renamed accessdir to access_dir and fix db update This patch does many small things : - Added "INSERT IGNORE" in the atomic update - Renamed accessdir to access_dir - Now decode the filename to utf8 to make sure the stat() method point at it. Test plan: 1. Define <access_dir>/tmp/koha-public</access_dir> in your koha_conf 2. change dir to /tmp/koha-public 3. create a file with a normal name, i.e. echo "bonjour koha" > testfile.txt 4. create a file with a name containing a unicode character, i.e. touch hi❤.txt 5. Go in the "Reports" section in the intranet and select "Report/log files" 6. Verify that the filename "hi❤.txt" looks fine 7. Click on the "testfile.txt" file, a download dialog should appear. 8. Download the file and verify its content. It should contain the text "bonjour koha". Sponsored by : CCSR
I am very sorry, this doesn't apply for me. I tried to fix it, but I got stuck with the last 3 patches: Applying: Bug 11317: (Follow-up) Fix tab chars and move javascript to the footer error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt). error: could not build fake ancestor Patch failed at 0001 Bug 11317: (Follow-up) Fix tab chars and move javascript to the footer The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am -i --continue". If you prefer to skip this patch, run "git am -i --skip" instead. To restore the original branch and stop patching, run "git am -i --abort". Please also note that bug 20538 will require some changes, but they should be fast! For the path, I am no expert. Maybe lean on the sample paths for uploaded files or plugins?
Created attachment 74379 [details] [review] Bug 11317: Squash attachements to clean and fix merge errors: Add a way to access files from the intranet This squash contains all of these commits: - Adds a page to access log files on the server from the intranet - Update ID to allow for permalinking - Rename config to "'accessdir' and fix qa - Allows for multiple directories to be accessible - Update the link under reports - (Follow-up) Fixing merge error and cosmetic changes - (Follow-up) Fix tab chars and move javascript to the footer - (QA Follow-up) Fix datatable - Make filename unicode-proof, renamed accessdir to access_dir and fix update Test plans: - Apply patch, update database - Add to koha-conf: <access_dir>/tmp/koha-public/one</access_dir> <access_dir>/tmp/koha-public/two</access_dir> <access_dir>/tmp/koha-public</access_dir> - Create these directories ( mkdir /tmp/koha-public , etc...) - Create these files: echo "hello world!" > /tmp/koha-public/❤ echo "test" > /tmp/koha-public/one/samename.txt echo "this is not the same" > /tmp/koha-public/two/samename.txt - Login as Superadmin, go to tools > reports files - Click on ❤, make sure it's downloadable and readable - Click on both samename.txt, look inside and make sure the file is different - Login as NON-superadmin. Go under tools, see no Report/Log under the third column - Go to add tools/access_file permission to user - See new entry under tools third column. - validate link is ok.
(In reply to Roch D'Amour from comment #134) > Working on this and wondering what could be a good default value for > accessdir? > > Also, renaming accessdir to access_dir make sense to me, I will include this > change in my fix to make this feature unicode proof. This configuration entry should default to empty. The patch is missing to add the entry in debian/templates/koha-conf-site.xml.in
Roch, just something I noticed on another Inlibro bug earlier already: Please provide fixes that QA or RM ask for as separate follow-up patches. This way you can keep the sign-off line on the initial patch. If not told otherwise it's also ok to switch back to Signed-off.
Please fix the errors from the QA script and add the changes to debian/templates/koha-conf-site.xml.in FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt FAIL forbidden patterns forbidden pattern: merge marker (<<<<<<<) (line 66) OK git manipulation OK spelling OK tt_valid OK valid_template FAIL koha-tmpl/intranet-tmpl/prog/en/modules/tools/access_files.tt FAIL forbidden patterns forbidden pattern: Do *not* include full path to css files, use the Asset TT plugin (bug 20538) (line 5) forbidden pattern: Do *not* include full path to js files, use the Asset TT plugin (bug 20538) (line 55) OK git manipulation OK spelling OK tt_valid OK valid_template
(In reply to Tomás Cohen Arazi from comment #138) > This configuration entry should default to empty. > > The patch is missing to add the entry in > debian/templates/koha-conf-site.xml.in I don't think I understood clearly. When I look in koha-conf-site.xml.in, I have: <!-- Accessible directory from the staff client, uncomment the following line and define a valid path to let the intranet user access it--> <!-- <access_dir></access_dir> --> Should I uncomment this line?
Created attachment 74756 [details] [review] Bug 11317: (follow-up) Add two-level entry to koha-conf and remove unwanted characters - Removed merge marker - Changed include path in favor of using the Asset tt plugin (bug 20538) - Changed access_dir to a two-level entry for clarity Test plans stay the same, just make sure that the two-level configuration entry work properly and everything pass QA.
Created attachment 74911 [details] [review] Bug 11317: Squash attachements to clean and fix merge errors: Add a way to access files from the intranet This squash contains all of these commits: - Adds a page to access log files on the server from the intranet - Update ID to allow for permalinking - Rename config to "'accessdir' and fix qa - Allows for multiple directories to be accessible - Update the link under reports - (Follow-up) Fixing merge error and cosmetic changes - (Follow-up) Fix tab chars and move javascript to the footer - (QA Follow-up) Fix datatable - Make filename unicode-proof, renamed accessdir to access_dir and fix update Test plans: - Apply patch, update database - Add to koha-conf: <access_dir>/tmp/koha-public/one</access_dir> <access_dir>/tmp/koha-public/two</access_dir> <access_dir>/tmp/koha-public</access_dir> - Create these directories ( mkdir /tmp/koha-public , etc...) - Create these files: echo "hello world!" > /tmp/koha-public/❤ echo "test" > /tmp/koha-public/one/samename.txt echo "this is not the same" > /tmp/koha-public/two/samename.txt - Login as Superadmin, go to tools > reports files - Click on ❤, make sure it's downloadable and readable - Click on both samename.txt, look inside and make sure the file is different - Login as NON-superadmin. Go under tools, see no Report/Log under the third column - Go to add tools/access_file permission to user - See new entry under tools third column. - validate link is ok. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 74912 [details] [review] Bug 11317: (follow-up) Add two-level entry to koha-conf and remove unwanted characters - Removed merge marker - Changed include path in favor of using the Asset tt plugin (bug 20538) - Changed access_dir to a two-level entry for clarity Test plans stay the same, just make sure that the two-level configuration entry work properly and everything pass QA. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 74916 [details] [review] Bug 11317: Add a way to access files from the intranet This squash contains all of these commits: - Adds a page to access log files on the server from the intranet - Update ID to allow for permalinking - Rename config to "'accessdir' and fix qa - Allows for multiple directories to be accessible - Update the link under reports - (Follow-up) Fixing merge error and cosmetic changes - (Follow-up) Fix tab chars and move javascript to the footer - (QA Follow-up) Fix datatable - Make filename unicode-proof, renamed accessdir to access_dir and fix update Test plans: - Apply patch, update database - Add to koha-conf: <access_dir>/tmp/koha-public/one</access_dir> <access_dir>/tmp/koha-public/two</access_dir> <access_dir>/tmp/koha-public</access_dir> - Create these directories ( mkdir /tmp/koha-public , etc...) - Create these files: echo "hello world!" > /tmp/koha-public/❤ echo "test" > /tmp/koha-public/one/samename.txt echo "this is not the same" > /tmp/koha-public/two/samename.txt - Login as Superadmin, go to tools > reports files - Click on ❤, make sure it's downloadable and readable - Click on both samename.txt, look inside and make sure the file is different - Login as NON-superadmin. Go under tools, see no Report/Log under the third column - Go to add tools/access_file permission to user - See new entry under tools third column. - validate link is ok. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74917 [details] [review] Bug 11317: (follow-up) Add two-level entry to koha-conf and remove unwanted characters - Removed merge marker - Changed include path in favor of using the Asset tt plugin (bug 20538) - Changed access_dir to a two-level entry for clarity Test plans stay the same, just make sure that the two-level configuration entry work properly and everything pass QA. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74918 [details] [review] Bug 11317: (QA follow-up) Fix merge error and punctuation 2 tiny fixes: - Remove space before : as this is not correct in English - Remove an errorenous commented line that probably snuck back in from a merge
Created attachment 74919 [details] [review] Bug 11317: (QA follow-up) Change tool name to be more general I think this tool could be used in multiple ways, for all kinds of files that you want to protect by login in Koha. So a more general name might work better. This is a suggestion, not mandatory.
Pushed to master for 18.05, thanks to everybody involved!
Created attachment 75019 [details] [review] Bug 11317: Add id and class attributes to body This fixes xt/tt_valid.t # Failed test '<body> tag with id and class attributes' # at xt/tt_valid.t line 91 # Files list: # intranet-tmpl/prog/en/modules/tools/access_files.tt: 8 # Looks like you failed 1 test of 3. [16:44:56] xt/tt_valid.t Dubious, test returned 1 (wstat 256, 0x100) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>