Bug 22435

I'm getting keener to ratify these offset lines so we don't record an 'add' line for every accountline we add.

I think it makes more logical sense to only record 'changes'.. i.e offsets (and adjustments) rather than having lots of lines with either an empty debit_id or credit_id.. the really useful lines are the ones with both _id's filled.

This would also have the side effect of removing a number of offset_types that would no longer be required so we were only recording the offset 'action' as the type.

(In reply to Martin Renvoize from comment #1)
> I'm getting keener to ratify these offset lines so we don't record an 'add'
> line for every accountline we add.
> 
> I think it makes more logical sense to only record 'changes'.. i.e offsets
> (and adjustments) rather than having lots of lines with either an empty
> debit_id or credit_id.. the really useful lines are the ones with both _id's
> filled.
> 
> This would also have the side effect of removing a number of offset_types
> that would no longer be required so we were only recording the offset
> 'action' as the type.

Offsets are meant to be a full record of all changes to accountlines, not just interactions between debits and credits when payments/writeoffs are made. This includes the initial balance of any newly created accountline. 

For example, it's very useful to have the offsets that show how a fine has accrued over time.If it's "wrong" and we see in the offsets that it was accruing .10 a day, then suddenly jumped to .25 a day, we can infer that perhaps the circulation rule was changed, or which rule was selected.

You'd still get the increments via the adjust method.. it's the 'create' I'm less enamoured by.

We have huge numbers of lines for the creation of an accountline that don't really add any information.. at the moment only fines can be adjusted without having a corresponding offsetting accountline.

Even if we don't agree to get rid of them entirely.. could we perhaps ratify their codes?


i.e. I would say it's significantly clearer for all the 'create' actions in the offsets to have the same type..

accountline.debit_type_code = OVERDUE + offset.type = CREATE
accountline.debit_type_code = ACCOUNT + offset.type = CREATE

etc...

as opposed to

accountline.debit_type_code = OVERDUE + offset.type = OVERDUE
accountline.debit_type_code = ACCOUNT + offest.type = ACCOUNT

> i.e. I would say it's significantly clearer for all the 'create' actions in
> the offsets to have the same type..
> 
> accountline.debit_type_code = OVERDUE + offset.type = CREATE
> accountline.debit_type_code = ACCOUNT + offset.type = CREATE
> 
> etc...
> 
> as opposed to
> 
> accountline.debit_type_code = OVERDUE + offset.type = OVERDUE
> accountline.debit_type_code = ACCOUNT + offest.type = ACCOUNT


Yes, I'm absolutely down with that!

Work in progress.. needs a DB update.

The more I work on this the more I feel we can distil the offset types down to just the following list:

'CREATE' - For the creation of the accountline
'APPLY'  - For when the accountlines are applied against each other
'OVERDUE_INCREASE' - For the fine increase adjustments calculated by the corn
'OVERDUE_DECREASE' - For the fine decrease adjustment that may happen at checkin/renewal time

With said list.. I think we could drop the offset_types table entirely and put them into an enum on the field instead.

Add the following to the list:

'VOID' - For when we're reversing a payment

Created attachment 120043 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Created attachment 121012 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Created attachment 121014 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details teplate and controller
to show a full history of what's happend to the accountlines passed,
including creation, increment/decrement, payments, cancellation and
voids.

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Created attachment 121016 [details] [review]
Bug 22435: Further refinements to account-details table

At this point, I'd love some feedback on the account-details page updates.... I think the two patches that affect it can be merged, but I'd like to know if we're showing enough detail on each line.. if it's the right detail and is clear.

I also need to add a Unit test for the introduction of the 'manager' relation on Koha::Account::Line and I think we could move from the account_offset_types table to a simple enum column at this point.

Created attachment 121028 [details] [review]
Bug 22435: Alternative Three - Account Details

Another alternative to try, feedback welcome.

> 3/ Compare before patch and after for the Details page.

IIUC, I should compare between
- same as master: before "Update accountline-details page to show all history"
- alt 1 at "Update accountline-details page to show all history"
- alt 2 at "Further refinements to account-details table"
- alt 3 at "Alternative Three - Account Details"

Which are all "data compatible" so I can do step 1/ once and switch between the four code-states to be compared.

1/ Create some credits and debits and apply them to each other in
various combinations

* lost item manual invoice
* two partial payment of it
* final full payment

* create credit
* 2 partial payouts
* void payment

* create credit
* 1 partial payouts
* 1 final payout

What are the other action doable with credits and debits ?

With
- alt 1 at "Update accountline-details page to show all history"

I get crashed about FK constraint.


It tried a unorthodox method to solve a patch not applying:

* sha1 information is lacking or useless
git am --show-current-patch=diff | patch -p1 --merge
git add THE RELEVANT FILES

# when git am is used by git bz
git bz apply --continue

# check comparing the diff of the resulting commit with the original patch
# cat is to remove paging
git show db11ac2237  | cat

# when it's done, if your BZ had a dependency, check it was added because the
# issue might have caused the main patch to be dropped and only the dependency
# to be added.


---

So it might have not worked.
Or there might be an actually issue.
Or it's not meant to work on commit ""Update accountline-details page to show all history""

Created attachment 121757 [details] [review]
Bug 22435: Update CREATE offset types

Created attachment 121758 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account

This patch updates the Koha::Account methods for adding debits and
credits to use the 'CREATE' offset type for the creation record.

Created attachment 121759 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line

This patch updates the Koha::Account::Line methods that result in new account
lines to use the 'CREATE' offset type for the creation record.

Created attachment 121760 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Created attachment 121761 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Created attachment 121762 [details] [review]
Bug 22435: Update unit tests

This patch updates the unit tests to reflect the changes we have made to
offset types.  We do two things:

1) Clean up calls to 'apply' to drop passing offset_type
2) Update test code to look for APPLY and CREATE instead of specific
   offset types.

