Bug 22393 - Remove last remaining manualinvoice use
Summary: Remove last remaining manualinvoice use
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 23049
Blocks: 22394
  Show dependency treegraph
 
Reported: 2019-02-22 11:05 UTC by Martin Renvoize
Modified: 2020-09-04 15:34 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 22393: Update members/manivoice to use add_debit (2.82 KB, patch)
2019-02-22 11:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice (12.87 KB, patch)
2019-11-12 12:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice (13.52 KB, patch)
2019-11-12 12:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: simplify? (4.47 KB, patch)
2019-11-12 13:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice (13.53 KB, patch)
2019-11-12 13:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice (13.53 KB, patch)
2019-11-12 14:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: (follow-up) Handle deleted items (3.29 KB, patch)
2019-11-12 14:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice (6.40 KB, patch)
2020-02-10 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: (follow-up) Handle deleted items (3.92 KB, patch)
2020-02-10 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice (13.01 KB, patch)
2020-04-21 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: (follow-up) Corrections for QA feedback (2.98 KB, patch)
2020-04-21 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice (13.07 KB, patch)
2020-04-29 04:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22393: (follow-up) Corrections for QA feedback (3.04 KB, patch)
2020-04-29 04:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22393: (QA follow-up) Typos and Filters (2.71 KB, patch)
2020-07-10 16:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice (13.14 KB, patch)
2020-08-15 13:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22393: (follow-up) Corrections for QA feedback (3.11 KB, patch)
2020-08-15 13:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22393: (QA follow-up) Typos and Filters (2.77 KB, patch)
2020-08-15 13:48 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 Martin Renvoize 2019-02-22 11:05:49 UTC
We removed all the trivial uses of C4::Accounts::manualinvoice in bug 21756 but split this removal out into it's own case as it is non-trivial.
Comment 1 Martin Renvoize 2019-02-22 11:13:59 UTC
Created attachment 85516 [details] [review]
Bug 22393: Update members/manivoice to use add_debit

https://bugs.koha-community.org/show_bug.cgi?id=21756
Comment 2 Martin Renvoize 2019-11-12 12:24:30 UTC
Created attachment 95298 [details] [review]
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice
Comment 3 Martin Renvoize 2019-11-12 12:39:58 UTC
Created attachment 95303 [details] [review]
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice
Comment 4 Martin Renvoize 2019-11-12 12:51:27 UTC
Test plan:

Try adding various combinations of data in the manual invoice screen.. All options should still work as before, however a few new error cases are now caught and warned about.. for example, try entering a barcode that cannot be found in the system and you will see a new error is displayed.
Comment 5 Jonathan Druart 2019-11-12 13:47:59 UTC
Created attachment 95308 [details] [review]
Bug 22393: simplify?
Comment 6 Martin Renvoize 2019-11-12 13:48:11 UTC
Created attachment 95309 [details] [review]
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice
Comment 7 Martin Renvoize 2019-11-12 14:40:06 UTC
Created attachment 95325 [details] [review]
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice
Comment 8 Martin Renvoize 2019-11-12 14:40:10 UTC
Created attachment 95326 [details] [review]
Bug 22393: (follow-up) Handle deleted items
Comment 9 Jonathan Druart 2019-12-05 10:49:01 UTC
Comment on attachment 95326 [details] [review]
Bug 22393: (follow-up) Handle deleted items

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

