Bug 20980

Summary: Manual credit offsets are stored as debits
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: josef.moravec, katrin.fischer, kyle, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This change may affect existing reports. Credits will no longer be recorded as 'debits' but rather get their own 'Manual Credit' type.
Version(s) released in:
Bug Depends on: 14826, 20978    
Bug Blocks: 20990, 21702    
Attachments: Bug 20980: Add 'Manual Credit' and fix existing offsets
Bug 20980: Make mancredit.pl use Koha::Account::add_credit
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'
Bug 20980: Add 'Manual Credit' and fix existing offsets
Bug 20980: Make mancredit.pl use Koha::Account::add_credit
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'
Bug 20980: (follow-up) Offset name consistency
Bug 20980: Add 'Manual Credit' and fix existing offsets
Bug 20980: Make mancredit.pl use Koha::Account::add_credit
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'
Bug 20980: (follow-up) Offset name consistency
Bug 20980: Add 'Manual Credit' and fix existing offsets
Bug 20980: Make mancredit.pl use Koha::Account::add_credit
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'
Bug 20980: (follow-up) Offset name consistency
Bug 20980: Add 'Manual Credit' and fix existing offsets
Bug 20980: Make mancredit.pl use Koha::Account::add_credit
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'
Bug 20980: (follow-up) Offset name consistency
Bug 20980: (QA follow-up) Do not delete existing data on tests fix
Bug 20980: (QA follow-up) Do not delete existing data on tests fix

Description Tomás Cohen Arazi 2018-06-21 17:28:02 UTC
mancredit.pl calls C4::Accounts::manualinvoice, which records debits. This leads to:

- account_offsets.type = 'Manual Debit'
- account_offsets.credit_id = NULL
- account_offsets.debit_id = accountlines_id

So we need to:
- Add 'Manual Credit' offset type
- Make sure accountlines_id is stored in credit_id column instead of debit_id
- Fix existing offsets.
Comment 1 Tomás Cohen Arazi 2018-06-22 15:57:14 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2018-06-22 15:57:21 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2018-06-22 15:57:27 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2018-06-23 10:49:43 UTC
Created attachment 76322 [details] [review]
Bug 20980: Add 'Manual Credit' and fix existing offsets

This patch adds a new offset type 'Manual Credit'. And adds an atomic
update for fixing existing offsets.
Comment 5 Tomás Cohen Arazi 2018-06-23 10:49:50 UTC
Created attachment 76323 [details] [review]
Bug 20980: Make mancredit.pl use Koha::Account::add_credit

This patch makes creating a manual credit from the UI record the account
offset as 'Manual Credit', and properly set account_offsets.credit_id
instead of account_offsets.debit_id.

To test:
- Create a manual credit (of 'Credit' type) for a known patron (acevedo?)
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> FAIL: The account offset for the manual credit has type=Manual Debit,
    credit_id=NULL and debit_id=accountlines_id
- Run the atomic update:
  $ updatedatabase
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> SUCCESS: The account offset has been corrected and now has
type=Manual Credit, credit_id=accountlines_id and debit_id=NULL
- Create a new manual credit (of 'Forgiven' type) for a known patron
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> SUCCESS: The account offset has been stored correctly as a credit!
- Sign off :-D
Comment 6 Tomás Cohen Arazi 2018-06-23 10:49:56 UTC
Created attachment 76324 [details] [review]
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'

This patch makes Koha::Account::add_credit record statistics logs
(through C4::Stats::UpdateStats) only for the 'payment' and 'writeoff'
credit types.

Both credit types are whitelisted in UpdateStats, so we keep the current
behaviour. Another option would've been to add new account types to the
white list, but with the account offsets it seems that using the
statistics table for account changes won't be useful for too long, as
offsets contain the same (and more) information.

To test:
- Apply the patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 7 Tomás Cohen Arazi 2018-06-23 10:50:02 UTC
Created attachment 76325 [details] [review]
Bug 20980: (follow-up) Offset name consistency