Created attachment 121763 [details] [review]
Bug 22435: Fix CREATE offset amounts

Updateing the tests highlighted a mistake in the original code.. the
offsets for the cancel and reduce creation lines were incorrectly
signed.

Created attachment 121764 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Created attachment 121765 [details] [review]
Bug 22435: Update for bug 27049

Created attachment 121766 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details teplate and controller
to show a full history of what's happend to the accountlines passed,
including creation, increment/decrement, payments, cancellation and
voids.

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Created attachment 121767 [details] [review]
Bug 22435: Improve database update

Created attachment 121768 [details] [review]
Bug 22435: Further refinements to account-details table

Created attachment 121769 [details] [review]
Bug 22435: Alternative Three - Account Details

Another alternative to try, feedback welcome.

I've rebased...

The final three patches apply one after the other.. so you can do an interactive bz apply and pick all but the last two.. (run the db update) and test..  then do another interactive bz apply and just send the next patch and again for the third.

I personally think the third is a nice option so perhaps it might be easier to just test the final full patchset and compare to without master?

You've picked a few good actions there.. adding a fee, paying it (fully and partially).. using one payment to pay for multiple debts.. voiding payments (before and after they've been applied in part or in full against some debts).. cancelling debts, partial writeoffs etc.

Basically.. we just want to work out if the details page gives a clear description of all the actions that have taken place in a payment/charges history.

Created attachment 121960 [details] [review]
Bug 22435: Update CREATE offset types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121961 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account

This patch updates the Koha::Account methods for adding debits and
credits to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121962 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line

This patch updates the Koha::Account::Line methods that result in new account
lines to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121963 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121964 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121965 [details] [review]
Bug 22435: Update unit tests

This patch updates the unit tests to reflect the changes we have made to
offset types.  We do two things:

1) Clean up calls to 'apply' to drop passing offset_type
2) Update test code to look for APPLY and CREATE instead of specific
   offset types.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121966 [details] [review]
Bug 22435: Fix CREATE offset amounts

