Bug 20629 - Remove ability to 'reverse' payments
Summary: Remove ability to 'reverse' payments
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 18790
Blocks: 7205 11972 14062 14140 21804
  Show dependency treegraph
 
Reported: 2018-04-20 15:53 UTC by Kyle M Hall
Modified: 2019-10-14 19:58 UTC (History)
7 users (show)

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


Attachments
Bug 20629: Remove ability to 'reverse' payments (5.88 KB, patch)
2018-04-30 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629 - Remove ability to 'reverse' payments (5.87 KB, patch)
2018-10-15 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629 - Remove ability to 'reverse' payments (6.24 KB, patch)
2018-10-15 16:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629 - Remove ability to 'reverse' payments (6.24 KB, patch)
2018-10-15 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629 - Pass accountline objects to template instead of hashref (4.82 KB, patch)
2018-10-15 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629: Follow up - fix reverse_col value (998 bytes, patch)
2018-10-15 19:00 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20629: Follow up to add POD for getcharges. (1.01 KB, patch)
2018-10-15 19:00 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20629: Remove ability to 'reverse' payments (6.30 KB, patch)
2018-10-15 19:01 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20629: Pass accountline objects to template instead of hashref (4.87 KB, patch)
2018-10-15 19:01 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20629: Follow up - fix reverse_col value (1.03 KB, patch)
2018-10-15 19:01 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20629: Follow up to add POD for getcharges. (1.06 KB, patch)
2018-10-15 19:01 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20629: Follow up - fix reverse_col value (1.10 KB, patch)
2018-10-15 19:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629: Follow up to add POD for getcharges. (1.13 KB, patch)
2018-10-15 19:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629: Rename to (2.48 KB, patch)
2018-10-17 10:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629: Rename reverse_col to actions_col (2.50 KB, patch)
2018-10-17 10:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20629: Remove ability to 'reverse' payments (6.32 KB, patch)
2018-10-26 16:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: Pass accountline objects to template instead of hashref (4.91 KB, patch)
2018-10-26 16:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: (follow-up) fix reverse_col value (1.13 KB, patch)
2018-10-26 16:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: Rename reverse_col to actions_col (2.54 KB, patch)
2018-10-26 16:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: (QA follow-up) Remove actions_col (3.27 KB, patch)
2018-10-26 16:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: Remove ability to 'reverse' payments (6.32 KB, patch)
2018-10-27 11:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: Pass accountline objects to template instead of hashref (4.91 KB, patch)
2018-10-27 11:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: (follow-up) fix reverse_col value (1.13 KB, patch)
2018-10-27 11:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: Rename reverse_col to actions_col (2.53 KB, patch)
2018-10-27 11:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20629: (QA follow-up) Remove actions_col (4.26 KB, patch)
2018-10-27 11:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-04-20 15:53:06 UTC
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.
Comment 1 Katrin Fischer 2018-04-21 08:04:12 UTC
The feature is quite problematic, +1 for removing it now.
Comment 2 Kyle M Hall 2018-04-30 10:58:06 UTC
Created attachment 74935 [details] [review]
Bug 20629: Remove ability to 'reverse' payments
Comment 3 Katrin Fischer 2018-08-12 21:56:30 UTC
Hi Kyle, should this be 'Needs Signoff'?
Comment 4 Katrin Fischer 2018-10-14 08:53:59 UTC
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
Comment 5 Kyle M Hall 2018-10-15 12:03:45 UTC
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
Comment 6 Mark Tompsett 2018-10-15 14:17:43 UTC
Unblessed doesn't get is_credit or is_debit, only an object would.
Either add, or better yet deal with the FIXME. :)
Comment 7 Kyle M Hall 2018-10-15 16:36:49 UTC
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
Comment 8 Kyle M Hall 2018-10-15 16:53:47 UTC
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
Comment 9 Kyle M Hall 2018-10-15 16:53:58 UTC
Created attachment 80622 [details] [review]
Bug 20629 - Pass accountline objects to template instead of hashref
Comment 10 Mark Tompsett 2018-10-15 17:33:29 UTC
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&amp;accountlines_id=[% account.accountlines_id | html %]&amp;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 11 Mark Tompsett 2018-10-15 17:35:51 UTC
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.
Comment 12 Mark Tompsett 2018-10-15 19:00:31 UTC
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.
Comment 13 Mark Tompsett 2018-10-15 19:00:36 UTC
Created attachment 80630 [details] [review]
Bug 20629: Follow up to add POD for getcharges.
Comment 14 Mark Tompsett 2018-10-15 19:01:35 UTC
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>
Comment 15 Mark Tompsett 2018-10-15 19:01:39 UTC
Created attachment 80632 [details] [review]
Bug 20629: Pass accountline objects to template instead of hashref

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 Mark Tompsett 2018-10-15 19:01:44 UTC
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>
Comment 17 Mark Tompsett 2018-10-15 19:01:48 UTC
Created attachment 80634 [details] [review]
Bug 20629: Follow up to add POD for getcharges.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 18 Kyle M Hall 2018-10-15 19:05:41 UTC
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>
Comment 19 Kyle M Hall 2018-10-15 19:06:09 UTC
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>
Comment 20 Kyle M Hall 2018-10-17 10:12:51 UTC
Created attachment 80692 [details] [review]
Bug 20629: Rename  to
Comment 21 Kyle M Hall 2018-10-17 10:34:54 UTC
Created attachment 80693 [details] [review]
Bug 20629: Rename reverse_col to actions_col
Comment 22 Tomás Cohen Arazi 2018-10-26 15:50:51 UTC
(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.
Comment 23 Tomás Cohen Arazi 2018-10-26 16:14:03 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2018-10-26 16:14:10 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2018-10-26 16:14:20 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2018-10-26 16:14:28 UTC Comment hidden (obsolete)
Comment 27 Tomás Cohen Arazi 2018-10-26 16:15:05 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2018-10-27 11:02:53 UTC
Doesn't apply anymore, rebasing.
Comment 29 Tomás Cohen Arazi 2018-10-27 11:04:09 UTC
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>
Comment 30 Tomás Cohen Arazi 2018-10-27 11:04:14 UTC
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>
Comment 31 Tomás Cohen Arazi 2018-10-27 11:04:19 UTC
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>
Comment 32 Tomás Cohen Arazi 2018-10-27 11:04:24 UTC
Created attachment 81352 [details] [review]
Bug 20629: Rename reverse_col to actions_col

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi 2018-10-27 11:04:29 UTC
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>
Comment 34 Nick Clemens 2018-11-06 17:54:59 UTC
Awesome work all!

Pushed to master for 18.11
Comment 35 Josef Moravec 2018-11-09 16:50:37 UTC
There were bad rebase somewhere, commit fb15a9631044cd0191c58feb5b8370e9b846136b reintroduces getcharges!
Comment 36 Martin Renvoize 2018-11-16 12:35:10 UTC
Enhancement, will not be backported to 18.05.x series.