Bug 3050 - Add an option to upload scanned invoices
Summary: Add an option to upload scanned invoices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: PC All
: P2 new feature (vote)
Assignee: Jacek Ablewicz
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-20 11:12 UTC by Chris Cormack
Modified: 2015-06-04 23:33 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 3050 - Add an option to upload scanned invoices (21.88 KB, patch)
2014-02-14 10:51 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3050 - Add an option to upload scanned invoices (22.10 KB, patch)
2014-02-15 17:59 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 3050: [QA Followup] Fix various QA issues, plus some minor improvements (29.47 KB, patch)
2014-04-01 11:04 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 3050 - Add an option to upload scanned invoices #1/3 (22.28 KB, patch)
2014-05-09 10:32 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 3050 - Add an option to upload scanned invoices #2/3 (4.67 KB, patch)
2014-05-09 10:32 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 3050 - Add an option to upload scanned invoices #3/3 (5.30 KB, patch)
2014-05-09 10:32 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Add an option to upload scanned invoices #1/3 (22.97 KB, patch)
2014-05-20 13:21 UTC, Paola Rossi
Details | Diff | Splinter Review
Add an option to upload scanned invoices #2/3 (4.86 KB, patch)
2014-05-20 13:22 UTC, Paola Rossi
Details | Diff | Splinter Review
Add an option to upload scanned invoices #3/3 (5.46 KB, patch)
2014-05-20 13:23 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 3050 - Add an option to upload scanned invoices #1/3 (22.40 KB, patch)
2014-05-21 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3050 - Add an option to upload scanned invoices #2/3 (4.79 KB, patch)
2014-05-21 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3050 - Add an option to upload scanned invoices #3/3 (5.41 KB, patch)
2014-05-21 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3050: QA follow-up (5.51 KB, patch)
2014-05-21 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:05:49 UTC


---- Reported by telefrancisco@gmail.com 2009-03-20 23:12:41 ----

It will be very useful if an option to upload to Koha (in Acquisitions module) the invoices is implemented. This is useful in order to have a visual confirmation in addition to the "manually-wrote" information.

Thanks.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:05 UTC  ---

This bug was previously known as _bug_ 3050 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3050

Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was telefrancisco@gmail.com.

Comment 1 Katrin Fischer 2011-10-28 12:06:28 UTC
This feature is not available in current stable release 3.6
Comment 2 Nicole C. Engard 2013-03-27 15:39:01 UTC
I was just asked for this feature in a webinar today - so it's still desired.

Nicole
Comment 3 Jacek Ablewicz 2014-02-14 10:51:28 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2014-02-15 17:59:41 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-03-21 10:47:17 UTC
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.
Comment 6 Jacek Ablewicz 2014-04-01 11:04:44 UTC Comment hidden (obsolete)
Comment 7 Jacek Ablewicz 2014-04-01 11:37:31 UTC
(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>; };
Comment 8 Jacek Ablewicz 2014-05-09 10:32:28 UTC Comment hidden (obsolete)
Comment 9 Jacek Ablewicz 2014-05-09 10:32:36 UTC Comment hidden (obsolete)
Comment 10 Jacek Ablewicz 2014-05-09 10:32:42 UTC Comment hidden (obsolete)
Comment 11 Paola Rossi 2014-05-16 14:47:21 UTC
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.
Comment 12 Jacek Ablewicz 2014-05-17 10:32:45 UTC
(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).
Comment 13 Paola Rossi 2014-05-20 13:20:37 UTC
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.
Comment 14 Paola Rossi 2014-05-20 13:21:57 UTC Comment hidden (obsolete)
Comment 15 Paola Rossi 2014-05-20 13:22:49 UTC Comment hidden (obsolete)
Comment 16 Paola Rossi 2014-05-20 13:23:38 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-05-21 10:07:43 UTC
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.
Comment 18 Jonathan Druart 2014-05-21 10:09:49 UTC
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>
Comment 19 Jonathan Druart 2014-05-21 10:09:57 UTC
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>
Comment 20 Jonathan Druart 2014-05-21 10:10:02 UTC
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>
Comment 21 Jonathan Druart 2014-05-21 10:10:06 UTC
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>
Comment 22 Galen Charlton 2014-05-25 21:46:13 UTC
Pushed to master.  Thanks, Jacek!