Updateing the tests highlighted a mistake in the original code.. the
offsets for the cancel and reduce creation lines were incorrectly
signed.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121967 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121968 [details] [review]
Bug 22435: Update for bug 27049

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121969 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details teplate and controller
to show a full history of what's happend to the accountlines passed,
including creation, increment/decrement, payments, cancellation and
voids.

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121970 [details] [review]
Bug 22435: Improve database update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121971 [details] [review]
Bug 22435: Further refinements to account-details table

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 121972 [details] [review]
Bug 22435: Alternative Three - Account Details

Another alternative to try, feedback welcome.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Looks good as far as I understand all this Koha money stuff.
Which is not as much as I would like to ^^"

Here is a gallery of master VS alternative 3. Since I don't know enough to tell if all the info and presentation is relevant, that will help other reviewers.

It will open in gallery mode and the left and right arrows allow to navigate.
They are ordered like
Transaction 1 with master
Transaction 1 with patch alt 3
Transaction 2 with master
Transaction 2 with patch alt 3
[...]

It's possible to download the images in archives to have them as files and see the names which can help keeping track of the above mentioned structure.
It's a temporary hosting for 90 days.

https://wtf.roflcopter.fr/pics/gallery#Q87QIysB/u4Zn1DoM.png,TyMWRSvw/zoKtB5MU.png,EZ3ymXo4/tegViGbi.png,2d4ztt1m/2G4MZlXf.png,SqnXeQEg/z9EIAAsp.png,tpqIHe05/sdIdLzoI.png,JOyHDhAd/kg03SVjn.png,qqxjtCsp/IzZmLQaB.png,I9eYXgp8/0S7sxkDZ.png,UvOcry0z/M3jsfjoe.png,auIkyTxQ/7iTjYqq7.png,XxSWPXbh/CsqyYMYh.png,Bp7jBRgo/NuSWhpQR.png,MS54T2vx/slv4blYa.png,D2Kw38dO/TTkgsn6I.png,UwfnG840/jQaEllak.png,RskobMTp/jsdstvq4.png,1wkf3RIq/QfSAsDCQ.png,59CsFLXw/iMDfvN76.png,KLogubC6/c5Fnwq54.png,vXwV3J3Y/FBEt3pPn.png,VoNmDbbn/xF4Kj7R0.png,Jq0Nx6Xi/xHiu1S9h.png,KPx6OBNh/ihggjuny.png,NPMLhU7g/KVhCGEOG.png,d5CwLjXH/k3xLva3p.png,DvIb09up/wITsiD7r.png,gK70Kqzu/1vQlc7Xt.png&gid=1&pid=1

Here are alternatives 1 and 2. With only 3 operations detailed.

== 1 ===
https://wtf.roflcopter.fr/pics/gallery#1NkLBOFC/aHHujYYJ.png,XZE1IKRG/ZOwluV8N.png,6zXftiyA/slGr0IuQ.png,rAKHrc99/Q07IxtMu.png,bP6n93Xy/q3Ev9Laf.png,rSTNMCLN/ibOUW5cN.png&gid=1&pid=2

== 2 ==
https://wtf.roflcopter.fr/pics/gallery#uj6eTF8G/cbf4sgh4.png,h3yzYYAw/mkUqeWPg.png,puNvDL1X/pEMzQBxJ.png,a40ZrUlU/r9pOQeJ2.png,s6POaFzd/URABzJEO.png,FQBJQ0rP/EjtE5Tkg.png&gid=1&pid=1

Created attachment 122148 [details] [review]
Bug 22435: Update CREATE offset types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122149 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account

This patch updates the Koha::Account methods for adding debits and
credits to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122150 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line

This patch updates the Koha::Account::Line methods that result in new account
lines to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122151 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122152 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122153 [details] [review]
Bug 22435: Update unit tests

This patch updates the unit tests to reflect the changes we have made to
offset types.  We do two things:

1) Clean up calls to 'apply' to drop passing offset_type
2) Update test code to look for APPLY and CREATE instead of specific
   offset types.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122154 [details] [review]
Bug 22435: Fix CREATE offset amounts

