Bug 11317 - Add a way to access files from the intranet
Summary: Add a way to access files from the intranet
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Blou
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 5771
  Show dependency treegraph
 
Reported: 2013-11-27 22:03 UTC by Frédérick Capovilla
Modified: 2017-08-16 14:45 UTC (History)
15 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off: Jonathan Druart
Text to go in the release notes:


Attachments
Adds a page to access log files on the server from the intranet. (6.83 KB, patch)
2013-11-27 22:03 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
New version of the patch, now using datatables (7.20 KB, patch)
2014-02-18 17:01 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (1.92 KB, patch)
2014-04-23 14:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (1.95 KB, patch)
2014-04-25 06:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (1.95 KB, patch)
2014-04-25 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11317: Adds a page to access log files on the server from the intranet. (8.04 KB, patch)
2015-02-16 15:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (2.01 KB, patch)
2015-02-16 15:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11317: Rename publiclogdir with publicdir (3.99 KB, patch)
2015-02-16 15:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11317: Correct critic, forbidden patterns issues when tested with koha-test-tools (4.21 KB, patch)
2015-03-12 19:09 UTC, Charles Farmer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11317: Adds a page to access log files on the server from the intranet. (8.10 KB, patch)
2015-06-02 18:08 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11317: Follow up - Update ID to allow for permalinking (2.08 KB, patch)
2015-06-02 18:08 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11317: Rename publiclogdir with publicdir (3.94 KB, patch)
2015-06-02 18:08 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11317: Correct critic, forbidden patterns issues when tested with koha-test-tools (4.34 KB, patch)
2015-06-02 18:08 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Test case : (4.97 KB, patch)
2015-11-13 21:18 UTC, Bouzid
Details | Diff | Splinter Review
Bug 11317: translate date with datetime format preference,replace in tools and rename preference in Koha-config.xml. Test case : (5.08 KB, patch)
2015-11-13 21:29 UTC, Bouzid
Details | Diff | Splinter Review
Bug 11317 - Rename config to 'accessdir' and fix qa (9.30 KB, patch)
2016-03-03 15:30 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Allows for multiple directories to be accessible (4.07 KB, patch)
2016-03-03 16:38 UTC, Blou
Details | Diff | Splinter Review
Bug 11317: Adds a page to access log files on the server from the intranet. (8.14 KB, patch)
2016-04-06 01:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (2.12 KB, patch)
2016-04-06 01:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11317 - Rename config to 'accessdir' and fix qa (9.37 KB, patch)
2016-04-06 01:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11317: Adds a page to access log files on the server from the intranet. (8.14 KB, patch)
2016-04-06 01:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (2.12 KB, patch)
2016-04-06 01:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11317 - Rename config to 'accessdir' and fix qa (9.37 KB, patch)
2016-04-06 01:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11317 - Allows for multiple directories to be accessible (4.13 KB, patch)
2016-04-06 01:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11317 - Move file access under Tools and add specific permission (6.58 KB, patch)
2016-04-19 15:06 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Move file access under Tools and add specific permission (10.24 KB, patch)
2016-04-19 15:18 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Move file access under Tools and add specific permission (7.23 KB, patch)
2016-04-19 15:21 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Allow sorting on date column in file access tool. (1.29 KB, patch)
2016-04-19 20:31 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Remove datatables-strings.inc from .tt (959 bytes, patch)
2016-07-15 15:42 UTC, Blou
Details | Diff | Splinter Review
Bug 11317: Adds a page to access log files on the server from the intranet. (9.96 KB, patch)
2016-12-15 17:10 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (2.12 KB, patch)
2016-12-15 17:10 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11317 - Rename config to 'accessdir' and fix qa (8.80 KB, patch)
2016-12-15 17:10 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11317 - Allows for multiple directories to be accessible (4.13 KB, patch)
2016-12-15 17:10 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11317 - Move file access under Tools and add specific permission (7.25 KB, patch)
2016-12-15 17:11 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11317 - Allow sorting on date column in file access tool. (1.29 KB, patch)
2016-12-15 17:11 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11317 - Remove datatables-strings.inc from .tt (959 bytes, patch)
2016-12-15 17:11 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11317: Adds a page to access log files on the server from the intranet. (9.98 KB, patch)
2017-05-02 15:06 UTC, Blou
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (2.12 KB, patch)
2017-05-02 15:06 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Rename config to 'accessdir' and fix qa (8.02 KB, patch)
2017-05-02 15:07 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Allows for multiple directories to be accessible (4.13 KB, patch)
2017-05-02 15:07 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Move file access under Tools and add specific permission (8.49 KB, patch)
2017-05-02 15:07 UTC, Blou
Details | Diff | Splinter Review
Bug 11317 - Update the link under reports (2.20 KB, patch)
2017-06-07 19:47 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Update the link under reports (3.02 KB, patch)
2017-06-07 19:56 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes (3.65 KB, patch)
2017-08-08 20:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11317 - Move file access under Tools and add specific permission (8.49 KB, patch)
2017-08-10 19:33 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Update the link under reports (1.69 KB, patch)
2017-08-10 20:01 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes (1.55 KB, patch)
2017-08-10 20:01 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Update the link under reports (1.69 KB, patch)
2017-08-10 20:29 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317: Adds a page to access log files on the server from the intranet. (9.87 KB, patch)
2017-08-11 15:39 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317: Follow up - Update ID to allow for permalinking (2.12 KB, patch)
2017-08-11 15:39 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Rename config to 'accessdir' and fix qa (8.02 KB, patch)
2017-08-11 15:39 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Allows for multiple directories to be accessible (4.13 KB, patch)
2017-08-11 15:39 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Move file access under Tools and add specific permission (8.49 KB, patch)
2017-08-11 15:40 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Update the link under reports (1.69 KB, patch)
2017-08-11 15:40 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes (1.55 KB, patch)
2017-08-11 15:40 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes (2.46 KB, patch)
2017-08-11 15:43 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes (2.46 KB, patch)
2017-08-16 14:08 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes (2.46 KB, patch)
2017-08-16 14:09 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes (2.82 KB, patch)
2017-08-16 14:15 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11317: Adds a page to access log files on the server from the intranet. (9.94 KB, patch)
2017-08-16 14:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11317: Follow up - Update ID to allow for permalinking (2.19 KB, patch)
2017-08-16 14:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11317 - Rename config to 'accessdir' and fix qa (8.09 KB, patch)
2017-08-16 14:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11317 - Allows for multiple directories to be accessible (4.20 KB, patch)
2017-08-16 14:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11317 - Move file access under Tools and add specific permission (8.56 KB, patch)
2017-08-16 14:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11317 - Update the link under reports (1.76 KB, patch)
2017-08-16 14:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11317 - Follow-up - Fixing merge error and cosmetic changes (2.89 KB, patch)
2017-08-16 14:43 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédérick Capovilla 2013-11-27 22:03:06 UTC Comment hidden (obsolete)
Comment 1 Jonathan Druart 2014-01-27 11:18:23 UTC
Frédérick,

