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.
Created attachment 76299 [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.
Created attachment 76300 [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
Created attachment 76301 [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
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.
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
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
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.
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)
(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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Awesome work all! Pushed to master for 18.11
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.
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.