The `void` routine in Koha::Account::Lines does restrict one to only being able to void a 'credit' accountline, however it does not distinguish between accounttypes. We do however, change the accounttype as part of the void routine to VOID and as such we may be loosing data, and thus audit trail, here. Credits can take the form of 'Payment', 'Writeoff', 'Lost Item Return', 'Fine Forgiven', 'Forgiven' and finally 'Credit'.
Actually.. we should be able to get to this original data via the offsets, walking backwards to find the first 'creation' offset... we don't however do that as yet and as such always display "Payment, voided" to the borrower for all such transactions.. this could be confusing to the end user?
I actually consider this a feature. It's always possible that something like a writeoff was done accidentally and needs to be voided. The rest of the credit types could be argued, but I don't since great utility in preventing them from being voided.
I don't think I gave this a good title.. I'll change it. I didn't really mean that's it's a bad thing that one can void all the different types of credit, but more that it's a bad thing that you loose the information as to what type of credit you have voided. In my opinion 'void' is a valid 'state' of any credit (and for that matter, I don't see why one shouldn't be able to void a debit too). See bug 22512 for more explanation, I may mark this bug as a duplicate at some point, or use to to encapsulate the corresponding work for the void part of adding states.
(In reply to Martin Renvoize from comment #3) > I don't think I gave this a good title.. I'll change it. > > I didn't really mean that's it's a bad thing that one can void all the > different types of credit, but more that it's a bad thing that you loose the > information as to what type of credit you have voided. > > In my opinion 'void' is a valid 'state' of any credit (and for that matter, > I don't see why one shouldn't be able to void a debit too). > > See bug 22512 for more explanation, I may mark this bug as a duplicate at > some point, or use to to encapsulate the corresponding work for the void > part of adding states. Yes, I agree completely. It would make more sense to have a 'voided' column, or a 'status' column that could have the state 'voided'. Are there other states we should consider for payments? A 'state' column could be re-used for 22512 which cuts now on the number of additional columns needed. What do you think?
Created attachment 86674 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86675 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I'm not also wondering about the logic of updating both 'amount' and 'amountoutstanding' here.. shouldn't we perhaps keep the 'amount' as the original value so one can see not only 'What' was voided but also 'How much'.
(In reply to Martin Renvoize from comment #7) > I'm not also wondering about the logic of updating both 'amount' and > 'amountoutstanding' here.. shouldn't we perhaps keep the 'amount' as the > original value so one can see not only 'What' was voided but also 'How much'. The logic behind the current behavior is to avoid the need to specifically remove VOIDs from reports when summing amounts. That data should be available the account offsets table. That being said, I'm not opposed to a change in this behavior.
This needs a bit of UI logic so new voided payments and old VOID types look the same in the UI. In addition, it would make sends to update accoulines with an accounttype of VOID to also have a status of VOID. Since future voided payments will retain the original accountype, it would be reasonable for Koha to show the original account type *and* show that it was voided.
Totally agree.. I wasn't fully ready for testing yet.. my bad for setting it to NSO.. I meant to leave it as NEW until I'd done more here.
(In reply to Martin Renvoize from comment #10) > Totally agree.. I wasn't fully ready for testing yet.. my bad for setting it > to NSO.. I meant to leave it as NEW until I'd done more here. NP! :)
Created attachment 86903 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86904 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86905 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86915 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86916 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86917 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87088 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87089 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87090 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87091 [details] [review] Bug 22511: (follow-up) Fix test plans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87221 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87222 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87223 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87224 [details] [review] Bug 22511: (follow-up) Fix test plans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87632 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87633 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87634 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87635 [details] [review] Bug 22511: (follow-up) Fix test plans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Freshly re-based and ready for SO
Created attachment 87650 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 87651 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 87652 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 87653 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 87654 [details] [review] Bug 22511: (follow-up) Fix test plans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 87704 [details] [review] Bug 22511: (follow-up) Add DB Update This update takes old VOID accountlines and attempts to restore the original accounttypes using the offsets and set the status to 'VOID' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87706 [details] [review] Bug 22511: (follow-up) Add DB Update This update takes old VOID accountlines and attempts to restore the original accounttypes using the offsets and set the status to 'VOID' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 87796 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87797 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87798 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87799 [details] [review] Bug 22511: (follow-up) Fix test plans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87800 [details] [review] Bug 22511: (follow-up) Add DB Update This update takes old VOID accountlines and attempts to restore the original accounttypes using the offsets and set the status to 'VOID' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Freshly rebased
Created attachment 87869 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87870 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87871 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87872 [details] [review] Bug 22511: (follow-up) Fix test plans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87873 [details] [review] Bug 22511: (follow-up) Add DB Update This update takes old VOID accountlines and attempts to restore the original accounttypes using the offsets and set the status to 'VOID' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Rebased again.. things are moving fast here at the moment :)
Created attachment 88707 [details] [review] Bug 22511: Update tests Updated tests to check for 'status' change and fixed 'accounttype' and moved from t/db_dependent/Accounts.t to the more appropriate t/db_dependent/Koha/Account/Lines.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 88708 [details] [review] Bug 22511: Update void method to use status We added a 'status' field to 'accountlines' in bug 22512 to allow for more fined grained control over 'types' and 'states'. This patch updates the void method to utilise this new field so we may keep the original credit type for later auditing should it be required. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 88709 [details] [review] Bug 22511: Update UI to use accountlines.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 88710 [details] [review] Bug 22511: (follow-up) Fix test plans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 88711 [details] [review] Bug 22511: (follow-up) Add DB Update This update takes old VOID accountlines and attempts to restore the original accounttypes using the offsets and set the status to 'VOID' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 88712 [details] [review] Bug 22511: (QA follow-up) Fix update database Test plan: Run db update (and have some voided accountlines) --> without patch it fails with error message similar to: Can't use string ("Payment") as a HASH ref while "strict refs" in use at (eval 1393) line 19. --> with patch it runs as expected Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 88713 [details] [review] Bug 22511: (QA follow-up) Refactor status description in templates Test plan: Before this patch, some accounttypes does not show status (for example voided payment) After this patch, the status is always generated - defaults to empty string This behaviour should be same in intranet and opac In OPAC templates there was even bug (used variable account instead of ACCOUNt_LINE), so the status was not shown at all Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Martin, it would be nice if you could sign-off my follow-ups ;)
Created attachment 88714 [details] [review] Bug 22511: (QA follow-up) Fix update database Test plan: Run db update (and have some voided accountlines) --> without patch it fails with error message similar to: Can't use string ("Payment") as a HASH ref while "strict refs" in use at (eval 1393) line 19. --> with patch it runs as expected Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88715 [details] [review] Bug 22511: (QA follow-up) Refactor status description in templates Test plan: Before this patch, some accounttypes does not show status (for example voided payment) After this patch, the status is always generated - defaults to empty string This behaviour should be same in intranet and opac In OPAC templates there was even bug (used variable account instead of ACCOUNt_LINE), so the status was not shown at all Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for the followups, nice bit of tidying and makes great sense.
(In reply to Martin Renvoize from comment #59) > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks Martin!
Created attachment 88748 [details] [review] Bug 22511: Fix type role->roll
(In reply to Nick Clemens from comment #62) > Created attachment 88748 [details] [review] [review] > Bug 22511: Fix type role->roll Will fix List item->Lost item in the DB update on pushing
(In reply to Nick Clemens from comment #63) > (In reply to Nick Clemens from comment #62) > > Created attachment 88748 [details] [review] [review] [review] > > Bug 22511: Fix type role->roll > > Will fix List item->Lost item in the DB update on pushing Silly me, I oversee it... thanks Nick
Awesome work all! Pushed to master for 19.05
Enhancement will not be backported to 18.11.x series.