---- Reported by nengard@gmail.com 2008-10-20 05:29:15 ---- The account should show which item was paid for instead of just putting the 'payment thanks' note on the account ---- Additional Comments From nengard@gmail.com 2008-10-20 05:29:31 ---- Created an attachment account ---- Additional Comments From nengard@gmail.com 2009-04-18 14:09:31 ---- Just want to bump this up as a priority for 3.2 - and something that I think should be pretty simple to put in place. ---- Additional Comments From nengard@gmail.com 2009-04-18 15:44:22 ---- Ryan, I hear you're re-writing account management -so I wanted you to see this. ---- Additional Comments From nengard@gmail.com 2010-04-27 21:59:48 ---- This has become a bigger problem now with the ability to reverse fines - you get an account line that says 'payment thanks - reversed' -- which really makes no sense at all. We need to see what was paid for. --- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:54 UTC --- This bug was previously known as _bug_ 2696 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=2696 Imported an attachment (id=800) Actual time not defined. Setting to 0.0 CC member arm@hanover.ca does not have an account here CC member bchurch@ptfs.com does not have an account here CC member rch@liblime.com does not have an account here The original submitter of attachment 800 [details] is unknown. Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
Created attachment 3069 [details] opac screenshot This problem still exists in current master and 3.2.x branches, not only in staff, but also in OPAC. The line on the 'My Fines' tab is always 'Payment,thanks. -'. Looks like something is missing.
At the moment the implementation is as follows (Acounts.pm line 65 ff and other places): --Snip---- Amounts owed are paid off oldest first. That is, if the patron has a $1 fine from Feb. 1, another $1 fine from Mar. 1, and makes a payment of $1.50, then the oldest fine will be paid off in full, and $0.50 will be credited to the next one. (...) # FIXME - I'm not at all sure about the above, because I don't # understand what the acct* tables in the Koha database are for. --End Snip ---- I tried to understand what happens in the code, and I came to the conclusion that the most important thing is a missing primary key in the table accountlines. This key could be transported through the chain of forms and involved code files to match payments with the fines the payments are made for. I a primary key in the table accountlines could be the basis to solve other bugs as well: - Bug 7531 - Reversing partial payments is buggy - (?) Bug 6413 - Notes in Fines doing wonky things - (?) Bug 2598 - Pay Fines allows payment against accruing fine. - (?) Bug 7205 - can't reverse manual credits - (?) Bug 7565 - Fee/fines receipt for printing - (?) Bug 6273 - SIP2 Fee Paid and Fee Paid Response support - Bug 3498 - Allow partial fine payments - Bug 2169 - Paying only partial fines (Duplicate of 3498) Maybe already addressed by: Bug 6427 - Rewrite of the accounts system ?
Oh, I see that the primary key is already addressed in Bug 7671 - add a real primary key "accountlinesid" in accountlines.
Created attachment 9583 [details] staff with payment description It looks like in some cases it is now showing what was paid for - when you pay for a specific line item.
Something to look out for when testing the accounts rewrite :)
(In reply to Katrin Fischer from comment #5) > Something to look out for when testing the accounts rewrite :) I can tell you that the new accounting system tracks and stores every fee paid for by every payment. If a payment is made that paid 3 fines, it creates three account_offset rows that link the payment to each of those fines, along with the amount paid on each! When you look at patrons fines, you can even "drill down" to see the entire history for each fee, and for each payment.
(In reply to Kyle M Hall from comment #6) > (In reply to Katrin Fischer from comment #5) > > Something to look out for when testing the accounts rewrite :) > > I can tell you that the new accounting system tracks and stores every fee > paid for by every payment. If a payment is made that paid 3 fines, it > creates three account_offset rows that link the payment to each of those > fines, along with the amount paid on each! > > When you look at patrons fines, you can even "drill down" to see the entire > history for each fee, and for each payment. So, it this still an open bug or can this be closed?
It's still an open bug.
Hi Everyone, I am new in this. I want to solve this bug. Please suggest me what is the actual requirement. Is it that when we pay fine in admin then it shows only payment thanks but you need that it should show item details also (i.e - Item Barcode or Item Title). Please help me.
Hi Chetan, this is not easy to solve, as one payment can be for multiple fines and can only include partial payments. This is why Kyle and others have been working on a rewrite of the accounts system.
Thanks Kartin, I understood the problem.
*** Bug 10167 has been marked as a duplicate of this bug. ***
Created attachment 64287 [details] [review] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl
Created attachment 64293 [details] [review] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl
Tests are missing
The patch and its dependencies applied cleanly and there were no problems when running updatedatabase.pl. I added/paid fines on accounts but when I click on the "Details" button of boraccount.pl tailing plack throws the following error message: Can't locate object method "search" via package "Koha::Account::Offsets" (perhaps you forgot to load "Koha::Account::Offsets"?) at /home/vagrant/kohaclone/members/accountline-details.pl line 52. Line 52 of accountline-details.pl consists of the following code: my @account_offsets = Koha::Account::Offsets->search( { $column => $accountlines_id } ); The only methods contained in Offsets.pm that are showing for me are: sub _type { return 'AccountOffset'; } sub object_class { return 'Koha::Account::Offset'; } Let me know if there appears to be a problem with my test and I will try it again.
Lowering severity, it has been set as critical 7 years ago.
Created attachment 64342 [details] [review] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl
Created attachment 64345 [details] [review] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl
Created attachment 64346 [details] [review] [SIGNED-OFF]Bug 2696: Payments Shows What Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Running updatedatabase.pl after patch application makes dependencies happy. Passes QA Tools and works as intended.
Created attachment 64350 [details] [review] [SIGNED-OFF] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Running updatedatabase.pl after patch application makes dependencies happy. Passes QA Tools and works as intended.
Created attachment 67479 [details] [review] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Running updatedatabase.pl after patch application makes dependencies happy. Passes QA Tools and works as intended.
Created attachment 70031 [details] [review] [PATCH] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Running updatedatabase.pl after patch application makes dependencies happy. Passes QA Tools and works as intended. Rebased (2017-12-21): Alex Arnaud <alex.arnaud@biblibre.com>
Quick test gave me: - Error on accountline-details.pl: "GetMember" is not exported by the C4::Members module Can't continue after import errors at /home/koha/src/members/accountline-details.pl line 26. You could use Koha::Patron->find now. - On the same script after fixing the first error: Template process failed: undef error - The method payment_type is not covered by tests! Also, i've rebased the patch: new account type ('PF', Processing fee),
Created attachment 72024 [details] [review] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Running updatedatabase.pl after patch application makes dependencies happy. Passes QA Tools and works as intended. Rebased (2017-12-21): Alex Arnaud <alex.arnaud@biblibre.com>
(In reply to Alex Arnaud from comment #24) > Quick test gave me: > > - Error on accountline-details.pl: > "GetMember" is not exported by the C4::Members module > Can't continue after import errors at > /home/koha/src/members/accountline-details.pl line 26. > You could use Koha::Patron->find now. > > - On the same script after fixing the first error: > Template process failed: undef error - The method payment_type is not > covered by tests! > > Also, i've rebased the patch: new account type ('PF', Processing fee), Removed call to GetMember. Don't forget to update the dbix class files before testing! That would explain your second error.
Kyle, Haven't you forgotten to add accountline-details.pl, accountline-details.tt and accounttype.inc in the last patch?
(In reply to Alex Arnaud from comment #27) > Kyle, > > Haven't you forgotten to add accountline-details.pl, accountline-details.tt > and accounttype.inc in the last patch? Wow, no idea what happened there. Thanks for letting me know!
Created attachment 72088 [details] [review] Bug 2696 - Fine Payments Should Show What Was Paid For Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Running updatedatabase.pl after patch application makes dependencies happy. Passes QA Tools and works as intended. Rebased (2017-12-21): Alex Arnaud <alex.arnaud@biblibre.com>
Kyle, it does not apply, can you rebase please?
Kyle, GetMember is still in use in members/accountline-details.pl. Also this script is not executable
Created attachment 72263 [details] [review] Bug 2696 [QA Followup] - Remove use of GetMember
Can't wait to have this, but currently blocked by FAILED QA on bug 18790 - Kyle, can you put this in our queue again once the other is in? Thx!
QA script and tests pass, will continue poking at this for a bit longer :)
Comment on attachment 72088 [details] [review] Bug 2696 - Fine Payments Should Show What Was Paid For Review of attachment 72088 [details] [review]: ----------------------------------------------------------------- Hi all, looking at how long we've wanted for this... wow. While testing I discovered several issues with the template variables that I am fixing in a QA follow-up. ::: Koha/Account/Offset.pm @@ +37,5 @@ > =cut > > +=head3 debit > + > +=cut 1) Please add POD to debit and credit! Not treating this as a blocker, but please provide a follow up. ::: koha-tmpl/intranet-tmpl/prog/en/includes/account_offset_type.inc @@ +1,2 @@ > +[% SWITCH account_offset.type %] > + [% CASE 'Payment' %]Payment 2) I've checked and the strings from account_offset_type.inc and accounttype.inc are picked up by translation scripts. Just a note, nothing to see here! ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/accountline-details.tt @@ +84,5 @@ > + <th>Amount</th> > + <th>Amount outstanding</th> > + <th>Type</th> > + <th>Note</th> > + <th>Transacting<br/>librarian id</th> 3) This is a translation issue, because it leaves you with "Transacting" and "library id" separately. We need to achieve a nice layout otherwise. I will remove the <br> in a QA follow-up. ::: members/accountline-details.pl @@ +18,5 @@ > +# along with Koha; if not, see <http://www.gnu.org/licenses>. > + > +use Modern::Perl; > + > +use CGI; 4) Should be: use CGI qw ( -utf8 );
Created attachment 74672 [details] [review] Bug 2696: Fine payments should show what was paid for Test Plan: 1) Apply this patch and its' dependencies 2) Create and pay some various fees and fines 3) View the payments for fees, and fees paid by payments by using the new 'Details' button available on boraccount.pl Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Running updatedatabase.pl after patch application makes dependencies happy. Passes QA Tools and works as intended. Rebased (2017-12-21): Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74673 [details] [review] Bug 2696: (QA follow-up) - Remove use of GetMember Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74674 [details] [review] Bug 2696: (QA follow-up) Several fixes to template variables - Fix link to patron account in breadcrumbs - Fix includes for accounttype.inc to display type correctly - in Details for fee - in Payments list - Fix use CGI qw ( -utf8 ); - Remove <br> from within a string for better translation
Comment on attachment 74672 [details] [review] Bug 2696: Fine payments should show what was paid for Review of attachment 74672 [details] [review]: ----------------------------------------------------------------- ::: Koha/Account/Offset.pm @@ +20,4 @@ > use Carp; > > use Koha::Database; > +use Koha::Account::Line; Should be Koha::Account::Lines @@ +39,5 @@ > +=head3 debit > + > +=cut > + > +sub debit { POD missing. @@ +42,5 @@ > + > +sub debit { > + my ( $self ) = @_; > + my $debit_rs = $self->_result->debit; > + return unless $debit_rs; I do not think we should "return unless" here, we do not do that for other methods. Is there a good reason? @@ +50,5 @@ > +=head3 credit > + > +=cut > + > +sub credit { Same as above ::: members/accountline-details.pl @@ +34,5 @@ > + query => $input, > + type => "intranet", > + authnotrequired => 0, > + flagsrequired => { > + borrowers => 1, should be edit_borrowers. @@ +48,5 @@ > +if ($accountline) { > + my $type = $accountline->amount < 0 ? 'credit' : 'debit'; > + my $column = $type eq 'credit' ? 'credit_id' : 'debit_id'; > + > + my @account_offsets = Koha::Account::Offsets->search( { $column => $accountlines_id } ); Would be better to call in scalar context and make the order by created_on here, not template-side.
It would also be great to use the new .inc in other parts of the code where they are displayed, to avoid duplicated code (on a follow-up bug report?)
(In reply to Jonathan Druart from comment #40) > It would also be great to use the new .inc in other parts of the code where > they are displayed, to avoid duplicated code (on a follow-up bug report?) Excellent idea. I'll file the bug report!
Created attachment 74765 [details] [review] Bug 2696 [QA Followup]: Fix POD, results sorting, permissions
I have fixed everything except: -- @@ +42,5 @@ > + > +sub debit { > + my ( $self ) = @_; > + my $debit_rs = $self->_result->debit; > + return unless $debit_rs; I do not think we should "return unless" here, we do not do that for other methods. Is there a good reason? -- which I believe to be incorrect. Please refer to Koha::Item::checkout for precedence. What you write makes sense *if* the foreign key *must* exist ( such as the case of items.biblionumber ). However, in this case, a checkout may not exist for this item ). Without that line, you would return a perfectly value but empty Koha::Checkout object. In that case code such as "if ( my $c = $item->checkout )" would by true instead of false like everyone would expect it to be. In fact, $item->checkout would *always* return true since the you'd get a Koha::Checkout object every time, valid or not. (In reply to Jonathan Druart from comment #39)
Kyle, please move the tests to a separate subtest and also test the "return undef" parts.
Created attachment 74817 [details] [review] Bug 2696 [QA Followup]: Add more test, move to subtest
Pushed to master for 18.05, thanks to everybody involved!
*** Bug 8016 has been marked as a duplicate of this bug. ***