Bug 22435

Summary: Clarify account_offset types by converting them to clear codes
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Fines and feesAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, jonathan.druart, josef.moravec, katrin.fischer, kyle, nick, sally.healey, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement serves to clarify how the account offsets table functions. We record all account actions in this table, including accountline creations, modifications and offsets. Prior to this patch we had a large number of different offset types, one for each accountline type. But we didn't clearly define what the offset was actually "doing". This patch replaces the existing offset types with a refined list; `CREATE`, `APPLY`, `OVERDUE_INCREASE`, `OVERDUE_DECREASE` and `VOID`. The accountline details page, accessible from the borrower account transactions table is updated to display the whole history of the selected accountline, including creation (CREATE), increments (OVERDUE_INCREASE/DECREASE) and offsets (Application of payments, cancellations, voids, writeoffs and refunds).
Version(s) released in:
21.11.00
Bug Depends on: 27636    
Bug Blocks: 17735, 28656, 28421, 29158    
Attachments: Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: Further refinements to account-details table
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: Further refinements to account-details table
Bug 22435: Alternative Three - Account Details
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: Further refinements to account-details table
Bug 22435: Alternative Three - Account Details
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: Further refinements to account-details table
Bug 22435: Alternative Three - Account Details
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: Further refinements to account-details table
Bug 22435: Alternative Three - Account Details
Bug 22435: (QA follow-up) Fix test file and adjust test
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: (QA follow-up) Fix test file and adjust test
Bug 22435: (follow-up) Update POS for reduced offset types
Bug 22435: (follow-up) Drop account_offset_types table
Bug 22435: (QA follow-up) Add missing test for manager relation
Bug 22435: (QA follow-up) Fix test for offset total
Bug 22435: Update DBIC Schema's
Bug 22435: (QA follow-up) Fix returned offsets in moredetail
Bug 22435: (QA follow-up) More test updates
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: (QA follow-up) Fix test file and adjust test
Bug 22435: (follow-up) Update POS for reduced offset types
Bug 22435: (follow-up) Drop account_offset_types table
Bug 22435: (QA follow-up) Add missing test for manager relation
Bug 22435: (QA follow-up) Fix test for offset total
Bug 22435: Update DBIC Schema's
Bug 22435: (QA follow-up) Fix returned offsets in moredetail
Bug 22435: (QA follow-up) More test updates
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: (QA follow-up) Fix test file and adjust test
Bug 22435: (follow-up) Update POS for reduced offset types
Bug 22435: (follow-up) Drop account_offset_types table
Bug 22435: (QA follow-up) Add missing test for manager relation
Bug 22435: (QA follow-up) Fix test for offset total
Bug 22435: Update DBIC Schema's
Bug 22435: (QA follow-up) Fix returned offsets in moredetail
Bug 22435: (QA follow-up) More test updates
Bug 22435: (follow-up) Set CREATE offset for credits to positive
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: (QA follow-up) Fix test file and adjust test
Bug 22435: (follow-up) Update POS for reduced offset types
Bug 22435: (follow-up) Drop account_offset_types table
Bug 22435: (QA follow-up) Add missing test for manager relation
Bug 22435: (QA follow-up) Fix test for offset total
Bug 22435: Update DBIC Schema's
Bug 22435: (QA follow-up) Fix returned offsets in moredetail
Bug 22435: (QA follow-up) More test updates
Bug 22435: (follow-up) Set CREATE offset for credits to positive
Bug 22435: (follow-up) Set CREATE offset for credits to positive
Bug 22435: Update CREATE offset types
Bug 22435: Use 'CREATE' offset type in Koha::Account
Bug 22435: Use 'CREATE' offset type in Koha::Account::Line
Bug 22435: Add 'APPLY' to account_offset_types
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Bug 22435: Update unit tests
Bug 22435: Fix CREATE offset amounts
Bug 22435: Fix _set_found_trigger
Bug 22435: Update for bug 27049
Bug 22435: Update accountline-details page to show all history
Bug 22435: Improve database update
Bug 22435: (QA follow-up) Fix test file and adjust test
Bug 22435: (follow-up) Update POS for reduced offset types
Bug 22435: (follow-up) Drop account_offset_types table
Bug 22435: (QA follow-up) Add missing test for manager relation
Bug 22435: (QA follow-up) Fix test for offset total
Bug 22435: Update DBIC Schema's
Bug 22435: (QA follow-up) Fix returned offsets in moredetail
Bug 22435: (QA follow-up) More test updates
Bug 22435: (follow-up) Set CREATE offset for credits to positive
Bug 22435: (follow-up) Set CREATE offset for credits to positive

