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.
Created attachment 69848 [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.
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.
Created attachment 69849 [details] [review] Bug 19826: Add K::Acq::Budget(s) and K::Acq::Budget::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.
(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.
Created attachment 69891 [details] [review] [SIGNED-OFF] Bug 19826: Add K::Acq::Budget(s) and K::Acq::Budget::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>
Tests are missing.
(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.
(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 ? :)
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?
(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.
(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
(In reply to Jonathan Druart from comment #6) > Tests are missing. Is use_ok() enough? I can submit a followup for each class.
Created attachment 69918 [details] [review] Bug 19826: Add K::Acq::Budget(s) and K::Acq::Budget::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>
Created attachment 69919 [details] [review] Bug 19826: Unit test files
(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? :)
(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?
You can have several orders by basket, so it's 1-n
(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
Created attachment 70063 [details] [review] Bug 19826: Add K::Acq::Budget(s) and K::Acq::Budget::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>
Created attachment 70064 [details] [review] Bug 19826: Unit test files Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(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.
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>
Created attachment 70120 [details] [review] Bug 19826: Unit test files Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(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!
I am ok with this approach - I think our relations could get confusing fast.
Created attachment 70156 [details] [review] Bug 19826: Add tests
If you are going to reuse these new modules, I expect that you will replace the different subroutines using DBIC directly in C4::Budgets
(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.
Created attachment 70157 [details] [review] Bug 19826: Add tests
Pushed to master for 18.05, thanks to everybody involved!
Enhancement, skipping for 17.11.x. Awesome work everybody!