Could you provide a test plan and replace tablesorter with datatables please?

Marked as Failed QA.
Comment 2 Frédérick Capovilla 2014-02-18 17:01:52 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2014-02-24 10:18:10 UTC
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.)
Comment 4 Martin Renvoize 2014-04-23 13:34:53 UTC
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.
Comment 5 Katrin Fischer 2014-04-23 13:40:35 UTC
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.
Comment 6 Martin Renvoize 2014-04-23 14:53:41 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize 2014-04-23 14:56:02 UTC
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.
Comment 8 Chris Cormack 2014-04-24 15:24:34 UTC
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.
Comment 9 Martin Renvoize 2014-04-25 06:37:25 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2014-04-25 06:39:47 UTC
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?
Comment 11 Jonathan Druart 2014-04-25 09:56:34 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2014-04-25 10:02:10 UTC
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..
Comment 13 Jonathan Druart 2015-02-16 15:03:27 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-02-16 15:03:58 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-02-16 15:04:03 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-02-16 15:04:43 UTC
I suggest to rename publiclogdir with publicdir. Feel free so obsolete the patch if you disagree.
Comment 17 Kyle M Hall 2015-02-20 14:30:53 UTC
 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)
Comment 18 Charles Farmer 2015-03-12 19:09:15 UTC Comment hidden (obsolete)
Comment 19 M. Tompsett 2015-05-09 15:54:16 UTC
How does this relate to something like bug 6874? :)
Comment 20 Bernardo Gonzalez Kriegel 2015-06-02 18:08:32 UTC Comment hidden (obsolete)
Comment 21 Bernardo Gonzalez Kriegel 2015-06-02 18:08:40 UTC Comment hidden (obsolete)
Comment 22 Bernardo Gonzalez Kriegel 2015-06-02 18:08:47 UTC Comment hidden (obsolete)
Comment 23 Bernardo Gonzalez Kriegel 2015-06-02 18:08:54 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2015-06-26 16:29:35 UTC
Please provide a test plan.
Comment 25 M. Tompsett 2015-06-26 16:38:44 UTC
(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.
Comment 26 Maxime Beaulieu 2015-07-07 15:33:12 UTC
(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!".
Comment 27 Martin Renvoize 2015-10-21 09:24:50 UTC
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)?
Comment 28 Katrin Fischer 2015-10-21 09:41:23 UTC
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.
Comment 29 Martin Renvoize 2015-10-22 06:53:15 UTC
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
Comment 30 Katrin Fischer 2015-10-22 22:06:29 UTC
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?
Comment 31 Blou 2015-11-13 20:53:20 UTC
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.
Comment 32 Bouzid 2015-11-13 21:18:05 UTC Comment hidden (obsolete)
Comment 33 Bouzid 2015-11-13 21:29:51 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2015-11-22 21:11:46 UTC
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.
Comment 35 Blou 2016-03-03 15:30:02 UTC Comment hidden (obsolete)
Comment 36 Blou 2016-03-03 15:33:56 UTC
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...
Comment 37 Blou 2016-03-03 16:38:59 UTC Comment hidden (obsolete)
Comment 38 Nick Clemens 2016-04-06 01:14:00 UTC Comment hidden (obsolete)
Comment 39 Nick Clemens 2016-04-06 01:14:06 UTC Comment hidden (obsolete)
Comment 40 Nick Clemens 2016-04-06 01:14:11 UTC Comment hidden (obsolete)
Comment 41 Nick Clemens 2016-04-06 01:15:05 UTC
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>
Comment 42 Nick Clemens 2016-04-06 01:15:13 UTC
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>
Comment 43 Nick Clemens 2016-04-06 01:15:21 UTC
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>
Comment 44 Nick Clemens 2016-04-06 01:15:30 UTC
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>
Comment 45 Nick Clemens 2016-04-06 01:17:04 UTC
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
Comment 46 Katrin Fischer 2016-04-06 06:12:47 UTC
Hi Nick, that qa test problem will be fixed by another patch soon. Is this otherwise ok?
Comment 47 Nick Clemens 2016-04-06 09:18:58 UTC
(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
Comment 48 Marcel de Rooy 2016-04-08 07:02:17 UTC
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.
Comment 49 Blou 2016-04-19 12:15:39 UTC
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.
Comment 50 Katrin Fischer 2016-04-19 12:32:09 UTC
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.
Comment 51 Kyle M Hall 2016-04-19 12:39:48 UTC
(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.
Comment 52 Blou 2016-04-19 12:45:09 UTC
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.
Comment 53 Blou 2016-04-19 13:04:35 UTC
#koha voted for "tools".  I will add new patch moving files under /tools, and add new permission for it.
Comment 54 Blou 2016-04-19 15:06:22 UTC Comment hidden (obsolete)
Comment 55 Blou 2016-04-19 15:09:06 UTC
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...
Comment 56 Blou 2016-04-19 15:18:26 UTC Comment hidden (obsolete)
Comment 57 Blou 2016-04-19 15:21:08 UTC Comment hidden (obsolete)
Comment 58 Blou 2016-04-19 20:31:39 UTC Comment hidden (obsolete)
Comment 59 Blou 2016-07-15 15:42:41 UTC Comment hidden (obsolete)
Comment 60 Nicolas Legrand 2016-10-12 12:21:43 UTC Comment hidden (obsolete)
Comment 61 Charles Farmer 2016-12-15 17:06:20 UTC
For easier testing, I'm resubmitting the whole shebang rebased on the current master.

Patches to follow
Comment 62 Charles Farmer 2016-12-15 17:10:31 UTC
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>
Comment 63 Charles Farmer 2016-12-15 17:10:42 UTC
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>
Comment 64 Charles Farmer 2016-12-15 17:10:48 UTC
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>
Comment 65 Charles Farmer 2016-12-15 17:10:55 UTC
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>
Comment 66 Charles Farmer 2016-12-15 17:11:00 UTC
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
Comment 67 Charles Farmer 2016-12-15 17:11:06 UTC
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.
Comment 68 Charles Farmer 2016-12-15 17:11:12 UTC
Created attachment 58226 [details] [review]
Bug 11317 - Remove datatables-strings.inc from .tt
Comment 69 Katrin Fischer 2017-05-01 23:53:33 UTC
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
Comment 70 Blou 2017-05-02 15:06:53 UTC
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>
Comment 71 Blou 2017-05-02 15:06:58 UTC
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>
Comment 72 Blou 2017-05-02 15:07:03 UTC
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>
Comment 73 Blou 2017-05-02 15:07:08 UTC
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>
Comment 74 Blou 2017-05-02 15:07:12 UTC
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.
Comment 75 Blou 2017-05-02 15:13:49 UTC
Rebased the (evil Sha1!!) patch .  Squashed the unsigned patches (last three).  Ready for take off.
Comment 76 Katrin Fischer 2017-05-25 19:21:50 UTC
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)!
Comment 77 Philippe Audet-Fortin 2017-06-07 19:47:15 UTC
Created attachment 64088 [details] [review]
Bug 11317 - Update the link under reports
Comment 78 Philippe Audet-Fortin 2017-06-07 19:56:53 UTC
Created attachment 64089 [details] [review]
Bug 11317 - Update the link under reports

Signed-off-by: Paudet <philippe.audet-fortin@inlibro.com>
Comment 79 Blou 2017-07-03 16:52:42 UTC
Should have been in Need Signoff for a month, now.
Comment 80 Katrin Fischer 2017-08-08 20:49:28 UTC
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.
Comment 81 Katrin Fischer 2017-08-08 20:51:48 UTC
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.
Comment 82 Philippe Audet-Fortin 2017-08-10 19:33:02 UTC
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.
Comment 83 Katrin Fischer 2017-08-10 19:51:10 UTC
Philippe, can this be tested again?
Comment 84 Philippe Audet-Fortin 2017-08-10 20:01:14 UTC Comment hidden (obsolete)
Comment 85 Philippe Audet-Fortin 2017-08-10 20:01:44 UTC
Created attachment 65856 [details] [review]
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Comment 86 Philippe Audet-Fortin 2017-08-10 20:29:45 UTC
Created attachment 65857 [details] [review]
Bug 11317 - Update the link under reports
Comment 87 Philippe Audet-Fortin 2017-08-10 20:37:30 UTC
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.
Comment 88 Katrin Fischer 2017-08-10 20:41:54 UTC
Philippe, you also killed my follow-up :(
Comment 89 Katrin Fischer 2017-08-10 21:31:49 UTC
Please:
- Remove link from the tools page
- Include link in the tools menu koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc
Comment 90 Philippe Audet-Fortin 2017-08-11 13:21:55 UTC
When you say remove the link from the tools page, do you mean report page?
Comment 91 Katrin Fischer 2017-08-11 14:37:57 UTC
Sorry, yes, that's what I meant!
Comment 92 Philippe Audet-Fortin 2017-08-11 15:39:33 UTC
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>
Comment 93 Philippe Audet-Fortin 2017-08-11 15:39:41 UTC
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>
Comment 94 Philippe Audet-Fortin 2017-08-11 15:39:47 UTC
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>
Comment 95 Philippe Audet-Fortin 2017-08-11 15:39:55 UTC
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>
Comment 96 Philippe Audet-Fortin 2017-08-11 15:40:02 UTC
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.
Comment 97 Philippe Audet-Fortin 2017-08-11 15:40:10 UTC
Created attachment 65914 [details] [review]
Bug 11317 - Update the link under reports
Comment 98 Philippe Audet-Fortin 2017-08-11 15:40:16 UTC
Created attachment 65915 [details] [review]
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Comment 99 Philippe Audet-Fortin 2017-08-11 15:43:14 UTC
Created attachment 65918 [details] [review]
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Comment 100 Philippe Audet-Fortin 2017-08-11 15:47:18 UTC
Sorry for the mess in the comment. 
I made the changes and now the patch apply without conflict.
Comment 101 Katrin Fischer 2017-08-12 10:07:28 UTC
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... :(
Comment 102 Blou 2017-08-14 12:11:07 UTC
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.
Comment 103 Katrin Fischer 2017-08-15 14:13:08 UTC
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
Comment 104 Katrin Fischer 2017-08-15 15:21:17 UTC
Global var in the BEGIN block might be the cause.
Comment 105 Philippe Audet-Fortin 2017-08-16 14:08:36 UTC Comment hidden (obsolete)
Comment 106 Philippe Audet-Fortin 2017-08-16 14:09:50 UTC Comment hidden (obsolete)
Comment 107 Philippe Audet-Fortin 2017-08-16 14:15:54 UTC
Created attachment 66093 [details] [review]
Bug 11317 - Follow-up - Fixing merge error and cosmetic changes
Comment 108 Philippe Audet-Fortin 2017-08-16 14:22:55 UTC
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.
Comment 109 Katrin Fischer 2017-08-16 14:42:46 UTC
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>
Comment 110 Katrin Fischer 2017-08-16 14:42:52 UTC
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>
Comment 111 Katrin Fischer 2017-08-16 14:42:58 UTC
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>
Comment 112 Katrin Fischer 2017-08-16 14:43:04 UTC
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>
Comment 113 Katrin Fischer 2017-08-16 14:43:10 UTC
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>
Comment 114 Katrin Fischer 2017-08-16 14:43:15 UTC
Created attachment 66099 [details] [review]
[SIGNED OFF] Bug 11317 - Update the link under reports

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 115 Katrin Fischer 2017-08-16 14:43:21 UTC
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>