Bug 19826

Summary: Introduce Koha::Acquisition::Budget(s) and Koha::Acquisition::Fund(s)
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, julian.maurice, katrin.fischer, kyle, m.de.rooy, matthias.meusburger, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15408, 19661    
Attachments: Bug 19826: Add K::Acq::Budget(s) and K::Acq::Fund(s)
Bug 19826: Add K::Acq::Budget(s) and K::Acq::Budget::Fund(s)
[SIGNED-OFF] Bug 19826: Add K::Acq::Budget(s) and K::Acq::Budget::Fund(s)
Bug 19826: Add K::Acq::Budget(s) and K::Acq::Budget::Fund(s)
Bug 19826: Unit test files
Bug 19826: Add K::Acq::Budget(s) and K::Acq::Budget::Fund(s)
Bug 19826: Unit test files
Bug 19826: Add K::Acq::Budget(s) and K::Acq::Fund(s)
Bug 19826: Unit test files
Bug 19826: Add tests
Bug 19826: Add tests

Description Tomás Cohen Arazi 2017-12-18 14:22:35 UTC
In order to implement budgets and funds endpoints, we need to provide a way to get them through the Koha::Object(s) layer instead of just DBI+hashref. This way we can take advantage of DBIC annotations, TO_JSON, helper params handling and query builder, etc.
Comment 1 Tomás Cohen Arazi 2017-12-18 14:29:03 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2017-12-18 14:41:12 UTC
I think it would be better to use Koha::Acquisition::Budgets and Koha::Acquisitions::Budgets::Funds instead of having both modules be siblings. My proposed names would indicate the actual hierarchy and relationship of budgets and funds where funds belong to a given budget.
Comment 3 Tomás Cohen Arazi 2017-12-18 14:42:36 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2017-12-18 14:46:33 UTC
(In reply to Kyle M Hall from comment #2)
> I think it would be better to use Koha::Acquisition::Budgets and
> Koha::Acquisitions::Budgets::Funds instead of having both modules be
> siblings. My proposed names would indicate the actual hierarchy and
> relationship of budgets and funds where funds belong to a given budget.

I agree with indicating the relationship. I've submitted a modified version.
Comment 5 Aleisha Amohia 2017-12-19 02:45:45 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2017-12-19 14:39:24 UTC
Tests are missing.
Comment 7 Jonathan Druart 2017-12-19 14:41:22 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> (In reply to Kyle M Hall from comment #2)
> > I think it would be better to use Koha::Acquisition::Budgets and
> > Koha::Acquisitions::Budgets::Funds instead of having both modules be
> > siblings. My proposed names would indicate the actual hierarchy and
> > relationship of budgets and funds where funds belong to a given budget.
> 
> I agree with indicating the relationship. I've submitted a modified version.

I do not, CCing more QA people to get their opinions.
Comment 8 Marcel de Rooy 2017-12-19 14:46:25 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Tomás Cohen Arazi from comment #4)
> > (In reply to Kyle M Hall from comment #2)
> > > I think it would be better to use Koha::Acquisition::Budgets and
> > > Koha::Acquisitions::Budgets::Funds instead of having both modules be
> > > siblings. My proposed names would indicate the actual hierarchy and
> > > relationship of budgets and funds where funds belong to a given budget.
> > 
> > I agree with indicating the relationship. I've submitted a modified version.
> 
> I do not, CCing more QA people to get their opinions.

What about Biblio and Item ? :)
Comment 9 Katrin Fischer 2017-12-19 14:57:41 UTC
Hm, can understand both sides. Bibio and Item is a good example, others I can think of: aqbasket and aqorder.
Is there a technical reason we shoudl do one or the other?
Comment 10 Tomás Cohen Arazi 2017-12-19 15:51:09 UTC
(In reply to Katrin Fischer from comment #9)
> Hm, can understand both sides. Bibio and Item is a good example, others I
> can think of: aqbasket and aqorder.
> Is there a technical reason we shoudl do one or the other?

It is mostly a matter of taste, and how devs are used to read names and infere things from them.

In my opinion, if theres a one-to-many relationship, I is more obvious if I'm unfamiliar with the codebase, when I read it the way I wrote the patches.

I can re-submit the original version if the RM requires it. Or if there's some decision making deciding. Maybe worth voting on the next dev meeting. I bet it will be 2-2 and 6/8 people saying don't care LOL.
Comment 11 Tomás Cohen Arazi 2017-12-19 15:51:34 UTC
(In reply to Marcel de Rooy from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > (In reply to Tomás Cohen Arazi from comment #4)
> > > (In reply to Kyle M Hall from comment #2)
> > > > I think it would be better to use Koha::Acquisition::Budgets and
> > > > Koha::Acquisitions::Budgets::Funds instead of having both modules be
> > > > siblings. My proposed names would indicate the actual hierarchy and
> > > > relationship of budgets and funds where funds belong to a given budget.
> > > 
> > > I agree with indicating the relationship. I've submitted a modified version.
> > 
> > I do not, CCing more QA people to get their opinions.
> 
> What about Biblio and Item ? :)

That's a good rename too :-D
Comment 12 Tomás Cohen Arazi 2017-12-19 15:52:03 UTC
(In reply to Jonathan Druart from comment #6)
> Tests are missing.

Is use_ok() enough? I can submit a followup for each class.
Comment 13 Tomás Cohen Arazi 2017-12-19 16:03:06 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2017-12-19 16:03:14 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2017-12-19 16:23:43 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> (In reply to Jonathan Druart from comment #6)
> > Tests are missing.
> 
> Is use_ok() enough? I can submit a followup for each class.

I usually reuse and adapt Cities.t, it introduces a base for next tests.

(In reply to Tomás Cohen Arazi from comment #10)
> In my opinion, if theres a one-to-many relationship, I is more obvious if
> I'm unfamiliar with the codebase, when I read it the way I wrote the patches.

Hum, so Koha::Acquisition::Vendor::Basket::Order? :)
Comment 16 Tomás Cohen Arazi 2017-12-19 17:31:46 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Tomás Cohen Arazi from comment #10)
> > In my opinion, if theres a one-to-many relationship, I is more obvious if
> > I'm unfamiliar with the codebase, when I read it the way I wrote the patches.
> 
> Hum, so Koha::Acquisition::Vendor::Basket::Order? :)

I'm not sure, I think there is a 1-1 relationship between baskets and orders?
Comment 17 Jonathan Druart 2017-12-19 17:51:04 UTC
You can have several orders by basket, so it's 1-n
Comment 18 Julian Maurice 2017-12-19 22:09:48 UTC
(In reply to Jonathan Druart from comment #15)
> Hum, so Koha::Acquisition::Vendor::Basket::Order? :)

Please, no :)
Not everything can be represented in a tree.
Where do you place basket groups in this hierarchy ? It could be Koha::Acq::Vendor::Basketgroup::Basket, but not every basket belongs to a basketgroup.
Also, there is a 1-n relationship between biblio and aqorders, but Koha::Biblio::Orders does not make much sense IMHO.

If there was a vote, I'd vote for everything on one level
Comment 19 Kyle M Hall 2017-12-21 16:08:55 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2017-12-21 16:09:04 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2017-12-21 16:15:25 UTC
(In reply to Julian Maurice from comment #18)
> (In reply to Jonathan Druart from comment #15)
> > Hum, so Koha::Acquisition::Vendor::Basket::Order? :)
> 
> Please, no :)
> Not everything can be represented in a tree.
> Where do you place basket groups in this hierarchy ? It could be
> Koha::Acq::Vendor::Basketgroup::Basket, but not every basket belongs to a
> basketgroup.
> Also, there is a 1-n relationship between biblio and aqorders, but
> Koha::Biblio::Orders does not make much sense IMHO.
> 
> If there was a vote, I'd vote for everything on one level

Not everything, but I would prefer Koha::Acq::Budget[s] and Koha::Acq::fund[s] too.
Setting to In Discussion to get more opinions.
Comment 22 Tomás Cohen Arazi 2017-12-22 14:13:14 UTC
Created attachment 70119 [details] [review]
Bug 19826: Add K::Acq::Budget(s) and K::Acq::Fund(s)

This patch introduces Koha::Object(s)-derived classes for budgets and funds (aqbudgetperiods and aqbudgets respectively).

C4::Budgets already uses DBIC calls on those tables so transition should be easy on top of this.

To test:
- Check the referenced schema files are the right ones.
- QA scripts shoudl be happy.
- Sign off :-D

Note: the POD coverage report throws a false negative result on singular classes which is trivially wrong.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Tomás Cohen Arazi 2017-12-22 14:13:27 UTC
Created attachment 70120 [details] [review]
Bug 19826: Unit test files

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Tomás Cohen Arazi 2017-12-22 14:17:33 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Julian Maurice from comment #18)
> > (In reply to Jonathan Druart from comment #15)
> > > Hum, so Koha::Acquisition::Vendor::Basket::Order? :)
> > 
> > Please, no :)
> > Not everything can be represented in a tree.
> > Where do you place basket groups in this hierarchy ? It could be
> > Koha::Acq::Vendor::Basketgroup::Basket, but not every basket belongs to a
> > basketgroup.
> > Also, there is a 1-n relationship between biblio and aqorders, but
> > Koha::Biblio::Orders does not make much sense IMHO.
> > 
> > If there was a vote, I'd vote for everything on one level
> 
> Not everything, but I would prefer Koha::Acq::Budget[s] and
> Koha::Acq::fund[s] too.
> Setting to In Discussion to get more opinions.

I don't really care, really. I've moved Fund(s).pm one level up, and marked PQA so Matthias can code the funds endpoint on top of this. Thanks!
Comment 25 Katrin Fischer 2017-12-22 14:19:51 UTC
I am ok with this approach - I think our relations could get confusing fast.
Comment 26 Jonathan Druart 2017-12-26 15:34:54 UTC
Created attachment 70156 [details] [review]
Bug 19826: Add tests
Comment 27 Jonathan Druart 2017-12-26 15:37:58 UTC
If you are going to reuse these new modules, I expect that you will replace the different subroutines using DBIC directly in C4::Budgets
Comment 28 Tomás Cohen Arazi 2017-12-26 15:43:44 UTC
(In reply to Jonathan Druart from comment #27)
> If you are going to reuse these new modules, I expect that you will replace
> the different subroutines using DBIC directly in C4::Budgets

I will :-D. Matthias is the one using this on the funds endpoint, though.
Comment 29 Jonathan Druart 2017-12-26 15:52:23 UTC
Created attachment 70157 [details] [review]
Bug 19826: Add tests
Comment 30 Jonathan Druart 2017-12-26 15:53:51 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 31 Nick Clemens 2018-01-16 12:16:28 UTC
Enhancement, skipping for 17.11.x.
Awesome work everybody!