Now that we have a true ability to void payments in Koha, we should probably remove the reverse payment option which was something of a kludge to begin with.
The feature is quite problematic, +1 for removing it now.
Created attachment 74935 [details] [review] Bug 20629: Remove ability to 'reverse' payments
Hi Kyle, should this be 'Needs Signoff'?
Patch doesn't apply - can you please rebase? CONFLICT (content): Merge conflict in members/printfeercpt.pl Auto-merging members/boraccount.pl CONFLICT (content): Merge conflict in members/boraccount.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt Auto-merging Koha/Account/Line.pm CONFLICT (content): Merge conflict in Koha/Account/Line.pm
Created attachment 80590 [details] [review] Bug 20629 - Remove ability to 'reverse' payments Test Plan: 1) Apply this patch 2) Note all references to reversing payments have been removed 3) Note ability to void payments remains unchanged
Unblessed doesn't get is_credit or is_debit, only an object would. Either add, or better yet deal with the FIXME. :)
Created attachment 80620 [details] [review] Bug 20629 - Remove ability to 'reverse' payments Test Plan: 1) Apply this patch 2) Note all references to reversing payments have been removed 3) Note ability to void payments remains unchanged
Created attachment 80621 [details] [review] Bug 20629 - Remove ability to 'reverse' payments Test Plan: 1) Apply this patch 2) Note all references to reversing payments have been removed 3) Note ability to void payments remains unchanged
Created attachment 80622 [details] [review] Bug 20629 - Pass accountline objects to template instead of hashref
Comment on attachment 80622 [details] [review] Bug 20629 - Pass accountline objects to template instead of hashref Review of attachment 80622 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt @@ +75,5 @@ > [% ELSE %] > <a target="_blank" href="printinvoice.pl?action=print&accountlines_id=[% account.accountlines_id | html %]&borrowernumber=[% account.borrowernumber | html %]" class="btn btn-default btn-xs"><i class="fa fa-print"></i> Print</a> > [% END %] > <a href="accountline-details.pl?accountlines_id=[% account.accountlines_id | html %]" class="btn btn-default btn-xs"><i class="fa fa-list"></i> Details</a> > [% IF ( reverse_col) %] reverse_col is always 0, because it is passed as 0, and you don't set it above, and you need to set it BEFORE the header.
Comment on attachment 80622 [details] [review] Bug 20629 - Pass accountline objects to template instead of hashref Review of attachment 80622 [details] [review]: ----------------------------------------------------------------- ::: members/boraccount.pl @@ +80,3 @@ > } > > my $reverse_col = 0; # Flag whether we need to show the reverse column perhaps two Account lines searches, and merge the array? That will give you the ability to set reverse_col correctly.
Created attachment 80629 [details] [review] Bug 20629: Follow up - fix reverse_col value Before this patch the Void button didn't appear. After this patch the Void button appears on payments.
Created attachment 80630 [details] [review] Bug 20629: Follow up to add POD for getcharges.
Created attachment 80631 [details] [review] Bug 20629: Remove ability to 'reverse' payments Test Plan: 1) Apply this patch 2) Note all references to reversing payments have been removed 3) Note ability to void payments remains unchanged Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 80632 [details] [review] Bug 20629: Pass accountline objects to template instead of hashref Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 80633 [details] [review] Bug 20629: Follow up - fix reverse_col value Before this patch the Void button didn't appear. After this patch the Void button appears on payments. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 80634 [details] [review] Bug 20629: Follow up to add POD for getcharges. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 80635 [details] [review] Bug 20629: Follow up - fix reverse_col value Before this patch the Void button didn't appear. After this patch the Void button appears on payments. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 80636 [details] [review] Bug 20629: Follow up to add POD for getcharges. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 80692 [details] [review] Bug 20629: Rename to
Created attachment 80693 [details] [review] Bug 20629: Rename reverse_col to actions_col
(In reply to Kyle M Hall from comment #21) > Created attachment 80693 [details] [review] [review] > Bug 20629: Rename reverse_col to actions_col I think there was a reason for it to be called reverse_col: it was originally used to make a decision on displaying the column with the reverse button (of course). But right now we really display other things (like the print button, the details button, the void button). We should really remove the conditional display, as it should always be displayed. @mtompset: I will remove your patch with the POD for getcharges, because it is actually getting removed in bug 21681.
Created attachment 81311 [details] [review] Bug 20629: Remove ability to 'reverse' payments Test Plan: 1) Apply this patch 2) Note all references to reversing payments have been removed 3) Note ability to void payments remains unchanged Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81312 [details] [review] Bug 20629: Pass accountline objects to template instead of hashref Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81313 [details] [review] Bug 20629: (follow-up) fix reverse_col value Before this patch the Void button didn't appear. After this patch the Void button appears on payments. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81314 [details] [review] Bug 20629: Rename reverse_col to actions_col Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81315 [details] [review] Bug 20629: (QA follow-up) Remove actions_col The actions_col template param requires a DB search, only to display the 'Actions' column header vs. an empty string. But there will always be buttons in there, that were added *after*. It made sense when only the 'Reverse' button was displayed, but now both 'Print' and 'Details' are displayed anyway. This patch removes it for good :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Doesn't apply anymore, rebasing.
Created attachment 81349 [details] [review] Bug 20629: Remove ability to 'reverse' payments Test Plan: 1) Apply this patch 2) Note all references to reversing payments have been removed 3) Note ability to void payments remains unchanged Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81350 [details] [review] Bug 20629: Pass accountline objects to template instead of hashref Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81351 [details] [review] Bug 20629: (follow-up) fix reverse_col value Before this patch the Void button didn't appear. After this patch the Void button appears on payments. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81352 [details] [review] Bug 20629: Rename reverse_col to actions_col Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81353 [details] [review] Bug 20629: (QA follow-up) Remove actions_col The actions_col template param requires a DB search, only to display the 'Actions' column header vs. an empty string. But there will always be buttons in there, that were added *after*. It made sense when only the 'Reverse' button was displayed, but now both 'Print' and 'Details' are displayed anyway. This patch removes it for good :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Awesome work all! Pushed to master for 18.11
There were bad rebase somewhere, commit fb15a9631044cd0191c58feb5b8370e9b846136b reintroduces getcharges!
Enhancement, will not be backported to 18.05.x series.