Updateing the tests highlighted a mistake in the original code.. the
offsets for the cancel and reduce creation lines were incorrectly
signed.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122155 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122156 [details] [review]
Bug 22435: Update for bug 27049

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122157 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details teplate and controller
to show a full history of what's happend to the accountlines passed,
including creation, increment/decrement, payments, cancellation and
voids.

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122158 [details] [review]
Bug 22435: Improve database update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122159 [details] [review]
Bug 22435: Further refinements to account-details table

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122160 [details] [review]
Bug 22435: Alternative Three - Account Details

Another alternative to try, feedback welcome.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122161 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

I ran through all I could think of, things look good after these patches and tests pass, code reads well, this is a good early candidate to push for further testing

Nice work, Martin!

1. Can you summarize what is done in this patchset please? A full test plan would be nice too.

2. APPLY is not in account_offset_types.sql

3.
+    $dbh->do( "UPDATE account_offsets SET type = 'APPLY' WHERE type != 'OVERDUE_INCREASE' AND type != 'OVERDUE_DECREASE' AND type != 'CREATE'" );

Hum, is this query correct? Aren't we missing a condition?

4. Tests missing for Koha::Account::Line->manager

Created attachment 122235 [details] [review]
Bug 22435: Update CREATE offset types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122236 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account

This patch updates the Koha::Account methods for adding debits and
credits to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122237 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line

This patch updates the Koha::Account::Line methods that result in new account
lines to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122238 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122239 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122240 [details] [review]
Bug 22435: Update unit tests

This patch updates the unit tests to reflect the changes we have made to
offset types.  We do two things:

1) Clean up calls to 'apply' to drop passing offset_type
2) Update test code to look for APPLY and CREATE instead of specific
   offset types.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122241 [details] [review]
Bug 22435: Fix CREATE offset amounts

Updateing the tests highlighted a mistake in the original code.. the
offsets for the cancel and reduce creation lines were incorrectly
signed.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122242 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122243 [details] [review]
Bug 22435: Update for bug 27049

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122244 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details template and controller
to show the full history of what's happend to the accountline passed.
This includes the creation, any increments/decrements (for fines), and
finally any offsets against the total (payments, cancellations, voids).

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations.
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122245 [details] [review]
Bug 22435: Improve database update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122246 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122247 [details] [review]
Bug 22435: (follow-up) Update POS for reduced offset types

This patch updates the Koha::Charges::Sales class to use the 'CREATE'
and 'APPLY' offset types.

Test plan
1/ Run t/db_dependent/Koha/Charges/Sales.t

Created attachment 122248 [details] [review]
Bug 22435: (follow-up) Drop account_offset_types table

This patch drops the now defunkt account_offset_types table and ensure
the type field in account_offsets is instead an enum type column.

Created attachment 122249 [details] [review]
Bug 22435: (QA follow-up) Add missing test for manager relation

Created attachment 122250 [details] [review]
Bug 22435: (QA follow-up) Fix test for offset total

This patch updates the test for the offset total method to adhere to the
new restricted set of offset types available.

Created attachment 122251 [details] [review]
Bug 22435: Update DBIC Schema's

Created attachment 122252 [details] [review]
Bug 22435: (QA follow-up) Fix returned offsets in moredetail

This patch updates the offsets query for lost item returns displayed in
the catalogue/moredetail controller.

Test plan
1/ Check item detail display for a lost item at various stages of lost
and charged for (Lost, Writeoff, Forgiven, Paid, Found)
2/ Compare before the patchset and after - There should be no difference

So, I managed to miss a commit from Bugzilla.. but whilst retrieving it I spotted a couple of issues which I've also dealt with as follow-ups here (as well as including a Schema Update for ease of testing).

I've also updated the 'Text for release notes' a bit to embellish the details a little more.

Created attachment 122253 [details] [review]
Bug 22435: (QA follow-up) More test updates

Created attachment 122256 [details] [review]
Bug 22435: Update CREATE offset types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122257 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account

This patch updates the Koha::Account methods for adding debits and
credits to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122258 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line

