Created attachment 79272 [details] [review] Bug 21401: Add column transaction_library to table account_offsets
Created attachment 79273 [details] [review] Bug 21401: Update schema
Created attachment 79274 [details] [review] Bug 21401: Add tests for transaction_library
Created attachment 79275 [details] [review] Bug 21401: Take transaction library from userenv when not defined Test plan: 1) Apply the patch 2) Run database update 3) prove t/db_dependent/Account.t
Created attachment 79284 [details] [review] Bug 21401: Take transaction library from userenv when not defined Test plan: 1) Apply the patch 2) Run database update 3) prove t/db_dependent/Account.t Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79285 [details] [review] Bug 21401: Add column transaction_library to table account_offsets Signed-off-by: Michal Denare <black23@gmail.com>
Created attachment 79286 [details] [review] Bug 21401: Update schema Signed-off-by: Your Full Name <your_email> Signed-off-by: Your Full Name <your_email>
Created attachment 79287 [details] [review] Bug 21401: Add tests for transaction_library Signed-off-by: Your Full Name <your_email> Signed-off-by: Your Full Name <your_email>
Created attachment 79288 [details] [review] Bug 21401: Take transaction library from userenv when not defined Test plan: 1) Apply the patch 2) Run database update 3) prove t/db_dependent/Account.t Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Your Full Name <your_email>
- Use cases? - Maybe we should use ON DELETE SET NULL instead of cascade?
(In reply to Jonathan Druart from comment #10) > - Use cases? Statistics - we need reports for payment, classified by branch and type of fee, account_offsets table do the needed linking between fine and its payment, we just need to add a branch information > - Maybe we should use ON DELETE SET NULL instead of cascade? Good point, I'll fix it.
Created attachment 79688 [details] [review] Bug 21401: (follow-up) on update set null instead of cascade on transaction library
A few points. 1) This information is actually already stored in the statistics table (though I'll happily admit that's a pain and not especially useful for querying against) 2) It feels confusing to have this at the account_offsets level (which often has both debit_id and credit_id), it feels like it would be better placed in accountlines to me.
also, as much as I personally don't like it.. you'd be best to conform the the guidelines for your new field name: https://wiki.koha-community.org/wiki/Koha_Objects#Database_Table_Conventions - Every foreign key should have the same name as the primary key it links to when possible ( exceptions would be multiple FKs to the same table, and where a descriptor is better e.g. widgets.borrowernumber = widgets.managing_borrowernumber )
(In reply to Martin Renvoize from comment #13) > A few points. > > 1) This information is actually already stored in the statistics table > (though I'll happily admit that's a pain and not especially useful for > querying against) That's the reason I wanted to add the branch to accounts > 2) It feels confusing to have this at the account_offsets level (which often > has both debit_id and credit_id), it feels like it would be better placed in > accountlines to me. You are right, I'll change this to accountlines, it is not clear if the library is for credit or debit and it could be different library...
(In reply to Martin Renvoize from comment #14) > also, as much as I personally don't like it.. you'd be best to conform the > the guidelines for your new field name: > > https://wiki.koha-community.org/wiki/Koha_Objects#Database_Table_Conventions > - Every foreign key should have the same name as the primary key it links to > when possible ( exceptions would be multiple FKs to the same table, and > where a descriptor is better e.g. widgets.borrowernumber = > widgets.managing_borrowernumber ) True, I'll do it this way. Thanks for comments!
Created attachment 80355 [details] [review] Bug 21401: Add column transaction_library to table account_offsets
Created attachment 80356 [details] [review] Bug 21401: DO NOT PUSH - schema changes
Created attachment 80357 [details] [review] Bug 21401: Add tests for transaction_library
Created attachment 80358 [details] [review] Bug 21401: Take transaction library from userenv when not defined Test plan: 1) Apply the patch 2) Run database update 3) prove t/db_dependent/Account.t
Sorry Josef, I think this also needs some minor updates to Koha::Account.. in particular near the bottom of the pay method, but I've not delved further yet.
I look at Koha::Account, and it does make sense to add it do log, do you have anything else in mind?
I think we should leave the FinesLog stuff in (for backwards compatibility - perhaps we can create a deprecation plan for removing this in the future). Both the 'pay' and 'add_credit' methods need minor adjustments to account for the new branchcode recording. These are the main routines that should, in theory, be used throughout Koha to work with the accountlines and account_offset tables for payments and credits. (though I'd leave in your adaptation to 'new' in accountlines as it's a good catchall too). Have you considered the debits case at all? With you're overriden 'new' method in accountlines you'll be recording branchcodes with a fallback to userenv, but I have not yet walked the codepath backwards to check that fine creation etc would ever actually pass through a branchcode (or whether userenv will be set and make sense). All worth having a quick look at. Please don't be put off.. I do like this enhancement :)
Oooh.. also.. we'de need library_id adding to the existing api endpoints (especially aiming for this release as the endpoint will 'go live' with 18.11 for the first time)
Same as bug 19066.
I will rework this on top of 19066 - would like to add foreign key - api changes
Was there any further work you wanted to undertake here Josef.. or did it all end up folded into bug 19066?
(In reply to Martin Renvoize from comment #27) > Was there any further work you wanted to undertake here Josef.. or did it > all end up folded into bug 19066? For me, it is covered by bug 19066, so I am not going to continue on this.
Brill, thanks for the update Josef. [U+1F600]