This feature is not available in current stable release 3.6

I was just asked for this feature in a webinar today - so it's still desired.

Nicole

Comment on attachment 25306 [details] [review]
[SIGNED-OFF] Bug 3050 - Add an option to upload scanned invoices

Review of attachment 25306 [details] [review]:
-----------------------------------------------------------------

QA comments:

1/ A couple of errors are raised by the qa script:
 FAIL	Koha/Misc/Files.pm
   FAIL	  pod
		*** WARNING: Verbatim paragraph in NAME section  in file Koha/Misc/Files.pm
		*** ERROR: =item without previous =over  in file Koha/Misc/Files.pm

 FAIL	acqui/invoice-files.pl
   FAIL	  forbidden patterns
		forbidden pattern: trailing space char (line 130)

2/ The new module needs unit tests.

3/ And the following:

::: C4/Auth.pm
@@ +358,4 @@
>              EnableBorrowerFiles         => C4::Context->preference('EnableBorrowerFiles'),
>              UseKohaPlugins              => C4::Context->preference('UseKohaPlugins'),
>              UseCourseReserves            => C4::Context->preference("UseCourseReserves"),
> +            AcqEnableFiles              => C4::Context->preference('AcqEnableFiles'),

You should check the syspref value in the templates using Koha.Preference('AcqEnableFiles')

::: Koha/Misc/Files.pm
@@ +19,5 @@
> +# You should have received a copy of the GNU General Public License
> +# along with Koha; if not, see <http://www.gnu.org/licenses>.
> +
> +use Modern::Perl;
> +use strict; ## not needed if Modern::Perl, but perlcritic complains..

No complain from perlcritic if you define
  [TestingAndDebugging::RequireUseStrict]
  equivalent_modules = Modern::Perl
in your .perlcriticrc
Take a look at the perlcriticrc file in the qa-tools.

@@ +70,5 @@
> +            date_uploaded
> +        FROM misc_files
> +        WHERE table_tag = ? AND record_id = ?
> +        ORDER BY file_name, date_uploaded
> +    ";

Replace quotes with simple quotes around sql queries.

::: acqui/invoice-files.pl
@@ +28,5 @@
> +=cut
> +
> +use strict;
> +use warnings;
> +

use Modern::Perl;