This patch updates the Koha::Account::Line methods that result in new account
lines to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122259 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122260 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122261 [details] [review]
Bug 22435: Update unit tests

This patch updates the unit tests to reflect the changes we have made to
offset types.  We do two things:

1) Clean up calls to 'apply' to drop passing offset_type
2) Update test code to look for APPLY and CREATE instead of specific
   offset types.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122262 [details] [review]
Bug 22435: Fix CREATE offset amounts

Updateing the tests highlighted a mistake in the original code.. the
offsets for the cancel and reduce creation lines were incorrectly
signed.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122263 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122264 [details] [review]
Bug 22435: Update for bug 27049

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122265 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details template and controller
to show the full history of what's happend to the accountline passed.
This includes the creation, any increments/decrements (for fines), and
finally any offsets against the total (payments, cancellations, voids).

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations.
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122266 [details] [review]
Bug 22435: Improve database update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122267 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 122268 [details] [review]
Bug 22435: (follow-up) Update POS for reduced offset types

This patch updates the Koha::Charges::Sales class to use the 'CREATE'
and 'APPLY' offset types.

Test plan
1/ Run t/db_dependent/Koha/Charges/Sales.t

Created attachment 122269 [details] [review]
Bug 22435: (follow-up) Drop account_offset_types table

This patch drops the now defunkt account_offset_types table and ensure
the type field in account_offsets is instead an enum type column.

Created attachment 122270 [details] [review]
Bug 22435: (QA follow-up) Add missing test for manager relation

Created attachment 122271 [details] [review]
Bug 22435: (QA follow-up) Fix test for offset total

This patch updates the test for the offset total method to adhere to the
new restricted set of offset types available.

Created attachment 122272 [details] [review]
Bug 22435: Update DBIC Schema's

Created attachment 122273 [details] [review]
Bug 22435: (QA follow-up) Fix returned offsets in moredetail

This patch updates the offsets query for lost item returns displayed in
the catalogue/moredetail controller.

Test plan (Same as bug 19919)
Test plan:
1) Pay off a LOST item
2) Check for the associated display of 'Paidfor?:' on the itemdetails page
3) Writeoff a LOST item
4) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Created attachment 122274 [details] [review]
Bug 22435: (QA follow-up) More test updates

Created attachment 122553 [details] [review]
Bug 22435: Update CREATE offset types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122554 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account

This patch updates the Koha::Account methods for adding debits and
credits to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122555 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line

This patch updates the Koha::Account::Line methods that result in new account
lines to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122556 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122557 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122558 [details] [review]
Bug 22435: Update unit tests

This patch updates the unit tests to reflect the changes we have made to
offset types.  We do two things:

1) Clean up calls to 'apply' to drop passing offset_type
2) Update test code to look for APPLY and CREATE instead of specific
   offset types.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122559 [details] [review]
Bug 22435: Fix CREATE offset amounts

Updateing the tests highlighted a mistake in the original code.. the
offsets for the cancel and reduce creation lines were incorrectly
signed.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122560 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122561 [details] [review]
Bug 22435: Update for bug 27049

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122562 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details template and controller
to show the full history of what's happend to the accountline passed.
This includes the creation, any increments/decrements (for fines), and
finally any offsets against the total (payments, cancellations, voids).

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations.
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122563 [details] [review]
Bug 22435: Improve database update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122564 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122565 [details] [review]
Bug 22435: (follow-up) Update POS for reduced offset types

This patch updates the Koha::Charges::Sales class to use the 'CREATE'
and 'APPLY' offset types.

Test plan
1/ Run t/db_dependent/Koha/Charges/Sales.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122566 [details] [review]
Bug 22435: (follow-up) Drop account_offset_types table

This patch drops the now defunkt account_offset_types table and ensure
the type field in account_offsets is instead an enum type column.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122567 [details] [review]
Bug 22435: (QA follow-up) Add missing test for manager relation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122568 [details] [review]
Bug 22435: (QA follow-up) Fix test for offset total

