Bug 21401 - Account offsets should save the transacting library
Summary: Account offsets should save the transacting library
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Josef Moravec
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 19066
Blocks: 21538
  Show dependency treegraph
 
Reported: 2018-09-24 06:22 UTC by Josef Moravec
Modified: 2019-03-01 08:33 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21401: Add column transaction_library to table account_offsets (2.84 KB, patch)
2018-09-24 07:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21401: Update schema (3.00 KB, patch)
2018-09-24 07:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21401: Add tests for transaction_library (2.46 KB, patch)
2018-09-24 07:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21401: Take transaction library from userenv when not defined (947 bytes, patch)
2018-09-24 07:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21401: Take transaction library from userenv when not defined (997 bytes, patch)
2018-09-24 09:42 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21401: Add column transaction_library to table account_offsets (2.93 KB, patch)
2018-09-24 09:46 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21401: Update schema (3.08 KB, patch)
2018-09-24 09:46 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21401: Add tests for transaction_library (2.55 KB, patch)
2018-09-24 09:46 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21401: Take transaction library from userenv when not defined (1.02 KB, patch)
2018-09-24 09:46 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21401: (follow-up) on update set null instead of cascade on transaction library (2.63 KB, patch)
2018-10-01 05:31 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21401: Add column transaction_library to table account_offsets (2.68 KB, patch)
2018-10-10 14:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21401: DO NOT PUSH - schema changes (3.04 KB, patch)
2018-10-10 14:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21401: Add tests for transaction_library (2.35 KB, patch)
2018-10-10 14:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21401: Take transaction library from userenv when not defined (1.03 KB, patch)
2018-10-10 14:59 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2018-09-24 06:22:30 UTC

    
Comment 1 Josef Moravec 2018-09-24 07:47:28 UTC
Created attachment 79272 [details] [review]
Bug 21401: Add column transaction_library to table account_offsets
Comment 2 Josef Moravec 2018-09-24 07:47:32 UTC
Created attachment 79273 [details] [review]
Bug 21401: Update schema
Comment 3 Josef Moravec 2018-09-24 07:47:35 UTC
Created attachment 79274 [details] [review]
Bug 21401: Add tests for transaction_library
Comment 4 Josef Moravec 2018-09-24 07:47:39 UTC
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
Comment 5 Michal Denar 2018-09-24 09:42:47 UTC
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>
Comment 6 Michal Denar 2018-09-24 09:46:38 UTC
Created attachment 79285 [details] [review]
Bug 21401: Add column transaction_library to table account_offsets

Signed-off-by: Michal Denare <black23@gmail.com>
Comment 7 Michal Denar 2018-09-24 09:46:42 UTC
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>
Comment 8 Michal Denar 2018-09-24 09:46:46 UTC
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>
Comment 9 Michal Denar 2018-09-24 09:46:50 UTC
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>
Comment 10 Jonathan Druart 2018-09-30 22:49:39 UTC
- Use cases?

- Maybe we should use ON DELETE SET NULL instead of cascade?
Comment 11 Josef Moravec 2018-10-01 05:06:48 UTC
(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.
Comment 12 Josef Moravec 2018-10-01 05:31:02 UTC
Created attachment 79688 [details] [review]
Bug 21401: (follow-up) on update set null instead of cascade on transaction library
Comment 13 Martin Renvoize 2018-10-08 15:14:07 UTC
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.
Comment 14 Martin Renvoize 2018-10-08 15:19:58 UTC
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 )
Comment 15 Josef Moravec 2018-10-10 08:01:43 UTC
(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...
Comment 16 Josef Moravec 2018-10-10 08:02:31 UTC
(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!
Comment 17 Josef Moravec 2018-10-10 14:59:46 UTC
Created attachment 80355 [details] [review]
Bug 21401: Add column transaction_library to table account_offsets
Comment 18 Josef Moravec 2018-10-10 14:59:50 UTC
Created attachment 80356 [details] [review]
Bug 21401: DO NOT PUSH - schema changes
Comment 19 Josef Moravec 2018-10-10 14:59:54 UTC
Created attachment 80357 [details] [review]
Bug 21401: Add tests for transaction_library
Comment 20 Josef Moravec 2018-10-10 14:59:58 UTC
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
Comment 21 Martin Renvoize 2018-10-10 16:29:31 UTC
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.
Comment 22 Josef Moravec 2018-10-11 06:26:51 UTC
I look at Koha::Account, and it does make sense to add it do log, do you have anything else in mind?
Comment 23 Martin Renvoize 2018-10-11 08:30:43 UTC
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 :)
Comment 24 Martin Renvoize 2018-10-11 13:35:19 UTC
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)
Comment 25 Jonathan Druart 2018-10-25 20:01:27 UTC
Same as bug 19066.
Comment 26 Josef Moravec 2018-10-26 08:21:23 UTC
I will rework this on top of 19066
- would like to add foreign key
- api changes
Comment 27 Martin Renvoize 2019-02-28 15:57:49 UTC
Was there any further work you wanted to undertake here Josef.. or did it all end up folded into bug 19066?
Comment 28 Josef Moravec 2019-03-01 06:11:15 UTC
(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.
Comment 29 Martin Renvoize 2019-03-01 08:33:59 UTC
Brill, thanks for the update Josef. 😀