|
Description
Martin Renvoize (ashimema)
2019-03-22 14:36:40 UTC
Created attachment 86913 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86914 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87092 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87093 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87094 [details] [review] Bug 22563: (follow-up) Corrections for tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87637 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87638 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87639 [details] [review] Bug 22563: (follow-up) Corrections for tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87640 [details] [review] Bug 22563: Clean up remaing LR cases Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87641 [details] [review] Bug 22563: (follow-up) Fix Lost in NoIssuesChargeGuarantees test Created attachment 87642 [details] [review] Bug 22563: Clean up remaining L cases Freshly re-based and ready for SO Created attachment 87801 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87802 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87803 [details] [review] Bug 22563: (follow-up) Corrections for tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87804 [details] [review] Bug 22563: Clean up remaing LR cases Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 87805 [details] [review] Bug 22563: (follow-up) Fix Lost in NoIssuesChargeGuarantees test Created attachment 87806 [details] [review] Bug 22563: Clean up remaining L cases Freshly rebased Martin, how can I test your patch? Please, gime us some test plan. Thank you. Resetting to assigned as I've spotted a followup I need to make. It would seem that accountlines wise we have always marked overdues for lost items as 'returned'. I think this is misleading and should be replaced with an 'OVERDUE' + 'LOST' combination to show the fines final state as LOST rather than RETURNED. I don't believe there's any way to retrospectively apply this to prior records unfortunately. Followup will come shortly (along with a full test plan.. appologies for not including one before now) Created attachment 88279 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88280 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88281 [details] [review] Bug 22563: (follow-up) Corrections for tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88282 [details] [review] Bug 22563: Clean up remaing LR cases Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88283 [details] [review] Bug 22563: (follow-up) Fix Lost in NoIssuesChargeGuarantees test Created attachment 88284 [details] [review] Bug 22563: Clean up remaining L cases Created attachment 88285 [details] [review] Bug 22563: (follow-up) Items LOST should not be RETURNED Up until now we marked lost items as returned in the accountlines, now we have the oportunity to mark these distinctly with an appropriate status we should. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Rebased and followup added. Test Plan 1) Find an overdue with fines 2) Ensure you are not forgiving fines when an item is marked as lost 3) Mark the item as lost 4) Confirm the Fine is given a status of 'Lost' Back to assigned.. with all the foundations work in patches further up the tree I've lost track somewhat within this one.. Need to go back and come up with a thorough test plan again Created attachment 88323 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88324 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88325 [details] [review] Bug 22563: (follow-up) Corrections for tests Updates to unit tests to reflect the accounttype changes and introduction of status usage. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88326 [details] [review] Bug 22563: (follow-up) Items LOST should not be RETURNED Up until now we marked lost items as returned in the accountlines, now we have the oportunity to mark these distinctly with an appropriate status we should. Test Plan 1) Find an overdue with fines 2) Ensure you are not forgiving fines when an item is marked as lost 3) Mark the item as lost 4) Confirm the Fine is given a status of 'Lost' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88327 [details] [review] Bug 22563: (follow-up) Corrections for overdue tests Updates to unit tests to reflect the accounttype changes and introduction of status usage for _FixOverduesOnReturn. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88328 [details] [review] Bug 22563: (follow-up) Clean up remaing L, LR and CR cases in the database Test plan: 1) Confirm that after the DB update there are no L, LR or CR accountlines present. 2) Read the db update and confirm is makes sense. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Test plan: This patch doesn't strictly change any functionality but rather cleans up the codepaths. The corresponding unit tests have been carefully updated to more thoroughly test the functionality and reflect the updated field values. Functionality is all around the Lost Item functionalities. So, using the different ways of marking an item as lost you will want to check the results of charges given the various combinations of charging for lost items and overdue forgiveness preferences. You will also want to check the cash_register_stats report to ensure it still accurately reflects the above combinations. Created attachment 88720 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88721 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88722 [details] [review] Bug 22563: (follow-up) Corrections for tests Updates to unit tests to reflect the accounttype changes and introduction of status usage. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88723 [details] [review] Bug 22563: (follow-up) Items LOST should not be RETURNED Up until now we marked lost items as returned in the accountlines, now we have the oportunity to mark these distinctly with an appropriate status we should. Test Plan 1) Find an overdue with fines 2) Ensure you are not forgiving fines when an item is marked as lost 3) Mark the item as lost 4) Confirm the Fine is given a status of 'Lost' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88724 [details] [review] Bug 22563: (follow-up) Corrections for overdue tests Updates to unit tests to reflect the accounttype changes and introduction of status usage for _FixOverduesOnReturn. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 88725 [details] [review] Bug 22563: (follow-up) Clean up remaing L, LR and CR cases in the database Test plan: 1) Confirm that after the DB update there are no L, LR or CR accountlines present. 2) Read the db update and confirm is makes sense. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Ready for SO again Created attachment 89462 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89463 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89464 [details] [review] Bug 22563: (follow-up) Corrections for tests Updates to unit tests to reflect the accounttype changes and introduction of status usage. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89465 [details] [review] Bug 22563: (follow-up) Items LOST should not be RETURNED Up until now we marked lost items as returned in the accountlines, now we have the oportunity to mark these distinctly with an appropriate status we should. Test Plan 1) Find an overdue with fines 2) Ensure you are not forgiving fines when an item is marked as lost 3) Mark the item as lost 4) Confirm the Fine is given a status of 'Lost' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89466 [details] [review] Bug 22563: (follow-up) Corrections for overdue tests Updates to unit tests to reflect the accounttype changes and introduction of status usage for _FixOverduesOnReturn. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89467 [details] [review] Bug 22563: (follow-up) Clean up remaing L, LR and CR cases in the database Test plan: 1) Confirm that after the DB update there are no L, LR or CR accountlines present. 2) Read the db update and confirm is makes sense. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 90221 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90222 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90223 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90224 [details] [review] Bug 22563: (follow-up) Corrections for tests Updates to unit tests to reflect the accounttype changes and introduction of status usage. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90225 [details] [review] Bug 22563: (follow-up) Items LOST should not be RETURNED Up until now we marked lost items as returned in the accountlines, now we have the oportunity to mark these distinctly with an appropriate status we should. Test Plan 1) Find an overdue with fines 2) Ensure you are not forgiving fines when an item is marked as lost 3) Mark the item as lost 4) Confirm the Fine is given a status of 'Lost' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90226 [details] [review] Bug 22563: (follow-up) Corrections for overdue tests Updates to unit tests to reflect the accounttype changes and introduction of status usage for _FixOverduesOnReturn. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90227 [details] [review] Bug 22563: (follow-up) Clean up remaing L, LR and CR cases in the database Test plan: 1) Confirm that after the DB update there are no L, LR or CR accountlines present. 2) Read the db update and confirm is makes sense. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90307 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90308 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90309 [details] [review] Bug 22563: (follow-up) Corrections for tests Updates to unit tests to reflect the accounttype changes and introduction of status usage. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90310 [details] [review] Bug 22563: (follow-up) Items LOST should not be RETURNED Up until now we marked lost items as returned in the accountlines, now we have the oportunity to mark these distinctly with an appropriate status we should. Test Plan 1) Find an overdue with fines 2) Ensure you are not forgiving fines when an item is marked as lost 3) Mark the item as lost 4) Confirm the Fine is given a status of 'Lost' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90311 [details] [review] Bug 22563: (follow-up) Corrections for overdue tests Updates to unit tests to reflect the accounttype changes and introduction of status usage for _FixOverduesOnReturn. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90312 [details] [review] Bug 22563: (follow-up) Clean up remaing L, LR and CR cases in the database Test plan: 1) Confirm that after the DB update there are no L, LR or CR accountlines present. 2) Read the db update and confirm is makes sense. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90330 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90331 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90332 [details] [review] Bug 22563: (follow-up) Corrections for tests Updates to unit tests to reflect the accounttype changes and introduction of status usage. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90333 [details] [review] Bug 22563: (follow-up) Items LOST should not be RETURNED Up until now we marked lost items as returned in the accountlines, now we have the oportunity to mark these distinctly with an appropriate status we should. Test Plan 1) Find an overdue with fines 2) Ensure you are not forgiving fines when an item is marked as lost 3) Mark the item as lost 4) Confirm the Fine is given a status of 'Lost' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90334 [details] [review] Bug 22563: (follow-up) Corrections for overdue tests Updates to unit tests to reflect the accounttype changes and introduction of status usage for _FixOverduesOnReturn. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90335 [details] [review] Bug 22563: (follow-up) Clean up remaing L, LR and CR cases in the database Test plan: 1) Confirm that after the DB update there are no L, LR or CR accountlines present. 2) Read the db update and confirm is makes sense. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90632 [details] [review] Bug 22563: (QA follow-up) Use issue_id in chargelostitem C4::Accounts::chargelostitem contained a FIXME which asked if an item should be charged for it lost, returned and then lost again. We add handling for that case here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 90637 [details] [review] Bug 22563: (QA follow-up) Catch LOST maninvoice cases One could add a manual invoice of type LOST but we would not associate it with an issue_id. This patch tries to catch such cases and extrapolate an issue_id to add to the accountline. Created attachment 90667 [details] [review] Bug 22563: (QA follow-up) Correction to DB update Created attachment 91338 [details] [review] Bug 22563: Be more descriptive with accountypes Convert 'L' to 'LOST' to become more descriptive in it's use. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 91339 [details] [review] Bug 22563: Update lost handling to use accountline.status Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91340 [details] [review] Bug 22563: (follow-up) Corrections for tests Updates to unit tests to reflect the accounttype changes and introduction of status usage. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91341 [details] [review] Bug 22563: (follow-up) Items LOST should not be RETURNED Up until now we marked lost items as returned in the accountlines, now we have the oportunity to mark these distinctly with an appropriate status we should. Test Plan 1) Find an overdue with fines 2) Ensure you are not forgiving fines when an item is marked as lost 3) Mark the item as lost 4) Confirm the Fine is given a status of 'Lost' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91342 [details] [review] Bug 22563: (follow-up) Corrections for overdue tests Updates to unit tests to reflect the accounttype changes and introduction of status usage for _FixOverduesOnReturn. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91343 [details] [review] Bug 22563: (follow-up) Clean up remaing L, LR and CR cases in the database Test plan: 1) Confirm that after the DB update there are no L, LR or CR accountlines present. 2) Read the db update and confirm is makes sense. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91344 [details] [review] Bug 22563: (QA follow-up) Use issue_id in chargelostitem C4::Accounts::chargelostitem contained a FIXME which asked if an item should be charged for it lost, returned and then lost again. We add handling for that case here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 91345 [details] [review] Bug 22563: (QA follow-up) Catch LOST maninvoice cases One could add a manual invoice of type LOST but we would not associate it with an issue_id. This patch tries to catch such cases and extrapolate an issue_id to add to the accountline. Created attachment 91346 [details] [review] Bug 22563: (QA follow-up) Correction to DB update Created attachment 91347 [details] [review] Bug 22563: Typo in query (manualinvoice) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 91348 [details] [review] Bug 22563: (follow-up) Fix tests in Koha/Account.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Rebased on top of master, and added minor follow-ups for the failing tests. All of them because of the L -> LOST change. Solid work, Martin. Congrats! Nice work! Pushed to master for 19.11.00 |