This patch updates the test for the offset total method to adhere to the
new restricted set of offset types available.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122569 [details] [review]
Bug 22435: Update DBIC Schema's

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122570 [details] [review]
Bug 22435: (QA follow-up) Fix returned offsets in moredetail

This patch updates the offsets query for lost item returns displayed in
the catalogue/moredetail controller.

Test plan (Same as bug 19919)
Test plan:
1) Pay off a LOST item
2) Check for the associated display of 'Paidfor?:' on the itemdetails page
3) Writeoff a LOST item
4) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122571 [details] [review]
Bug 22435: (QA follow-up) More test updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Very nice and welcome enhancement. This was really the missing piece in the cleanup. Simplifynig things everywhere.
Nice work on the transaction history BTW.

Created attachment 122586 [details] [review]
Bug 22435: (follow-up) Set CREATE offset for credits to positive

The history view of an accountline shows increases and decreases in the
amount outstanding from the perspective of the credit/debit line you are
inspecting.

Apply creates a negative offset amount which acts to 'reduce' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.  Starting with a negative offset for the credit thus
makes this nonsenical.

Void creates positive offset amounts which act to 'restore' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.

Created attachment 122899 [details] [review]
Bug 22435: Update CREATE offset types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122900 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account

This patch updates the Koha::Account methods for adding debits and
credits to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122901 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line

This patch updates the Koha::Account::Line methods that result in new account
lines to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122902 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122903 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122904 [details] [review]
Bug 22435: Update unit tests

This patch updates the unit tests to reflect the changes we have made to
offset types.  We do two things:

1) Clean up calls to 'apply' to drop passing offset_type
2) Update test code to look for APPLY and CREATE instead of specific
   offset types.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122905 [details] [review]
Bug 22435: Fix CREATE offset amounts

Updateing the tests highlighted a mistake in the original code.. the
offsets for the cancel and reduce creation lines were incorrectly
signed.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122906 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122907 [details] [review]
Bug 22435: Update for bug 27049

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122908 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details template and controller
to show the full history of what's happend to the accountline passed.
This includes the creation, any increments/decrements (for fines), and
finally any offsets against the total (payments, cancellations, voids).

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations.
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122909 [details] [review]
Bug 22435: Improve database update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122910 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122911 [details] [review]
Bug 22435: (follow-up) Update POS for reduced offset types

This patch updates the Koha::Charges::Sales class to use the 'CREATE'
and 'APPLY' offset types.

Test plan
1/ Run t/db_dependent/Koha/Charges/Sales.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122912 [details] [review]
Bug 22435: (follow-up) Drop account_offset_types table

This patch drops the now defunkt account_offset_types table and ensure
the type field in account_offsets is instead an enum type column.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122913 [details] [review]
Bug 22435: (QA follow-up) Add missing test for manager relation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122914 [details] [review]
Bug 22435: (QA follow-up) Fix test for offset total

This patch updates the test for the offset total method to adhere to the
new restricted set of offset types available.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122915 [details] [review]
Bug 22435: Update DBIC Schema's

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122916 [details] [review]
Bug 22435: (QA follow-up) Fix returned offsets in moredetail

This patch updates the offsets query for lost item returns displayed in
the catalogue/moredetail controller.

Test plan (Same as bug 19919)
Test plan:
1) Pay off a LOST item
2) Check for the associated display of 'Paidfor?:' on the itemdetails page
3) Writeoff a LOST item
4) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122917 [details] [review]
Bug 22435: (QA follow-up) More test updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 122918 [details] [review]
Bug 22435: (follow-up) Set CREATE offset for credits to positive

The history view of an accountline shows increases and decreases in the
amount outstanding from the perspective of the credit/debit line you are
inspecting.

Apply creates a negative offset amount which acts to 'reduce' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.  Starting with a negative offset for the credit thus
makes this nonsenical.

Void creates positive offset amounts which act to 'restore' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Just a quick rebase ;)

Does that invalidate the passed QA stamp?

Created attachment 122937 [details] [review]
Bug 22435: (follow-up) Set CREATE offset for credits to positive