Description Martin Renvoize 2019-02-28 15:55:04 UTC
Currently, account_offset.types are all descriptive strings.. they should really be 'codes' to distinguish them from user-facing data and discourage developers from adding more untranslatable strings at the database level.

I also don't think the current strings make a great deal of sense as they're often not describing an 'offset' action but the account action itself which is already better defined within the accountline.accounttype fields.
Comment 1 Martin Renvoize 2021-02-05 15:20:15 UTC
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.
Comment 2 Kyle M Hall 2021-02-05 15:32:46 UTC
(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.
Comment 3 Martin Renvoize 2021-02-05 17:32:17 UTC
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
Comment 4 Kyle M Hall 2021-02-09 19:20:23 UTC
> 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!
Comment 5 Martin Renvoize 2021-03-16 17:20:28 UTC Comment hidden (obsolete)
Comment 6 Martin Renvoize 2021-03-16 17:20:33 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize 2021-03-16 17:21:01 UTC
Work in progress.. needs a DB update.
Comment 8 Martin Renvoize 2021-03-17 12:27:07 UTC
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.
Comment 9 Martin Renvoize 2021-03-17 17:06:56 UTC
Add the following to the list:

'VOID' - For when we're reversing a payment
Comment 10 Martin Renvoize 2021-03-19 14:13:55 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2021-03-19 14:13:58 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2021-03-19 14:14:02 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize 2021-03-19 14:14:06 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize 2021-03-19 14:14:10 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize 2021-03-19 14:14:14 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize 2021-03-19 14:14:17 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2021-04-23 09:45:34 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2021-04-23 09:45:39 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize 2021-04-23 09:45:42 UTC Comment hidden (obsolete)
Comment 20 Martin Renvoize 2021-04-23 09:45:47 UTC
Created attachment 120043 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types
Comment 21 Martin Renvoize 2021-04-23 09:45:51 UTC Comment hidden (obsolete)
Comment 22 Martin Renvoize 2021-04-23 09:45:55 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2021-04-23 09:46:01 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2021-05-14 13:29:21 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2021-05-14 13:30:00 UTC Comment hidden (obsolete)
Comment 26 Martin Renvoize 2021-05-14 13:30:04 UTC Comment hidden (obsolete)
Comment 27 Martin Renvoize 2021-05-14 13:30:09 UTC Comment hidden (obsolete)
Comment 28 Martin Renvoize 2021-05-14 13:30:14 UTC Comment hidden (obsolete)
Comment 29 Martin Renvoize 2021-05-14 13:30:18 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2021-05-14 13:30:23 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2021-05-14 13:30:27 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2021-05-14 13:30:32 UTC Comment hidden (obsolete)
Comment 33 Martin Renvoize 2021-05-14 15:44:52 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2021-05-14 15:44:56 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2021-05-14 15:45:01 UTC Comment hidden (obsolete)
Comment 36 Martin Renvoize 2021-05-14 15:45:06 UTC Comment hidden (obsolete)
Comment 37 Martin Renvoize 2021-05-14 15:45:11 UTC Comment hidden (obsolete)
Comment 38 Martin Renvoize 2021-05-14 15:45:16 UTC Comment hidden (obsolete)
Comment 39 Martin Renvoize 2021-05-14 15:45:20 UTC Comment hidden (obsolete)
Comment 40 Martin Renvoize 2021-05-14 15:45:25 UTC Comment hidden (obsolete)
Comment 41 Martin Renvoize 2021-05-14 15:45:29 UTC Comment hidden (obsolete)
Comment 42 Martin Renvoize 2021-05-14 15:45:34 UTC Comment hidden (obsolete)
Comment 43 Martin Renvoize 2021-05-14 15:45:39 UTC Comment hidden (obsolete)
Comment 44 Martin Renvoize 2021-05-14 15:55:29 UTC Comment hidden (obsolete)
Comment 45 Martin Renvoize 2021-05-14 15:55:33 UTC Comment hidden (obsolete)
Comment 46 Martin Renvoize 2021-05-15 07:34:44 UTC Comment hidden (obsolete)
Comment 47 Martin Renvoize 2021-05-15 07:46:39 UTC Comment hidden (obsolete)
Comment 48 Martin Renvoize 2021-05-15 07:46:44 UTC Comment hidden (obsolete)
Comment 49 Martin Renvoize 2021-05-15 07:46:49 UTC Comment hidden (obsolete)
Comment 50 Martin Renvoize 2021-05-15 07:46:54 UTC Comment hidden (obsolete)
Comment 51 Martin Renvoize 2021-05-15 07:46:59 UTC Comment hidden (obsolete)
Comment 52 Martin Renvoize 2021-05-15 07:47:04 UTC Comment hidden (obsolete)
Comment 53 Martin Renvoize 2021-05-15 07:47:10 UTC Comment hidden (obsolete)
Comment 54 Martin Renvoize 2021-05-15 07:47:14 UTC
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.
Comment 55 Martin Renvoize 2021-05-15 07:47:19 UTC Comment hidden (obsolete)
Comment 56 Martin Renvoize 2021-05-15 07:47:23 UTC
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.
Comment 57 Martin Renvoize 2021-05-15 07:47:28 UTC Comment hidden (obsolete)
Comment 58 Martin Renvoize 2021-05-15 07:47:32 UTC
Created attachment 121016 [details] [review]
Bug 22435: Further refinements to account-details table
Comment 59 Martin Renvoize 2021-05-15 07:52:02 UTC
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.
Comment 60 Martin Renvoize 2021-05-17 07:18:06 UTC
Created attachment 121028 [details] [review]
Bug 22435: Alternative Three - Account Details

Another alternative to try, feedback welcome.
Comment 61 Victor Grousset/tuxayo 2021-06-06 19:02:17 UTC
> 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.
Comment 62 Victor Grousset/tuxayo 2021-06-08 18:56:23 UTC
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 ?
Comment 63 Victor Grousset/tuxayo 2021-06-08 19:08:22 UTC
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""
Comment 64 Martin Renvoize 2021-06-10 11:09:13 UTC
Created attachment 121757 [details] [review]
Bug 22435: Update CREATE offset types
Comment 65 Martin Renvoize 2021-06-10 11:09:18 UTC
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.
Comment 66 Martin Renvoize 2021-06-10 11:09:23 UTC
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.
Comment 67 Martin Renvoize 2021-06-10 11:09:27 UTC
Created attachment 121760 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types
Comment 68 Martin Renvoize 2021-06-10 11:09:32 UTC
Created attachment 121761 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type
Comment 69 Martin Renvoize 2021-06-10 11:09:36 UTC
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.
Comment 70 Martin Renvoize 2021-06-10 11:09:41 UTC
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.
Comment 71 Martin Renvoize 2021-06-10 11:09:46 UTC
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.
Comment 72 Martin Renvoize 2021-06-10 11:09:50 UTC
Created attachment 121765 [details] [review]
Bug 22435: Update for bug 27049
Comment 73 Martin Renvoize 2021-06-10 11:09:55 UTC
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.
Comment 74 Martin Renvoize 2021-06-10 11:09:59 UTC
Created attachment 121767 [details] [review]
Bug 22435: Improve database update
Comment 75 Martin Renvoize 2021-06-10 11:10:03 UTC
Created attachment 121768 [details] [review]
Bug 22435: Further refinements to account-details table
Comment 76 Martin Renvoize 2021-06-10 11:10:08 UTC
Created attachment 121769 [details] [review]
Bug 22435: Alternative Three - Account Details

Another alternative to try, feedback welcome.
Comment 77 Martin Renvoize 2021-06-10 11:15:26 UTC
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.
Comment 78 Victor Grousset/tuxayo 2021-06-15 07:32:57 UTC
Created attachment 121960 [details] [review]
Bug 22435: Update CREATE offset types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 79 Victor Grousset/tuxayo 2021-06-15 07:33:02 UTC
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>
Comment 80 Victor Grousset/tuxayo 2021-06-15 07:33:07 UTC
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>
Comment 81 Victor Grousset/tuxayo 2021-06-15 07:33:12 UTC
Created attachment 121963 [details] [review]
Bug 22435: Add 'APPLY' to account_offset_types

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 82 Victor Grousset/tuxayo 2021-06-15 07:33:16 UTC
Created attachment 121964 [details] [review]
Bug 22435: ->apply() should always use 'APPLY' for offset_type

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 83 Victor Grousset/tuxayo 2021-06-15 07:33:21 UTC
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>
Comment 84 Victor Grousset/tuxayo 2021-06-15 07:33:26 UTC
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>
Comment 85 Victor Grousset/tuxayo 2021-06-15 07:33:31 UTC
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>
Comment 86 Victor Grousset/tuxayo 2021-06-15 07:33:35 UTC
Created attachment 121968 [details] [review]
Bug 22435: Update for bug 27049

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 87 Victor Grousset/tuxayo 2021-06-15 07:33:40 UTC
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>
Comment 88 Victor Grousset/tuxayo 2021-06-15 07:33:45 UTC
Created attachment 121970 [details] [review]
Bug 22435: Improve database update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 89 Victor Grousset/tuxayo 2021-06-15 07:33:50 UTC
Created attachment 121971 [details] [review]
Bug 22435: Further refinements to account-details table

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 90 Victor Grousset/tuxayo 2021-06-15 07:33:54 UTC
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>
Comment 91 Victor Grousset/tuxayo 2021-06-15 07:36:47 UTC
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
Comment 93 Nick Clemens 2021-06-18 15:19:34 UTC
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>
Comment 94 Nick Clemens 2021-06-18 15:19:38 UTC
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>
Comment 95 Nick Clemens 2021-06-18 15:19:43 UTC
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>
Comment 96 Nick Clemens 2021-06-18 15:19:48 UTC
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>
Comment 97 Nick Clemens 2021-06-18 15:19:53 UTC
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>
Comment 98 Nick Clemens 2021-06-18 15:19:58 UTC
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>
Comment 99 Nick Clemens 2021-06-18 15:20:03 UTC
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>
Comment 100 Nick Clemens 2021-06-18 15:20:08 UTC
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>
Comment 101 Nick Clemens 2021-06-18 15:20:13 UTC
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>
Comment 102 Nick Clemens 2021-06-18 15:20:19 UTC
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>
Comment 103 Nick Clemens 2021-06-18 15:20:23 UTC
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>
Comment 104 Nick Clemens 2021-06-18 15:20:28 UTC
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>
Comment 105 Nick Clemens 2021-06-18 15:20:34 UTC
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>
Comment 106 Nick Clemens 2021-06-18 15:20:39 UTC
Created attachment 122161 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 107 Nick Clemens 2021-06-18 15:25:04 UTC
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!
Comment 108 Jonathan Druart 2021-06-21 09:47:23 UTC
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
Comment 109 Martin Renvoize 2021-06-21 15:59:59 UTC
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>
Comment 110 Martin Renvoize 2021-06-21 16:00:04 UTC
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>
Comment 111 Martin Renvoize 2021-06-21 16:00:09 UTC
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>
Comment 112 Martin Renvoize 2021-06-21 16:00:14 UTC
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>
Comment 113 Martin Renvoize 2021-06-21 16:00:19 UTC
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>
Comment 114 Martin Renvoize 2021-06-21 16:00:24 UTC
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>
Comment 115 Martin Renvoize 2021-06-21 16:00:29 UTC
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>
Comment 116 Martin Renvoize 2021-06-21 16:00:34 UTC
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>
Comment 117 Martin Renvoize 2021-06-21 16:00:39 UTC
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>
Comment 118 Martin Renvoize 2021-06-21 16:00:45 UTC
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>
Comment 119 Martin Renvoize 2021-06-21 16:00:50 UTC
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>
Comment 120 Martin Renvoize 2021-06-21 16:00:55 UTC
Created attachment 122246 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 121 Martin Renvoize 2021-06-21 16:01:01 UTC
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
Comment 122 Martin Renvoize 2021-06-21 16:01:06 UTC
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.
Comment 123 Martin Renvoize 2021-06-21 16:01:11 UTC
Created attachment 122249 [details] [review]
Bug 22435: (QA follow-up) Add missing test for manager relation
Comment 124 Martin Renvoize 2021-06-21 16:01:16 UTC
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.
Comment 125 Martin Renvoize 2021-06-21 16:01:21 UTC
Created attachment 122251 [details] [review]
Bug 22435: Update DBIC Schema's
Comment 126 Martin Renvoize 2021-06-21 16:13:02 UTC
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
Comment 127 Martin Renvoize 2021-06-21 16:23:28 UTC
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.
Comment 128 Martin Renvoize 2021-06-21 17:33:20 UTC
Created attachment 122253 [details] [review]
Bug 22435: (QA follow-up) More test updates
Comment 129 Martin Renvoize 2021-06-22 07:52:00 UTC
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>
Comment 130 Martin Renvoize 2021-06-22 07:52:05 UTC
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>
Comment 131 Martin Renvoize 2021-06-22 07:52:10 UTC
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>
Comment 132 Martin Renvoize 2021-06-22 07:52:15 UTC
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>
Comment 133 Martin Renvoize 2021-06-22 07:52:20 UTC
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>
Comment 134 Martin Renvoize 2021-06-22 07:52:25 UTC
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>
Comment 135 Martin Renvoize 2021-06-22 07:52:30 UTC
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>
Comment 136 Martin Renvoize 2021-06-22 07:52:35 UTC
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>
Comment 137 Martin Renvoize 2021-06-22 07:52:41 UTC
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>
Comment 138 Martin Renvoize 2021-06-22 07:52:46 UTC
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>
Comment 139 Martin Renvoize 2021-06-22 07:52:51 UTC
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>
Comment 140 Martin Renvoize 2021-06-22 07:52:56 UTC
Created attachment 122267 [details] [review]
Bug 22435: (QA follow-up) Fix test file and adjust test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 141 Martin Renvoize 2021-06-22 07:53:01 UTC
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
Comment 142 Martin Renvoize 2021-06-22 07:53:06 UTC
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.
Comment 143 Martin Renvoize 2021-06-22 07:53:11 UTC
Created attachment 122270 [details] [review]
Bug 22435: (QA follow-up) Add missing test for manager relation
Comment 144 Martin Renvoize 2021-06-22 07:53:16 UTC
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.
Comment 145 Martin Renvoize 2021-06-22 07:53:21 UTC
Created attachment 122272 [details] [review]
Bug 22435: Update DBIC Schema's
Comment 146 Martin Renvoize 2021-06-22 07:53:26 UTC
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.
Comment 147 Martin Renvoize 2021-06-22 07:53:32 UTC
Created attachment 122274 [details] [review]
Bug 22435: (QA follow-up) More test updates
Comment 148 Tomás Cohen Arazi 2021-07-02 15:30:58 UTC
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>
Comment 149 Tomás Cohen Arazi 2021-07-02 15:31:11 UTC
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>
Comment 150 Tomás Cohen Arazi 2021-07-02 15:31:21 UTC
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>
Comment 151 Tomás Cohen Arazi 2021-07-02 15:31:32 UTC
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>
Comment 152 Tomás Cohen Arazi 2021-07-02 15:31:44 UTC
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>
Comment 153 Tomás Cohen Arazi 2021-07-02 15:31:58 UTC
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>
Comment 154 Tomás Cohen Arazi 2021-07-02 15:32:08 UTC
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>
Comment 155 Tomás Cohen Arazi 2021-07-02 15:32:21 UTC
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>
Comment 156 Tomás Cohen Arazi 2021-07-02 15:32:32 UTC
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>
Comment 157 Tomás Cohen Arazi 2021-07-02 15:32:43 UTC
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>
Comment 158 Tomás Cohen Arazi 2021-07-02 15:32:55 UTC
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>
Comment 159 Tomás Cohen Arazi 2021-07-02 15:33:09 UTC
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>
Comment 160 Tomás Cohen Arazi 2021-07-02 15:33:18 UTC
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>
Comment 161 Tomás Cohen Arazi 2021-07-02 15:33:34 UTC
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>
Comment 162 Tomás Cohen Arazi 2021-07-02 15:33:41 UTC
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>
Comment 163 Tomás Cohen Arazi 2021-07-02 15:33:48 UTC
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>
Comment 164 Tomás Cohen Arazi 2021-07-02 15:33:56 UTC
Created attachment 122569 [details] [review]
Bug 22435: Update DBIC Schema's

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 165 Tomás Cohen Arazi 2021-07-02 15:34:05 UTC
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>
Comment 166 Tomás Cohen Arazi 2021-07-02 15:34:15 UTC
Created attachment 122571 [details] [review]
Bug 22435: (QA follow-up) More test updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 167 Tomás Cohen Arazi 2021-07-02 15:37:03 UTC
Very nice and welcome enhancement. This was really the missing piece in the cleanup. Simplifynig things everywhere.
Nice work on the transaction history BTW.
Comment 168 Martin Renvoize 2021-07-05 13:20:58 UTC
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.
Comment 169 Martin Renvoize 2021-07-16 15:24:38 UTC
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>
Comment 170 Martin Renvoize 2021-07-16 15:24:43 UTC
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>
Comment 171 Martin Renvoize 2021-07-16 15:24:49 UTC
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>
Comment 172 Martin Renvoize 2021-07-16 15:24:54 UTC
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>
Comment 173 Martin Renvoize 2021-07-16 15:24:59 UTC
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>
Comment 174 Martin Renvoize 2021-07-16 15:25:05 UTC
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>
Comment 175 Martin Renvoize 2021-07-16 15:25:11 UTC
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>
Comment 176 Martin Renvoize 2021-07-16 15:25:16 UTC
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>
Comment 177 Martin Renvoize 2021-07-16 15:25:21 UTC
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>
Comment 178 Martin Renvoize 2021-07-16 15:25:27 UTC
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>
Comment 179 Martin Renvoize 2021-07-16 15:25:33 UTC
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>
Comment 180 Martin Renvoize 2021-07-16 15:25:38 UTC
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>
Comment 181 Martin Renvoize 2021-07-16 15:25:44 UTC
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>
Comment 182 Martin Renvoize 2021-07-16 15:25:49 UTC
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>
Comment 183 Martin Renvoize 2021-07-16 15:25:55 UTC
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>
Comment 184 Martin Renvoize 2021-07-16 15:26:01 UTC
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>
Comment 185 Martin Renvoize 2021-07-16 15:26:06 UTC
Created attachment 122915 [details] [review]
Bug 22435: Update DBIC Schema's

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 186 Martin Renvoize 2021-07-16 15:26:12 UTC
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>
Comment 187 Martin Renvoize 2021-07-16 15:26:17 UTC
Created attachment 122917 [details] [review]
Bug 22435: (QA follow-up) More test updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 188 Martin Renvoize 2021-07-16 15:26:23 UTC
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>
Comment 189 Martin Renvoize 2021-07-16 15:26:56 UTC
Just a quick rebase ;)
Comment 190 Victor Grousset/tuxayo 2021-07-18 16:03:45 UTC
Does that invalidate the passed QA stamp?
Comment 191 Jonathan Druart 2021-07-19 09:33:37 UTC
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
Comment 192 Jonathan Druart 2021-07-19 09:37:55 UTC
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)" );
Comment 193 Martin Renvoize 2021-07-19 11:26:37 UTC
(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.
Comment 194 Jonathan Druart 2021-07-19 12:09:09 UTC
(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!
Comment 195 Jonathan Druart 2021-07-26 10:06:50 UTC
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.
Comment 196 Martin Renvoize 2021-08-03 14:33:00 UTC
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>
Comment 197 Martin Renvoize 2021-08-03 14:33:06 UTC
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>
Comment 198 Martin Renvoize 2021-08-03 14:33:12 UTC
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>
Comment 199 Martin Renvoize 2021-08-03 14:33:17 UTC
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>
Comment 200 Martin Renvoize 2021-08-03 14:33:23 UTC
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>
Comment 201 Martin Renvoize 2021-08-03 14:33:29 UTC
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>
Comment 202 Martin Renvoize 2021-08-03 14:33:35 UTC
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>
Comment 203 Martin Renvoize 2021-08-03 14:33:41 UTC
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>
Comment 204 Martin Renvoize 2021-08-03 14:33:47 UTC
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>
Comment 205 Martin Renvoize 2021-08-03 14:33:53 UTC
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>
Comment 206 Martin Renvoize 2021-08-03 14:33:58 UTC
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>
Comment 207 Martin Renvoize 2021-08-03 14:34:04 UTC
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>
Comment 208 Martin Renvoize 2021-08-03 14:34:10 UTC
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>
Comment 209 Martin Renvoize 2021-08-03 14:34:15 UTC
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>
Comment 210 Martin Renvoize 2021-08-03 14:34:21 UTC
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>
Comment 211 Martin Renvoize 2021-08-03 14:34:27 UTC
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>
Comment 212 Martin Renvoize 2021-08-03 14:34:33 UTC
Created attachment 123439 [details] [review]
Bug 22435: Update DBIC Schema's

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 213 Martin Renvoize 2021-08-03 14:34:39 UTC
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>
Comment 214 Martin Renvoize 2021-08-03 14:34:45 UTC
Created attachment 123441 [details] [review]
Bug 22435: (QA follow-up) More test updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 215 Martin Renvoize 2021-08-03 14:34:51 UTC
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>
Comment 216 Martin Renvoize 2021-08-03 14:39:33 UTC
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>
Comment 217 Martin Renvoize 2021-08-03 14:40:30 UTC
Yup, I neglected to add the fix for that test to my final commit the first time around!

All fixed now.. and rebased.
Comment 218 Jonathan Druart 2021-08-04 12:05:46 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 219 Martin Renvoize 2021-11-10 10:28:40 UTC
*** Bug 24382 has been marked as a duplicate of this bug. ***