@@ +40,5 @@
> +
> +my $input = new CGI;
> +my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user(
> +    {
> +        template_name   => 'acqui/invoice-files.tmpl',

s/tmpl/tt

@@ +61,5 @@
> +    my $file = $mf->GetFile( id => $file_id );
> +
> +    my $fname = $file->{'file_name'};
> +    my $ftype = $file->{'file_type'};
> +    if ($input->param('view') && ($ftype =~ /^image\//i || $fname =~ /\.pdf/i)) {

Maybe m|^image/|i is more readable.

@@ +103,5 @@
> +            else {
> +                my $file_content;
> +                while (<$uploaded_file>) {
> +                    $file_content .= $_;
> +                }

I think
  my $file_content = <$uploaded_file>;
does the same thing.

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice-files.tt
@@ +27,5 @@
> +                    </div>
> +                [% END %]
> +
> +                [% IF ( files ) %]
> +                <table>

Please use Datatable for this table.

@@ +64,5 @@
> +                        <legend>Upload New File</legend>
> +                        <ol>
> +                        <li><input type="hidden" name="op" value="upload" />
> +                        <input type="hidden" name="invoiceid" value="[% invoiceid %]" />
> +                        <input type="hidden" name="MAX_FILE_SIZE" value="9000000" />

This variable is not retrieve in the pl file. Is it useless?

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt
@@ +172,5 @@
>          [% END %]
> +        [% IF ( AcqEnableFiles && files ) %]
> +            <br>
> +            <h2>Files attached to invoice</h2>
> +            <table>

Please use Datatable for this table.

(In reply to Jonathan Druart from comment #5)

> Review of attachment 25306 [details] [review] [review]:
> -----------------------------------------------------------------

> 
>  FAIL	acqui/invoice-files.pl
>    FAIL	  forbidden patterns
> 		forbidden pattern: trailing space char (line 130)

I was unable to locate that trailing space[s] in line 130, but this part of the code was rewrited anyway, so this issue should now be fixed.

> You should check the syspref value in the templates using
> Koha.Preference('AcqEnableFiles')

Btw, this nice little TT plugin doesn't seem to utf8-encode it's output, unless I'm very much mistaken.. Not really a problem with 0|1 kind of a syspref, though.

> > +                while (<$uploaded_file>) {
> > +                    $file_content .= $_;
> > +                }
> 
> I think
>   my $file_content = <$uploaded_file>;
> does the same thing.
> 

Doesn't work for files with "\n" 's in them. Replaced it with:

   my $file_content = do { local $/; <$uploaded_file>; };

At 9), I find this error:

On detail of a single invoice, a closed invoice shows to be in "Open" Status, and koha lets me "Close" this already closed invoice by "Save" button.
The only way to reopen a closed invoice is by Invoices' table, using "Reopen" in the 7-th column (the column has no name).

On master, on the contrary, for a closed invoice I see:
Status: Closed on ... some date.
Reopen:
Save

So I pass the patch to "Failed QA" status.

(In reply to Paola Rossi from comment #11)

> On detail of a single invoice, a closed invoice shows to be in "Open"
> Status, and koha lets me "Close" this already closed invoice by "Save"
> button.
> The only way to reopen a closed invoice is by Invoices' table, using
> "Reopen" in the 7-th column (the column has no name).
> 

Paola,
thanks for testing; indeed, there is an issue regarding closed invoices handling on invoice details page (well spotted) - I can replicate it on current master (without bug 3050 patches applied, see below), apparently it's an regression introduced by bug 10613 (BTW, shame on me - I had signed bug 10613 off without noticing this particular problem). But:

> On master, on the contrary, for a closed invoice I see:
> Status: Closed on ... some date.
> Reopen:
> Save

that part seems to be kinda questionable.. I mean (according to git log -p acqui/invoice.pl output on freshly git-cloned current master), this specific issue is not related to (and not caused by) bug 3050 patches. Unless I'm very much mistaken, it is a completely separate issue (see proposed patch in bug 12274) which is present in current master (no relation to bug 3050 whatsoever).

Kind Jacek, you're right about the matter in comment 12.

Today I test against master 3.15.00.052.
I find no regression.

So I pass the patch to "Signed Off" status.

QA comment:
1/ I don't consider a patch should fail qa if it does not use DBIC since we don't have any recommendation on how to integrate it into Koha scripts/modules at the moment.
2/ You use a new way for ordering dates in a table, I would prefer to keep the usual method (using span title).
3/ It would be great if the variable names in the module match the DB field names (for instance record_id vs recordid vs recid). Not blocker.
4/ These patch use implicit use of conditionals:
  * $a || return;
  * defined($a) || do { some assignment };
I am not sure we have coding guidelines about that but I really would prefer the code being consistent.
  * return if $a;
  * if ( defined($a) ) {
      some assignment;
    }
Not blocker.

I will provide a follow-up for 2.

Mared as Passed QA.

Created attachment 28391 [details] [review]
Bug 3050 - Add an option to upload scanned invoices #1/3

(part #1: new module w/ UT + script + template)

New feature, adds an ability to attach arbitrary files to
acquisition records (currently: to the invoices - but it can
be extended to baskets, basketgroups, budgets etc.).

Note: this code is (heavily) based on "Bug 8130 - attach PDF
files to a patron record" by Kale M Hall, main difference being
that new table (misc_files) and new module (Koha/Misc/Files.pm)
are intended to be a little more generic solution - they allow to
store and manage files associated with great many kinds of records,
from arbitrary tables.

Test plan:
1) Apply patch[es]
2) Run installer/data/mysql/updatedatabase.pl
3) Enable system preference 'AcqEnableFiles' in acquisition
4) New option 'Manage invoice files' appears in the invoice
detail page
5) Upload/view/download/delete some files for some invoices
6) Try to delete invoice with files attached (files should
get deleted as well)
7) Try to merge 2+ invoices with files attached; after merge,
all files previously attached to individual invoices being
merged should be attached to resulting invoice (merge destination)
8) prove t/db_dependent/Koha_Misc_Files.t
9) Ensure there are no regressions of any kind in invoice detail
page (acqui/invoice.pl).

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 28392 [details] [review]
Bug 3050 - Add an option to upload scanned invoices #2/3

(part #2: changes to existing acqui scripts & templates)

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 28393 [details] [review]
Bug 3050 - Add an option to upload scanned invoices #3/3

(part #3: DB updates + new syspref)

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 28394 [details] [review]
Bug 3050: QA follow-up

1/ Use the usual way for ordering dates in table
2/ Add a confirmation dialog box on deleting a file
3/ Add some UTs

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Pushed to master.  Thanks, Jacek!