Bug 20980 - Manual credit offsets are stored as debits
Summary: Manual credit offsets are stored as debits
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 14826 20978
Blocks: 20990
  Show dependency treegraph
 
Reported: 2018-06-21 17:28 UTC by Tomás Cohen Arazi
Modified: 2018-08-02 17:10 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
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.


Attachments
Bug 20980: Add 'Manual Credit' and fix existing offsets (2.45 KB, patch)
2018-06-22 15:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: Make mancredit.pl use Koha::Account::add_credit (4.20 KB, patch)
2018-06-22 15:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff' (2.79 KB, patch)
2018-06-22 15:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: Add 'Manual Credit' and fix existing offsets (3.05 KB, patch)
2018-06-23 10:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: Make mancredit.pl use Koha::Account::add_credit (4.20 KB, patch)
2018-06-23 10:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff' (2.80 KB, patch)
2018-06-23 10:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: (follow-up) Offset name consistency (983 bytes, patch)
2018-06-23 10:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: Add 'Manual Credit' and fix existing offsets (3.10 KB, patch)
2018-06-25 18:50 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20980: Make mancredit.pl use Koha::Account::add_credit (4.25 KB, patch)
2018-06-25 18:51 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff' (2.84 KB, patch)
2018-06-25 18:51 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20980: (follow-up) Offset name consistency (1.01 KB, patch)
2018-06-25 18:51 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20980: Add 'Manual Credit' and fix existing offsets (3.20 KB, patch)
2018-06-26 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20980: Make mancredit.pl use Koha::Account::add_credit (4.34 KB, patch)
2018-06-26 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff' (2.92 KB, patch)
2018-06-26 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20980: (follow-up) Offset name consistency (1.09 KB, patch)
2018-06-26 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20980: Add 'Manual Credit' and fix existing offsets (3.16 KB, patch)
2018-07-06 12:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: Make mancredit.pl use Koha::Account::add_credit (4.31 KB, patch)
2018-07-06 12:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: (follow-up) Only use UpdateStats for 'payment' and 'writeoff' (2.42 KB, patch)
2018-07-06 12:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: (follow-up) Offset name consistency (1.07 KB, patch)
2018-07-06 12:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: (QA follow-up) Do not delete existing data on tests fix (1.08 KB, patch)
2018-07-06 12:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20980: (QA follow-up) Do not delete existing data on tests fix (1.27 KB, patch)
2018-07-06 12:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.