::: members/maninvoice.pl
@@ +103,5 @@
>              $item_id = $item->itemnumber;
>          }
>          else {
> +            my $old_itemnumber = $dbh->selectrow_array(
> +                "SELECT itemnumber FROM deleted_items WHERE barcode=?",

deleted_items?
Comment 10 Katrin Fischer 2019-12-05 11:05:40 UTC
I also find this strange - I don't think we should ever delete from deleted_items? And why select by barcode?
Comment 11 Martin Renvoize 2019-12-17 15:02:55 UTC
(In reply to Katrin Fischer from comment #10)
> I also find this strange - I don't think we should ever delete from
> deleted_items? And why select by barcode?

We're not deleting from deleted_items.. we're looking up the item deleted_items using barcode if and only if we can't find the item already in items.  It's to maintain a relationship between the accountline and a possible deleted item.
Comment 12 Martin Renvoize 2019-12-17 15:03:02 UTC
(In reply to Katrin Fischer from comment #10)
> I also find this strange - I don't think we should ever delete from
> deleted_items? And why select by barcode?

We're not deleting from deleted_items.. we're looking up the item deleted_items using barcode if and only if we can't find the item already in items.  It's to maintain a relationship between the accountline and a possible deleted item.
Comment 13 Martin Renvoize 2020-02-10 15:13:35 UTC
Created attachment 98662 [details] [review]
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice

This patch re-arrange the manualinvoice controller script to clarify
code flow, replaces the last call to C4::Accounts::manualinvoice with a
call to Koha::Accounts->add_debit wrapped in a try catch block.  It also
adds a check on passed barcodes when the invoice type is 'LOST' so it
can link the subsequently created accuntline to the item and issue.
Comment 14 Martin Renvoize 2020-02-10 15:13:39 UTC
Created attachment 98663 [details] [review]
Bug 22393: (follow-up) Handle deleted items

This patch adds fallback handling to looking items in the old items
table if they're not found in the items table.
Comment 15 Martin Renvoize 2020-02-10 15:14:04 UTC
Rebased
Comment 16 Victor Grousset/tuxayo 2020-04-21 00:47:56 UTC
Doesn't apply:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice
Using index info to reconstruct a base tree...
M	members/maninvoice.pl
Falling back to patching base and 3-way merge...
Auto-merging members/maninvoice.pl
CONFLICT (content): Merge conflict in members/maninvoice.pl
error: Failed to merge in the changes.
Patch failed at 0001 Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice
hint: Use 'git am --show-current-patch=diff' to see the failed 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-22393-Remove-last-remaining-use-of-C4Accountsm-8xPypt.patch
Comment 17 Victor Grousset/tuxayo 2020-04-21 00:52:01 UTC
Conflict part:
(if that helps: with a third section thanks to git option merge.conflictStyle set to diff3)

<<<<<<< HEAD
        if ($add eq 'save and pay') {
            print $input->redirect(
                "/cgi-bin/koha/members/pay.pl?borrowernumber=$borrowernumber"
            );
        } else {
            print $input->redirect(
                "/cgi-bin/koha/members/boraccount.pl?borrowernumber=$borrowernumber"
            );
        }

        exit;
    }
}
else {
||||||| constructed merge base
        print $input->redirect(
            "/cgi-bin/koha/members/boraccount.pl?borrowernumber=$borrowernumber"
        );
        exit;
    }
}
else {
=======
            if ( C4::Context->preference('AccountAutoReconcile') ) {
                $patron->account->reconcile_balance;
            }
>>>>>>> Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice
Comment 18 Martin Renvoize 2020-04-21 08:56:21 UTC
Created attachment 103336 [details] [review]
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice

This patch re-arranges the manualinvoice controller script to clarify
code flow, replaces the last call to C4::Accounts::manualinvoice with a
call to Koha::Accounts->add_debit wrapped in a try catch block and also
adds a check on passed barcodes when the invoice type is 'LOST' so it
can link the subsequently created accountline to the item and issue.

Test plan
1/ Add a manual invoice (without entering a barcode)
2/ Add a manual invoice with a valid barcode (Not a LOST type)
3/ Add a manual invoice with a valid, but old, barcode (Not a LOST type)
4/ Add a manual invoice with an invalid barcode, not an error is
displayed
5/ Add a manual invoice with type 'LOST' and a valid barcode for a
checkout your user has had checked out
6/ Add a manual invoice with type 'LOST' and a valid barcode, but not
one that will match a checkout for your user. Note an error is displayed
7/ When errors are displayed, note the form contains data from the
previous submission so you can just correct the error rather than
re-enter all data.
8/ Signoff
Comment 19 Martin Renvoize 2020-04-21 08:57:13 UTC
Rebased and cleaned up.. test plan added.
Comment 20 Nick Clemens 2020-04-21 11:41:49 UTC
(In reply to Martin Renvoize from comment #18)
> 3/ Add a manual invoice with a valid, but old, barcode (Not a LOST type)

Do you mean a deleted barcode? The code tries, but there is a CONSTRAINT error

> 6/ Add a manual invoice with type 'LOST' and a valid barcode, but not
> one that will match a checkout for your user. Note an error is displayed

This succeeds with no problems, and I think that is right. If a patron comes in, takes a book off the shelf and destroys it they may want to won up. Or a parent taking responsibility for a child. There are many cases you may want to charge a lost with no checkout

> 7/ When errors are displayed, note the form contains data from the
> previous submission so you can just correct the error rather than
> re-enter all data.

The amount does not pass through, this would be helpful too
Comment 21 Martin Renvoize 2020-04-21 12:11:17 UTC
(In reply to Nick Clemens from comment #20)
> (In reply to Martin Renvoize from comment #18)
> > 3/ Add a manual invoice with a valid, but old, barcode (Not a LOST type)
> 
> Do you mean a deleted barcode? The code tries, but there is a CONSTRAINT
> error

I meant a deleted item, i.e. where the item has moved to deleted_items.. though this actually makes me wonder about the case where a barcode is deleted from an item.. I'll ponder that case.  As for the constraint, hmm.. I'll investigate that.. could be a bad constraint and lose of data elsewhere I've not fully considered.

> 
> > 6/ Add a manual invoice with type 'LOST' and a valid barcode, but not
> > one that will match a checkout for your user. Note an error is displayed
> 
> This succeeds with no problems, and I think that is right. If a patron comes
> in, takes a book off the shelf and destroys it they may want to won up. Or a
> parent taking responsibility for a child. There are many cases you may want
> to charge a lost with no checkout

Agreed, this should be allowed.. I think I just miswrote the plan there.

> 
> > 7/ When errors are displayed, note the form contains data from the
> > previous submission so you can just correct the error rather than
> > re-enter all data.
> 
> The amount does not pass through, this would be helpful too

Hmm, I thought I tested that
Comment 22 Martin Renvoize 2020-04-21 14:43:53 UTC
The fallback search for deleted_items works well, but the subsequent accountline creation fails due to a foreign key relationship from accountlines.itemnumber to items.itemnumber.. as the item is now in the delete_items table the key does not exist in items. (We have an 'on delete set null' condition for said key relationship, which feels incorrect to me).


> > 
> > > 6/ Add a manual invoice with type 'LOST' and a valid barcode, but not
> > > one that will match a checkout for your user. Note an error is displayed
> > 
> > This succeeds with no problems, and I think that is right. If a patron comes
> > in, takes a book off the shelf and destroys it they may want to won up. Or a
> > parent taking responsibility for a child. There are many cases you may want
> > to charge a lost with no checkout
> 
> Agreed, this should be allowed.. I think I just miswrote the plan there.

I'm not so sure here.. we have internal logic for refunding lost and found items etc which rely on consistently set issue_id's.. perhaps a 'Damaged paid for' might be more helpful.. hmm.
Comment 23 Martin Renvoize 2020-04-21 14:57:01 UTC
Created attachment 103382 [details] [review]
Bug 22393: (follow-up) Corrections for QA feedback

We correct the behviour of 'amount' being passed back to the template
and add a work around for the existance of deleted_items alongside a FK
to items in the accountlines table.
Comment 24 Victor Grousset/tuxayo 2020-04-24 05:46:56 UTC
> 4/ Add a manual invoice with an invalid barcode, not an error is displayed

You mean "note that an error is display" right?

> 6/ Add a manual invoice with type 'LOST' and a valid barcode, but not
> one that will match a checkout for your user. Note an error is displayed

IIUC a non-checked out item is fine.
Comment 25 Victor Grousset/tuxayo 2020-04-24 05:47:53 UTC
Will sign off and amend the test plan depending on the above points.
Comment 26 Victor Grousset/tuxayo 2020-04-29 04:28:00 UTC
Created attachment 103897 [details] [review]
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice

This patch re-arranges the manualinvoice controller script to clarify
code flow, replaces the last call to C4::Accounts::manualinvoice with a
call to Koha::Accounts->add_debit wrapped in a try catch block and also
adds a check on passed barcodes when the invoice type is 'LOST' so it
can link the subsequently created accountline to the item and issue.

Test plan
1/ Add a manual invoice (without entering a barcode)
2/ Add a manual invoice with a valid barcode (Not a LOST type)
3/ Add a manual invoice with a valid, but old, barcode (Not a LOST type)
4/ Add a manual invoice with an invalid barcode, note that an error is
displayed
5/ Add a manual invoice with type 'LOST' and a valid barcode for a
checkout your user has had checked out
6/ Add a manual invoice with type 'LOST' and a valid barcode, but not
one that will match a checkout for your user. Note an error is displayed
7/ When errors are displayed, note the form contains data from the
previous submission so you can just correct the error rather than
re-enter all data.
8/ Signoff

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 27 Victor Grousset/tuxayo 2020-04-29 04:28:05 UTC
Created attachment 103898 [details] [review]
Bug 22393: (follow-up) Corrections for QA feedback

We correct the behviour of 'amount' being passed back to the template
and add a work around for the existance of deleted_items alongside a FK
to items in the accountlines table.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 28 Victor Grousset/tuxayo 2020-04-29 04:29:11 UTC
After a few days and rereading the thing I'll be optimistic and sign it off :)

If the points from comment 24 are clear also for QA then the patches can move on to get QAed.


Fixed steps 4 of the plan:
> 4/ Add a manual invoice with an invalid barcode, not an error is
> displayed> 4/ Add a manual invoice with an invalid barcode, note that an error is
> displayed
Comment 29 Katrin Fischer 2020-07-05 01:21:40 UTC
Please fix the QA script errors:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/members/maninvoice.tt
   FAIL	  filters
		missing_filter at line 45 (                        An error occured, please try again: [% error %])
		missing_filter at line 66 (                            <li><label for="barcode">Barcode: </label><input type="text" name="barcode" id="barcode" value="[% barcode %]" /></li>)
		missing_filter at line 67 (                            <li><label for="desc">Description: </label><input type="text" name="desc" id="desc" size="50" value="[% desc %]" /></li>)
		missing_filter at line 68 (                            <li><label for="note">Note: </label><input type="text" name="note" size="50" id="note" value="[% note %]" /></li>)
		missing_filter at line 69 (                            <li><label for="amount">Amount: </label><input type="number" name="amount" id="amount" required="required" step="any" min="0" value="[% amount %]" /> Example: 5.00</li>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  js_in_body
   FAIL	  spelling
		 occured  ==> occurred
   OK	  tt_valid
   OK	  valid_template
Comment 30 Martin Renvoize 2020-07-10 16:42:00 UTC
Created attachment 106773 [details] [review]
Bug 22393: (QA follow-up) Typos and Filters

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Katrin Fischer 2020-08-15 13:48:26 UTC
Created attachment 108330 [details] [review]
Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice

This patch re-arranges the manualinvoice controller script to clarify
code flow, replaces the last call to C4::Accounts::manualinvoice with a
call to Koha::Accounts->add_debit wrapped in a try catch block and also
adds a check on passed barcodes when the invoice type is 'LOST' so it
can link the subsequently created accountline to the item and issue.

Test plan
1/ Add a manual invoice (without entering a barcode)
2/ Add a manual invoice with a valid barcode (Not a LOST type)
3/ Add a manual invoice with a valid, but old, barcode (Not a LOST type)
4/ Add a manual invoice with an invalid barcode, note that an error is
displayed
5/ Add a manual invoice with type 'LOST' and a valid barcode for a
checkout your user has had checked out
6/ Add a manual invoice with type 'LOST' and a valid barcode, but not
one that will match a checkout for your user. Note an error is displayed
7/ When errors are displayed, note the form contains data from the
previous submission so you can just correct the error rather than
re-enter all data.
8/ Signoff

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Katrin Fischer 2020-08-15 13:48:31 UTC
Created attachment 108331 [details] [review]
Bug 22393: (follow-up) Corrections for QA feedback

We correct the behviour of 'amount' being passed back to the template
and add a work around for the existance of deleted_items alongside a FK
to items in the accountlines table.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Katrin Fischer 2020-08-15 13:48:35 UTC
Created attachment 108332 [details] [review]
Bug 22393: (QA follow-up) Typos and Filters

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Jonathan Druart 2020-08-18 15:46:06 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 35 Caroline Cyr La Rose 2020-08-20 20:34:22 UTC
I changed the component from "Architecture, internals and plumbing" to "System administration" so that it appears under Administration in the release notes. I hope this is ok. If not, feel free to change it back :)
Comment 36 Caroline Cyr La Rose 2020-08-20 20:43:07 UTC
Sorry!! Wrong bug, I put it back
Comment 37 Lucas Gass 2020-09-04 15:34:32 UTC
enhancement will not be backported to 20.05.x