The history view of an accountline shows increases and decreases in the
amount outstanding from the perspective of the credit/debit line you are
inspecting.

Apply creates a negative offset amount which acts to 'reduce' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.  Starting with a negative offset for the credit thus
makes this nonsenical.

Void creates positive offset amounts which act to 'restore' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

JD Amended patch: Remove trailing spaces

Martin, shouldn't we apply the following change to make the db rev idempotent?

-    $dbh->do( "UPDATE account_offsets SET type = 'CREATE' WHERE type != 'OVERDUE_INCREASE' AND type != 'OVERDUE_DECREASE' AND ( debit_id IS NULL OR credit_id IS NULL)" );
+    $dbh->do( "UPDATE account_offsets SET type = 'CREATE' WHERE type != 'OVERDUE_INCREASE' AND type != 'OVERDUE_DECREASE' AND type != 'APPLY' AND ( debit_id IS NULL OR credit_id IS NULL)" );

(In reply to Jonathan Druart from comment #192)
> Martin, shouldn't we apply the following change to make the db rev
> idempotent?
> 
> -    $dbh->do( "UPDATE account_offsets SET type = 'CREATE' WHERE type !=
> 'OVERDUE_INCREASE' AND type != 'OVERDUE_DECREASE' AND ( debit_id IS NULL OR
> credit_id IS NULL)" );
> +    $dbh->do( "UPDATE account_offsets SET type = 'CREATE' WHERE type !=
> 'OVERDUE_INCREASE' AND type != 'OVERDUE_DECREASE' AND type != 'APPLY' AND (
> debit_id IS NULL OR credit_id IS NULL)" );

Hmm, it wouldn't do any harm, though I believe we shouldn't have any cases where APPLY exists where debit_id or credit_id is null.

Thanks for the whitespace fix, I'll have to double check my git config.. seems I lost something on the last reset of that environment.

(In reply to Martin Renvoize from comment #193)
> (In reply to Jonathan Druart from comment #192)
> > Martin, shouldn't we apply the following change to make the db rev
> > idempotent?
> > 
> > -    $dbh->do( "UPDATE account_offsets SET type = 'CREATE' WHERE type !=
> > 'OVERDUE_INCREASE' AND type != 'OVERDUE_DECREASE' AND ( debit_id IS NULL OR
> > credit_id IS NULL)" );
> > +    $dbh->do( "UPDATE account_offsets SET type = 'CREATE' WHERE type !=
> > 'OVERDUE_INCREASE' AND type != 'OVERDUE_DECREASE' AND type != 'APPLY' AND (
> > debit_id IS NULL OR credit_id IS NULL)" );
> 
> Hmm, it wouldn't do any harm, though I believe we shouldn't have any cases
> where APPLY exists where debit_id or credit_id is null.

Correct, forget that!

t/db_dependent/Accounts.t .. 17/27 
    #   Failed test 'First offset amount is -100.00'
    #   at t/db_dependent/Accounts.t line 379.
    #          got: '100'
    #     expected: '-100'
    # Looks like you failed 1 test of 12.
t/db_dependent/Accounts.t .. 20/27 
#   Failed test 'More Koha::Account::pay tests'
#   at t/db_dependent/Accounts.t line 400.

    #   Failed test 'First offset amount is -60.00'
    #   at t/db_dependent/Accounts.t line 452.
    #          got: '60'
    #     expected: '-60'
    # Looks like you failed 1 test of 12.
t/db_dependent/Accounts.t .. 21/27 
#   Failed test 'Even more Koha::Account::pay tests'
#   at t/db_dependent/Accounts.t line 473.

Created attachment 123423 [details] [review]
Bug 22435: Update CREATE offset types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123424 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account

This patch updates the Koha::Account methods for adding debits and
credits to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123425 [details] [review]
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line

This patch updates the Koha::Account::Line methods that result in new account
lines to use the 'CREATE' offset type for the creation record.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123426 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123427 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123428 [details] [review]
Bug 22435: Update unit tests

This patch updates the unit tests to reflect the changes we have made to
offset types.  We do two things:

1) Clean up calls to 'apply' to drop passing offset_type
2) Update test code to look for APPLY and CREATE instead of specific
   offset types.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123429 [details] [review]
Bug 22435: Fix CREATE offset amounts

Updateing the tests highlighted a mistake in the original code.. the
offsets for the cancel and reduce creation lines were incorrectly
signed.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123430 [details] [review]
Bug 22435: Fix _set_found_trigger

The _set_found_trigger utilised the 'Writeoff' offset type to
distinguish between the application of a writeoff and any other form of
offset application.

This patch updates the trigger to use the full link through from offset
to account credit line to get the credit type being offset.

Test plan
1/ Run t/db_dependent/Koha/Items.t and prove it fails before the patch,
but passes after applying this patch.
2/ Run t/db_dependent/Koha/Account/Offsets.t and prove it passes both
before and after applying this patch.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123431 [details] [review]
Bug 22435: Update for bug 27049

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123432 [details] [review]
Bug 22435: Update accountline-details page to show all history

The accountline-details page took a simplistic approach to displaying
the history of an accountline. This patch drops the now defunct
offset_type_description template block from it's respective include
files, and then updates the accountline-details template and controller
to show the full history of what's happend to the accountline passed.
This includes the creation, any increments/decrements (for fines), and
finally any offsets against the total (payments, cancellations, voids).

Test plan
1/ Create some credits and debits and apply them to each other in
various combinations.
2/ View the 'Details' page for some of the debit/credits
3/ Compare before patch and after for the Details page.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123433 [details] [review]
Bug 22435: Improve database update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123434 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123435 [details] [review]
Bug 22435: (follow-up) Update POS for reduced offset types

This patch updates the Koha::Charges::Sales class to use the 'CREATE'
and 'APPLY' offset types.

Test plan
1/ Run t/db_dependent/Koha/Charges/Sales.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123436 [details] [review]
Bug 22435: (follow-up) Drop account_offset_types table

This patch drops the now defunkt account_offset_types table and ensure
the type field in account_offsets is instead an enum type column.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123437 [details] [review]
Bug 22435: (QA follow-up) Add missing test for manager relation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123438 [details] [review]
Bug 22435: (QA follow-up) Fix test for offset total

This patch updates the test for the offset total method to adhere to the
new restricted set of offset types available.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123439 [details] [review]
Bug 22435: Update DBIC Schema's

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123440 [details] [review]
Bug 22435: (QA follow-up) Fix returned offsets in moredetail

This patch updates the offsets query for lost item returns displayed in
the catalogue/moredetail controller.

Test plan (Same as bug 19919)
Test plan:
1) Pay off a LOST item
2) Check for the associated display of 'Paidfor?:' on the itemdetails page
3) Writeoff a LOST item
4) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123441 [details] [review]
Bug 22435: (QA follow-up) More test updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123442 [details] [review]
Bug 22435: (follow-up) Set CREATE offset for credits to positive

The history view of an accountline shows increases and decreases in the
amount outstanding from the perspective of the credit/debit line you are
inspecting.

Apply creates a negative offset amount which acts to 'reduce' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.  Starting with a negative offset for the credit thus
makes this nonsenical.

Void creates positive offset amounts which act to 'restore' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 123443 [details] [review]
Bug 22435: (follow-up) Set CREATE offset for credits to positive

The history view of an accountline shows increases and decreases in the
amount outstanding from the perspective of the credit/debit line you are
inspecting.

Apply creates a negative offset amount which acts to 'reduce' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.  Starting with a negative offset for the credit thus
makes this nonsenical.

Void creates positive offset amounts which act to 'restore' both the
outstanding 'debt' for the debit line and the outstanding 'credit' for
the credit line.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Yup, I neglected to add the fix for that test to my final commit the first time around!

All fixed now.. and rebased.

Pushed to master for 21.11, thanks to everybody involved!

*** Bug 24382 has been marked as a duplicate of this bug. ***