|
Description
Martin Renvoize (ashimema)
2019-02-22 11:05:49 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 Created attachment 95298 [details] [review] Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice Created attachment 95303 [details] [review] Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice 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. Created attachment 95308 [details] [review] Bug 22393: simplify? Created attachment 95309 [details] [review] Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice Created attachment 95325 [details] [review] Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice Created attachment 95326 [details] [review] Bug 22393: (follow-up) Handle deleted items 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? I also find this strange - I don't think we should ever delete from deleted_items? And why select by barcode? (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. (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. 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. 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. Rebased 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 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
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 Rebased and cleaned up.. test plan added. (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 (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 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.
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. > 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. Will sign off and amend the test plan depending on the above points. 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> 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> 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 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 Created attachment 106773 [details] [review] Bug 22393: (QA follow-up) Typos and Filters Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> 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> 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> Pushed to master for 20.11, thanks to everybody involved! 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 :) Sorry!! Wrong bug, I put it back enhancement will not be backported to 20.05.x |