This patch makes 'credit' account types generate 'Manual Credit' offset.
The original patch used 'Payment' but I think the code wants to
distinguish between payments and manually added credits (otherwise they
would've picked the same accounttype value, and they don't.
Comment 8 Katrin Fischer 2018-06-23 11:48:41 UTC
Just pointing out... we do have rules about capitalization:
Manuel Credit should be Manuel credit (and possibly should actually be a code... but that's another story)
Comment 9 Tomás Cohen Arazi 2018-06-25 17:27:38 UTC
(In reply to Katrin Fischer from comment #8)
> Just pointing out... we do have rules about capitalization:
> Manuel Credit should be Manuel credit (and possibly should actually be a
> code... but that's another story)

I agree with you. I'm willing to file a new bug and provide patches to fix this if this patchset moves.
Comment 10 Josef Moravec 2018-06-25 18:50:58 UTC
Created attachment 76401 [details] [review]
Bug 20980: Add 'Manual Credit' and fix existing offsets

This patch adds a new offset type 'Manual Credit'. And adds an atomic
update for fixing existing offsets.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Josef Moravec 2018-06-25 18:51:03 UTC
Created attachment 76402 [details] [review]
Bug 20980: Make mancredit.pl use Koha::Account::add_credit

This patch makes creating a manual credit from the UI record the account
offset as 'Manual Credit', and properly set account_offsets.credit_id
instead of account_offsets.debit_id.

To test:
- Create a manual credit (of 'Credit' type) for a known patron (acevedo?)
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> FAIL: The account offset for the manual credit has type=Manual Debit,
    credit_id=NULL and debit_id=accountlines_id
- Run the atomic update:
  $ updatedatabase
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> SUCCESS: The account offset has been corrected and now has
type=Manual Credit, credit_id=accountlines_id and debit_id=NULL
- Create a new manual credit (of 'Forgiven' type) for a known patron
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> SUCCESS: The account offset has been stored correctly as a credit!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2018-06-25 18:51:09 UTC
Created attachment 76403 [details] [review]
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'

This patch makes Koha::Account::add_credit record statistics logs
(through C4::Stats::UpdateStats) only for the 'payment' and 'writeoff'
credit types.

Both credit types are whitelisted in UpdateStats, so we keep the current
behaviour. Another option would've been to add new account types to the
white list, but with the account offsets it seems that using the
statistics table for account changes won't be useful for too long, as
offsets contain the same (and more) information.

To test:
- Apply the patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2018-06-25 18:51:14 UTC
Created attachment 76404 [details] [review]
Bug 20980: (follow-up) Offset name consistency

This patch makes 'credit' account types generate 'Manual Credit' offset.
The original patch used 'Payment' but I think the code wants to
distinguish between payments and manually added credits (otherwise they
would've picked the same accounttype value, and they don't.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Kyle M Hall 2018-06-26 14:23:02 UTC
Created attachment 76442 [details] [review]
Bug 20980: Add 'Manual Credit' and fix existing offsets

This patch adds a new offset type 'Manual Credit'. And adds an atomic
update for fixing existing offsets.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2018-06-26 14:23:11 UTC
Created attachment 76443 [details] [review]
Bug 20980: Make mancredit.pl use Koha::Account::add_credit

This patch makes creating a manual credit from the UI record the account
offset as 'Manual Credit', and properly set account_offsets.credit_id
instead of account_offsets.debit_id.

To test:
- Create a manual credit (of 'Credit' type) for a known patron (acevedo?)
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> FAIL: The account offset for the manual credit has type=Manual Debit,
    credit_id=NULL and debit_id=accountlines_id
- Run the atomic update:
  $ updatedatabase
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> SUCCESS: The account offset has been corrected and now has
type=Manual Credit, credit_id=accountlines_id and debit_id=NULL
- Create a new manual credit (of 'Forgiven' type) for a known patron
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> SUCCESS: The account offset has been stored correctly as a credit!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2018-06-26 14:23:15 UTC
Created attachment 76444 [details] [review]
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'

This patch makes Koha::Account::add_credit record statistics logs
(through C4::Stats::UpdateStats) only for the 'payment' and 'writeoff'
credit types.

Both credit types are whitelisted in UpdateStats, so we keep the current
behaviour. Another option would've been to add new account types to the
white list, but with the account offsets it seems that using the
statistics table for account changes won't be useful for too long, as
offsets contain the same (and more) information.

To test:
- Apply the patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2018-06-26 14:23:18 UTC
Created attachment 76445 [details] [review]
Bug 20980: (follow-up) Offset name consistency

This patch makes 'credit' account types generate 'Manual Credit' offset.
The original patch used 'Payment' but I think the code wants to
distinguish between payments and manually added credits (otherwise they
would've picked the same accounttype value, and they don't.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Tomás Cohen Arazi 2018-07-06 12:22:41 UTC
Created attachment 76728 [details] [review]
Bug 20980: Add 'Manual Credit' and fix existing offsets

This patch adds a new offset type 'Manual Credit'. And adds an atomic
update for fixing existing offsets.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Tomás Cohen Arazi 2018-07-06 12:22:48 UTC
Created attachment 76729 [details] [review]
Bug 20980: Make mancredit.pl use Koha::Account::add_credit

This patch makes creating a manual credit from the UI record the account
offset as 'Manual Credit', and properly set account_offsets.credit_id
instead of account_offsets.debit_id.

To test:
- Create a manual credit (of 'Credit' type) for a known patron (acevedo?)
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> FAIL: The account offset for the manual credit has type=Manual Debit,
    credit_id=NULL and debit_id=accountlines_id
- Run the atomic update:
  $ updatedatabase
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> SUCCESS: The account offset has been corrected and now has
type=Manual Credit, credit_id=accountlines_id and debit_id=NULL
- Create a new manual credit (of 'Forgiven' type) for a known patron
- Run:
  $ sudo koha-mysql kohadev
  > SELECT * FROM account_offsets;
=> SUCCESS: The account offset has been stored correctly as a credit!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Tomás Cohen Arazi 2018-07-06 12:22:54 UTC
Created attachment 76730 [details] [review]
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff'

This patch makes Koha::Account::add_credit record statistics logs
(through C4::Stats::UpdateStats) only for the 'payment' and 'writeoff'
credit types.

Both credit types are whitelisted in UpdateStats, so we keep the current
behaviour. Another option would've been to add new account types to the
white list, but with the account offsets it seems that using the
statistics table for account changes won't be useful for too long, as
offsets contain the same (and more) information.

To test:
- Apply the patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Tomás Cohen Arazi 2018-07-06 12:23:02 UTC
Created attachment 76731 [details] [review]
Bug 20980: (follow-up) Offset name consistency

This patch makes 'credit' account types generate 'Manual Credit' offset.
The original patch used 'Payment' but I think the code wants to
distinguish between payments and manually added credits (otherwise they
would've picked the same accounttype value, and they don't.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Tomás Cohen Arazi 2018-07-06 12:23:10 UTC
Created attachment 76732 [details] [review]
Bug 20980: (QA follow-up) Do not delete existing data on tests fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2018-07-06 12:41:55 UTC
Created attachment 76734 [details] [review]
Bug 20980: (QA follow-up) Do not delete existing data on tests fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Nick Clemens 2018-07-06 14:46:28 UTC
Awesome work all!

Pushed to master for 18.11
Comment 25 Martin Renvoize 2018-07-10 08:19:01 UTC
I've decided not to backport this one to the 18.05.x series.

Although it's a nasty bug, it's been around a long time and I feel it appropriate to wait for 18.11 release, giving people time to prepare for the resulting internal api changes this patch, and it's dependencies, bring.
Comment 26 Martin Renvoize 2018-08-02 17:09:19 UTC
Reconsideration of that last comment, I've now Pushed to 18.05.x branch for 18.05.03.

This was done to unblock the chain of dependencies for bug 20942 which is a helpful enhancement encouraging third-party API integrations.