Description
Martin Renvoize (ashimema)
2018-10-30 18:52:20 UTC
Created attachment 81879 [details] [review] Bug 21727: Add ->adjust to Koha::Account::Line Proof Of Concept... tests to come Created attachment 82777 [details] [review] Bug 21727: Add tests for Koha::Account::Line->adjust Created attachment 82778 [details] [review] Bug 21727: Add ->adjust to Koha::Account::Line When running test I got this: Can't locate object method "throw" via package "Koha::Exceptions::Account::UnrecognisedType" (perhaps you forgot to load "Koha::Exceptions::Account::UnrecognisedType"?) at /home/vagrant/kohaclone/Koha/Account/Line.pm line 231 Maybe you forgot one patch? Or is there a dependency bug report? (In reply to Josef Moravec from comment #5) > When running test I got this: > > Can't locate object method "throw" via package > "Koha::Exceptions::Account::UnrecognisedType" (perhaps you forgot to load > "Koha::Exceptions::Account::UnrecognisedType"?) at > /home/vagrant/kohaclone/Koha/Account/Line.pm line 231 > > Maybe you forgot one patch? Or is there a dependency bug report? This one depends on a couple exceptions that are added in bug 21002. Adding the dependency now. Thanks Tomas, that's it.. sorry I missed the dependency chain in the first place. Created attachment 83054 [details] [review] Bug 21727: Add tests for Koha::Account::Line->adjust Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 83055 [details] [review] Bug 21727: Add ->adjust to Koha::Account::Line This patch introduces a high-level method for updating existing debits, similar to Koha::Account::add_credit and Koha::Account::add_debit. It updates the Koha::Account::Line object, and adds an offset. It also logs if required. It whitelists the account types that are allowed to be adjusted, just in case some others require adjusting, having a single source for truth. To test: - Apply this patches - Run: $ kshell k$ prove t/db_dependent/Koha/Account/Lines.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 83056 [details] [review] Bug 21727: (QA follow-up) Test lastincrement changes 'adjust' changes the 'lastincrement column, and this behaviour should be tested. This patch just adds tests for that. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Awesome.. thanks for testing (and enhancing the test coverage) Tomas. For Josef's benefit I've also updated the dependancies.. splitting out the exception class additions into their own bug so we're not dependant on the larger and less related bug 21002 Also to note.. the only use case that exists so far for this method is the adjustment of the non-definitive 'FU' type accountline which is a 'still incrementing' charge. I think the majority of accounts should be handled by other methods adding credit/debit pairs and only 'non-definitive/in-progress' incrementing fines should utilise this method.. One such incrementing fine I could think of perhaps wishing to use this in the future that might make sense would be the introduction of a more granular lost item charging scheme that acted as a buffer to 'overdue' and would charge at a different rate whilst the user either found the item or the system decided it was well and truly lost. (in such a case I would expect an LU accountype to get paired with the existing L type). (In reply to Martin Renvoize from comment #12) > Also to note.. the only use case that exists so far for this method is the > adjustment of the non-definitive 'FU' type accountline which is a 'still > incrementing' charge. > > I think the majority of accounts should be handled by other methods adding > credit/debit pairs and only 'non-definitive/in-progress' incrementing fines > should utilise this method.. > > One such incrementing fine I could think of perhaps wishing to use this in > the future that might make sense would be the introduction of a more > granular lost item charging scheme that acted as a buffer to 'overdue' and > would charge at a different rate whilst the user either found the item or > the system decided it was well and truly lost. (in such a case I would > expect an LU accountype to get paired with the existing L type). I would add the fact that this fee is either a fixed about (by itemtype) or the one specified in the lost item itself. On the second case, that value can get easily outdated compared to real bookseller prices in a changing world, so having a way to adjust that value makes sense in a future. Created attachment 83058 [details] [review] Bug 21727: Add clarifications to the POD of adjust Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83088 [details] [review] Bug 21727: Add tests for Koha::Account::Line->adjust Sponsored-by: PTFS Europe Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 83089 [details] [review] Bug 21727: Add ->adjust to Koha::Account::Line This patch introduces a high-level method for updating existing debits, similar to Koha::Account::add_credit and Koha::Account::add_debit. It updates the Koha::Account::Line object, and adds an offset. It also logs if required. It whitelists the account types that are allowed to be adjusted, just in case some others require adjusting, having a single source for truth. To test: - Apply this patches - Run: $ kshell k$ prove t/db_dependent/Koha/Account/Lines.t => SUCCESS: Tests pass! Sponsored-by: PTFS Europe Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 83090 [details] [review] Bug 21727: (QA follow-up) Test lastincrement changes 'adjust' changes the 'lastincrement column, and this behaviour should be tested. This patch just adds tests for that. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83091 [details] [review] Bug 21727: Add clarifications to the POD of adjust Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 83095 [details] [review] Bug 21727: Add tests for Koha::Account::Line->adjust Sponsored-by: PTFS Europe Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 83096 [details] [review] Bug 21727: Add ->adjust to Koha::Account::Line This patch introduces a high-level method for updating existing debits, similar to Koha::Account::add_credit and Koha::Account::add_debit. It updates the Koha::Account::Line object, and adds an offset. It also logs if required. It whitelists the account types that are allowed to be adjusted, just in case some others require adjusting, having a single source for truth. To test: - Apply this patches - Run: $ kshell k$ prove t/db_dependent/Koha/Account/Lines.t => SUCCESS: Tests pass! Sponsored-by: PTFS Europe Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 83097 [details] [review] Bug 21727: (QA follow-up) Test lastincrement changes 'adjust' changes the 'lastincrement column, and this behaviour should be tested. This patch just adds tests for that. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 83098 [details] [review] Bug 21727: Add clarifications to the POD of adjust Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 83101 [details] [review] Bug 21727: Add handling for cases requireing patron refunds As it is possible to partially pay off a fine that is still incrementing we can end up in a position (when dropbox mode is enabled) where the patron may be owed money by the library should they have paid off the fine between dropping the item off in dropbox mode and the increment that reverts the increase on the dropbox day. Marking as Failed QA.. Tomas has identified a case where the adjust may lead to a situation where the library owes the patron a refund and we agree we should never allow the 'amountoutstanding' to become negative. As such, I have produced a further patch to handle this case, adding a credit to the patrons account, but I still need to add a test for this case to the tests. Created attachment 83117 [details] [review] Bug 21727: Add handling for cases requireing patron refunds As it is possible to partially pay off a fine that is still incrementing we can end up in a position (when dropbox mode is enabled) where the patron may be owed money by the library should they have paid off the fine between dropping the item off in dropbox mode and the increment that reverts the increase on the dropbox day. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 83118 [details] [review] Bug 21727: Unit tests for the refund case This patch introduces tests for the behaviour when the negative adjust exceedes the amount that was already paid. In this case, the line amount is expected to be set to 0, and a credit with the payed amount created. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Account/Lines.t => SUCCESS: tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 83121 [details] [review] Bug 21727: Unit tests for the refund case This patch introduces tests for the behaviour when the negative adjust exceedes the amount that was already paid. In this case, the line amount is expected to be set to 0, and a credit with the payed amount created. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Account/Lines.t => SUCCESS: tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 83122 [details] [review] Bug 21727: Add handling for cases requireing patron refunds As it is possible to partially pay off a fine that is still incrementing we can end up in a position (when dropbox mode is enabled) where the patron may be owed money by the library should they have paid off the fine between dropping the item off in dropbox mode and the increment that reverts the increase on the dropbox day. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Great catch Awesome work all! Pushed to master for 19.05 Created attachment 83818 [details] [review] Bug 21727: (QA follow-up) Fix POD encoding (In reply to Tomás Cohen Arazi from comment #31) > Created attachment 83818 [details] [review] [review] > Bug 21727: (QA follow-up) Fix POD encoding Follow-up pushed to master for 19.05 Enhancement, will not be backported to 18.11.x series. *** Bug 24431 has been marked as a duplicate of this bug. *** |