Bug 13019 - Add base classes on which to build Koha objects
Summary: Add base classes on which to build Koha objects
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 11703
Blocks: 10363 11431 13719 13726 13738 14544 14616 15446 16961 16965
  Show dependency treegraph
 
Reported: 2014-10-01 11:32 UTC by Kyle M Hall
Modified: 2017-06-14 22:10 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13019 - Add base classes on which to build Koha objects (20.43 KB, patch)
2014-10-01 11:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (20.83 KB, patch)
2014-10-06 12:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (20.88 KB, patch)
2014-10-06 12:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (20.89 KB, patch)
2014-10-06 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (20.90 KB, patch)
2014-10-06 13:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (21.22 KB, patch)
2014-10-07 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (21.28 KB, patch)
2014-10-07 15:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (21.31 KB, patch)
2014-10-07 16:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (22.57 KB, patch)
2014-10-08 10:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (22.96 KB, patch)
2014-10-08 12:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (21.31 KB, patch)
2014-10-08 14:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (22.95 KB, patch)
2014-10-08 16:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (23.05 KB, patch)
2014-11-10 13:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (22.99 KB, patch)
2014-11-10 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13019: (follow-up) Remove smartmatch operator (4.11 KB, patch)
2014-11-10 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13019 [QA Followup] - Allow find() and search() to be called as static methods (2.20 KB, patch)
2014-11-21 17:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019: [QA Follow-up] Rename new_from_dbic and few typos (4.30 KB, patch)
2014-11-24 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (23.11 KB, patch)
2014-11-24 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13019: (follow-up) Remove smartmatch operator (4.21 KB, patch)
2014-11-24 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13019 [QA Followup] - Allow find() and search() to be called as static methods (2.29 KB, patch)
2014-11-24 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13019: [QA Follow-up] Rename new_from_dbic and few typos (4.30 KB, patch)
2014-11-24 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13019: Follow-up for find in Koha::Object (3.05 KB, patch)
2014-11-24 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (23.14 KB, patch)
2014-11-24 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019: (follow-up) Remove smartmatch operator (4.24 KB, patch)
2014-11-24 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 [QA Followup] - Allow find() and search() to be called as static methods (2.32 KB, patch)
2014-11-24 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019: [QA Follow-up] Rename new_from_dbic and few typos (4.33 KB, patch)
2014-11-24 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 [QA Followup] - Remove use of encode (830 bytes, patch)
2014-11-24 13:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 [QA Followup] - Remove use of encode (927 bytes, patch)
2014-11-24 13:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13019 [QA Followup] - Allow chaining (1.40 KB, patch)
2015-02-10 18:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 [QA Followup] - Fix stale unit test (910 bytes, patch)
2015-02-10 19:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 [QA Followup] - Fix stale unit test (914 bytes, patch)
2015-02-10 19:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Store list of objects as needed (2.55 KB, patch)
2015-02-17 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-10-01 11:32:28 UTC
The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example.

Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it.

By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits.
Comment 1 Kyle M Hall 2014-10-01 11:37:26 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-10-01 12:40:00 UTC
First thoughts (already sent on #koha):

1/ Type and ObjectClass could be attributes, instead of methods
2/ I really would like to see method name without capital letters (and underscore '_' to separate words)
3/ "Bad setter returns undef" => not sure about that. Bad setter should croak
4/ What about using Class::Accessor?
5/ t/db_dependent/Objects.t should be t/db_dependent/Borrowers.t
Comment 3 Kyle M Hall 2014-10-01 17:20:22 UTC
> 1/ Type and ObjectClass could be attributes, instead of methods

Could write a followup? I tried to do that but had issues with getting the base class to know about the package variable. I was properly implementing it wrong.

> 2/ I really would like to see method name without capital letters (and
> underscore '_' to separate words)

I have no strong preference here as long as we stick to one convention. My only concern would be a convention conflict where a table has the same name as a field in the table. That would lead to ambiguity, but I'm not sure if we even have an instance of this. For example, let's say a table has the field borrower which is actually a borrowernumber. I'd like to have $row->borrower return the borrowernumber, but $row->Borrower return the Koha::Borrower object. That way we know when we are getting back a scalar, and when we are getting back an Object. It would be silly to fetch the Koha::Borrower in a case where we just need the borrowernumber, which we would already have.

> 3/ "Bad setter returns undef" => not sure about that. Bad setter should croak

Agreed.

> 4/ What about using Class::Accessor?

I don't see the advantage to this. Can you give an example. This implementation uses a DBIC object as a place to store the data, so there is no need to have separate properties. Please note the this should not conflict this unit testing on a database-less system. t/Object.t should demonstrate this.

> 5/ t/db_dependent/Objects.t should be t/db_dependent/Borrowers.t

I can see your point, but the tests are really testing the base classes, and not the Borrower classes per se. However, I have no strong opinion so that's fine by me.
Comment 4 Jonathan Druart 2014-10-02 12:25:09 UTC
I tried to make Koha::Borrower extends Koha::Schema::Result::Borrower, but I get an error.
The idea was to remove the AUTOLOAD and avoid the IsStored, IsChanged, Id, etc. methods.

[the error was: Unable to perform storage-dependent operations with a detached result source (source '_unnamed_' is not associated with a schema)]
I suppose I tried to do something very bad :)

So, some additional remarks on the patch:
- "result" and "columns" should be _result and _columns to avoid collisions later, don't you think?
- Are you sure it is useful to test if the row is stored and has been changed (Store method)? Not sure about the perf, maybe DBIC does that (?).
- IsStored should be InStorage or completely removed, if useless.
- Delete could call delete in an eval, to avoid a call to in_storage
- what about 2 constructors? new and new_from_rs? The ResultSet should not be create if not used later
- what about a _result method? It would return ->{_result} if already exists, otherwise create it. All occurrences to ->{result} will be replaced by ->_result
Comment 5 Kyle M Hall 2014-10-03 17:19:51 UTC
(In reply to Jonathan Druart from comment #4)
> I tried to make Koha::Borrower extends Koha::Schema::Result::Borrower, but I
> get an error.
> The idea was to remove the AUTOLOAD and avoid the IsStored, IsChanged, Id,
> etc. methods.
> 
> [the error was: Unable to perform storage-dependent operations with a
> detached result source (source '_unnamed_' is not associated with a schema)]
> I suppose I tried to do something very bad :)
> 
> So, some additional remarks on the patch:

> - "result" and "columns" should be _result and _columns to avoid collisions
> later, don't you think?

Good idea

> - Are you sure it is useful to test if the row is stored and has been
> changed (Store method)? Not sure about the perf, maybe DBIC does that (?).

Yeah, I'd have to check the DBIC code, but that seems entirely likely.

> - IsStored should be InStorage or completely removed, if useless.

Again, your probably right. I can't see a use for this right now and we can always add an InStorage method at a later date if it *is* needed for something.

> - Delete could call delete in an eval, to avoid a call to in_storage

Do you think that will improve efficiency? in_storage has already been calculated and is just a dbic property. It seems like using eval would take more time.

> - what about 2 constructors? new and new_from_rs? The ResultSet should not
> be create if not used later

For Koha::Object? That's a good idea.

> - what about a _result method? It would return ->{_result} if already
> exists, otherwise create it. All occurrences to ->{result} will be replaced
> by ->_result

Also a good idea.

I'll see if I can't make some time next week to implement these changes.
Comment 6 Kyle M Hall 2014-10-06 12:04:39 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2014-10-06 12:08:59 UTC
(In reply to Jonathan Druart from comment #4)
> - "result" and "columns" should be _result and _columns to avoid collisions
> later, don't you think?

Done!

> - Are you sure it is useful to test if the row is stored and has been
> changed (Store method)? Not sure about the perf, maybe DBIC does that (?).

DBIC does indeed do that already. In fact, DBIC will only update changed columns and skipped those that are unchanged.

> - IsStored should be InStorage or completely removed, if useless.

Changed to InStorage

> - Delete could call delete in an eval, to avoid a call to in_storage

This one I did not implement, in_storage is really just a property to I believe it is quicker than using eval. We can always run some tests to see which is really faster.

> - what about 2 constructors? new and new_from_rs? The ResultSet should not
> be create if not used later

Done!

> - what about a _result method? It would return ->{_result} if already
> exists, otherwise create it. All occurrences to ->{result} will be replaced
> by ->_result

Done! Also done for _columns as well since it depends on _result
Comment 8 Kyle M Hall 2014-10-06 12:38:51 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2014-10-06 12:43:31 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2014-10-06 13:06:25 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2014-10-07 15:09:28 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2014-10-07 15:27:06 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-10-07 16:09:09 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-10-08 10:34:33 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2014-10-08 12:08:18 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2014-10-08 14:09:41 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-10-08 16:42:32 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2014-10-23 13:05:41 UTC
I have been playing around allot with this lately and my general feeling is that it's pretty workable already and a viable route to using proper objects in Koha. Whilst it has allot of further enhancements required to make it 'really' useful, it's in a good state as is for a first step as is.

I'll happily QA if someone else wants to signoff.. or I can signoff if another of the QA team is happy expending the effort of getting to grips with it ;)

My general thoughts as a whole follow:

I still feel the objects kinda of lock away allot of the advantages of dbic behind an abstraction that sometimes detracts rather than enhances; but with our debates going on and on I'de rather see Koha moving forward than stagnating in debate territory.  The only other option I see perhaps working is setting a bunch of coding rules/guidelines really pushing people in the direction of consistent objects.

My real feeling after all this is that we're doing way too much logical work in our .pl's.. if it's logic based at all, it really belongs in a .pm where it can be fully tested ;-) ! (Though I'de love to see Services in PM's tested, and UI in Angular Tested as a future route.. I'll be posting up some demo code for such soon)

This one is holding up bug 12892 now, so I'd really like to encourage some additional input, signoff and qa thought go into it asap :)
Comment 19 Marcel de Rooy 2014-10-27 09:59:20 UTC
Some questions after looking at the code:

Clean code on itself, still needs more documentation. 
Is the idea now to allow DBIC methods ONLY in Koha::Object and Objects? This is currently not achieved if you need to pass in a resultset.
Do we enforce its use in order to prevent chaos? This means creating a lot of new modules rightaway..

Having new and new_from_dbic was qualified above somewhere as a good idea. Why? I would merge them. Let new figure out what it receives. [If you want to encapsulate dbic btw, do not use that name when constructing your object.]
Instead of passing the resultset, you could add a find method in the object class or implement the same via the new constructor. Something like:
my $borrower= Koha::Borrower->find( $number );
or:
my $borrower= Koha::Borrower->new( $number );
New c/should see that this is different from passing { number => .., name => ..}
Comment 20 Katrin Fischer 2014-11-04 19:16:18 UTC
What is needed to get this moving again? Kyle, could you take a look at the last 2 comments?
Comment 21 Jonathan Druart 2014-11-10 13:16:20 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-11-10 13:17:22 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-11-10 13:17:27 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2014-11-10 13:51:20 UTC
(In reply to Jonathan Druart from comment #23)
Jonathan,
Any thoughts on comments 18 and 19?

Marcel
Comment 25 Jonathan Druart 2014-11-10 14:48:18 UTC
(In reply to M. de Rooy from comment #24)
> (In reply to Jonathan Druart from comment #23)
> Jonathan,
> Any thoughts on comments 18 and 19?

As Martin said, this is the first step to get modules in Koha using DBIC with a common way.
It will allow developers to easily instantiate objects in pl script.
We already discussed about the abstraction layer and I agree with it. Moreover Kyle fixed/implemented my concerns (comments 2 and 4).

> Is the idea now to allow DBIC methods ONLY in Koha::Object and Objects? This
> is currently not achieved if you need to pass in a resultset.
> Do we enforce its use in order to prevent chaos? This means creating a lot
> of new modules rightaway..

Yes of course. We will only add methods in Koha::Object[s] if needed and make sense.
Create 1 module per class in the Koha namespace is the logical consequence of this patch, it's not a problem.

> Having new and new_from_dbic was qualified above somewhere as a good idea.
> Why?

The first implementation of the constructor was uselessly complicated.
I believe this will add readability.

We already iterated several times with Kyle and other peoples involved, now it's the time to see something pushed and go further.
It's always possible to improve this code later if it doesn't match our needs or cause problems.
Comment 26 Marcel de Rooy 2014-11-12 14:23:28 UTC
(In reply to Jonathan Druart from comment #25)
Please be assured that I appreciate your work and eagerness to get things into Koha. I also know that it can be somewhat frustrating to see things develop rather slow. Since this is meant to be a very fundamental object, I would not recommend hurry here btw.

> Yes of course. We will only add methods in Koha::Object[s] if needed and
> make sense.
This sounds like a misunderstanding. If it would be so obvious to not have DBIx code in a perl script (or unit test!), why does Borrower.t contain constructs like: 
my $borrower = Koha::Database->new()->schema()->resultset('Borrower')->find( $borrowernumber );

Just replace it by something like Koha::Borrower->find( $borrowernumber ) or even extend new with similar functionality, as commented before. This would make new_from_dbic as external method (!! -- could be private though with an underscore) and passing resultsets superfluous. Leave the result set stuff in the objects.

Also note that Koha::Borrowers->new()->find is not really elegant. Why not offer both Koha::Borrower->find and Koha::Borrowers->find without going through new?

With some minimal changes in that direction, I would be inclined to pass qa..
Comment 27 Kyle M Hall 2014-11-21 17:27:36 UTC
(In reply to M. de Rooy from comment #26)
> (In reply to Jonathan Druart from comment #25)
> Please be assured that I appreciate your work and eagerness to get things
> into Koha. I also know that it can be somewhat frustrating to see things
> develop rather slow. Since this is meant to be a very fundamental object, I
> would not recommend hurry here btw.

I assure you, we haven't hurried ; ) This has taken months of discussion and proofs of concept so far!

> > Yes of course. We will only add methods in Koha::Object[s] if needed and
> > make sense.
> This sounds like a misunderstanding. If it would be so obvious to not have
> DBIx code in a perl script (or unit test!), why does Borrower.t contain
> constructs like: 
> my $borrower = Koha::Database->new()->schema()->resultset('Borrower')->find(
> $borrowernumber );

It's just a way to test that our objects are actually making changes to the database. We don't want to test our objects by using our objects!

> Just replace it by something like Koha::Borrower->find( $borrowernumber ) or
> even extend new with similar functionality, as commented before. This would
> make new_from_dbic as external method (!! -- could be private though with an
> underscore) and passing resultsets superfluous. Leave the result set stuff
> in the objects.

new_from_dbic will probably never be used in actual Koha code, but it needs to be external for the objects to use. You should never see new_from_dbic used in a pl file.

> 
> Also note that Koha::Borrowers->new()->find is not really elegant. Why not
> offer both Koha::Borrower->find and Koha::Borrowers->find without going
> through new?

Right now those methods are not static. That's an improvement we can work on over time! It's at least more elegant than Koha::Database->new()->schema()->resultset('Borrower')->find( $borrowernumber ); don't you think? ; )
Comment 28 Kyle M Hall 2014-11-21 17:40:55 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2014-11-21 17:42:25 UTC
This latest followup should take care of this!

> Also note that Koha::Borrowers->new()->find is not really elegant. Why not
> offer both Koha::Borrower->find and Koha::Borrowers->find without going
> through new?
> 
> With some minimal changes in that direction, I would be inclined to pass qa..
Comment 30 Marcel de Rooy 2014-11-24 12:29:45 UTC
(In reply to Kyle M Hall from comment #27)
> It's just a way to test that our objects are actually making changes to the
> database. We don't want to test our objects by using our objects!

I think that would be the only legitimate excuse :) But in that case it should not be in Borrowers. However, since our object is just a very smaller wrapper around DBIC, we are actually comparing DBIC with DBIC. So no big deal.
Comment 31 Marcel de Rooy 2014-11-24 12:35:35 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2014-11-24 12:36:15 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2014-11-24 12:36:20 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2014-11-24 12:36:24 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2014-11-24 12:36:29 UTC Comment hidden (obsolete)
Comment 36 Marcel de Rooy 2014-11-24 12:36:32 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2014-11-24 12:38:57 UTC
Kyle:
Before proceeding, I would like to know your opinion on the last patch. It is a suggestion to add find on Object.pm. What do you think?

Still another question: Is the return encode( 'UTF-8', $value ) really necessary? I have my doubts..
Comment 38 Kyle M Hall 2014-11-24 12:52:02 UTC
Comment on attachment 33845 [details] [review]
Bug 13019: Follow-up for find in Koha::Object

I highly recommend we don't include this patch. All logic related to getting one or more objects should stay in the Objects class. That way we have uniformity and no confusion over how to call find.
Comment 39 Kyle M Hall 2014-11-24 12:58:17 UTC
Created attachment 33846 [details] [review]
Bug 13019 - Add base classes on which to build Koha objects

The idea behind this is to have a pair of base classes on which to build
our new generation of Koha objects. Koha::Object is a base class, which
in it's most basic form, is to represent a row in a table. For example,
Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio
and Koha::Item for example.

Koha::Objects is to represent a way to fetch and manipulate sets of
objects. For example, Koha::Borrowers has a method to get a
Koha::Borrower object by id and a method to search for an get a list
of Koha::Borrower objects. Right now Koha::Objects has only the
essentials but can easily be extended and those enhancements will be
passed down to all the child classes based on it.

By using these classes as a base, we will add consistency to our
code, allow us to keep our code DRY, reduce bugs, and encapsulate our
database access among other benefits.

Test Plan:
1) Apply this patch
2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Kyle M Hall 2014-11-24 12:58:26 UTC
Created attachment 33847 [details] [review]
Bug 13019: (follow-up) Remove smartmatch operator

This patch also adds 1 test.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Kyle M Hall 2014-11-24 12:58:29 UTC
Created attachment 33848 [details] [review]
Bug 13019 [QA Followup] - Allow find() and search() to be called as static methods

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Kyle M Hall 2014-11-24 12:58:31 UTC
Created attachment 33849 [details] [review]
Bug 13019: [QA Follow-up] Rename new_from_dbic and few typos

Since new_from_dbic is not meant as a public method, this patch adds
a prefix to the name of this internal routine. For the same reason I
removed it from t/Borrower.t.
Removed one use overload-line in Objects (not used).
Resolved a few typos.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Kyle M Hall 2014-11-24 13:07:42 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2014-11-24 13:11:27 UTC
Created attachment 33852 [details] [review]
Bug 13019 [QA Followup] - Remove use of encode

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2014-11-24 13:21:42 UTC
QA Comment:
Some minor concerns have been addressed in the meantime.
This topic waits for some time now and has been discussed at various times, but without too much progress.
I think this set of patches is an interesting starting point now.
The Borrower example submitted now is too small to really show the advantages/disadvantages of this approach.
The code looks good. No complaints from qa tools.

I think we should push this now and first build a object on it, used in real Koha code (not just some tests). After evaluating that "exercise", we can think of adapting the coding guidelines.
Note that the object still need some further documentation, but I do not mind doing that in follow-ups later. Let's get this further now.

Passed QA
Comment 46 Jonathan Druart 2014-11-24 13:58:33 UTC
(In reply to M. de Rooy from comment #45)
> I think we should push this now and first build a object on it, used in real
> Koha code (not just some tests). After evaluating that "exercise", we can
> think of adapting the coding guidelines.

See bug 10363.
Comment 47 David Cook 2014-11-25 05:53:48 UTC
Comment on attachment 33846 [details] [review]
Bug 13019 - Add base classes on which to build Koha objects

Review of attachment 33846 [details] [review]:
-----------------------------------------------------------------

::: Koha/Object.pm
@@ +136,5 @@
> +=head3 $object->delete();
> +
> +Removes the object from storage.
> +
> +Returns:

I can see returning true on success, but shouldn't we throw an exception if the deletion fails or if the object was never in storage?

I would think it would make more sense to try a proper exception using die(), and then catch that when deleting an object using eval() or Try::Tiny (which I think is a prereq of DBIC anyway).

Mind you, off the top of my head, I'm not sure what exceptions DBIC throws if a delete fails or if the object was never there to begin with...

::: Koha/Objects.pm
@@ +187,5 @@
> +
> +sub _wrap {
> +    my ( $self, @dbic_rows ) = @_;
> +
> +    my @objects = map { $self->object_class()->new_from_dbic( $_ ) } @dbic_rows;

Mmm, I see... so you use Koha::Objects for fetching objects from the database... and it wraps the resultset with the applicable object based on Koha::Object...
Comment 48 David Cook 2014-11-25 07:38:11 UTC
Looking over this again... I don't know if we're implementing Koha::Objects in the best way possible.

After all, every Koha::Objects subclass is basically just going to consist of the following:

44	sub type {
45	    return 'Borrower';
46	}
47	 
48	sub object_class {
49	    return 'Koha::Borrower';
50	}

That seems like unnecessary duplication in my mind. Why not follow the DBIC model, and go with something like the following?

my $Borrowers= Koha::Objects->new('Borrower');
my @objects = $Borrowers->search({surname => 'Jones'});

I suppose the limitation of that is that you can't have custom methods for the group of objects, but would you need them? I imagine all the custom methods are going to be at the individual object level.

--

I must admit, after looking through DBIC more and more, I'm less sure of the encapsulation method.

I wonder how much functionality we'll use by encapsulating the DBIC objects in Koha objects.

Yet, DBIC objects on their own don't have enough functionality to be all we need.

Or do they?

Maybe we should keep DBIC objects as data objects, and then have other modules/objects for applying business logic to those objects.

In terms of CRUD, DBIC is fine the way it is.

But let's think about a Koha::Biblio object... it could have methods for importing, exporting, filtering, extracting data from a Koha::Schema::Biblio object or a Koha::Schema::Deletedbiblio object.

Koha::Biblio::new($biblionumber);
or maybe
Koha::Biblio::new($dbic_object); #not dissimilar to using inheritance, except that you can use Biblio or Deletedbiblio in this case

my $output = Koha::Biblio::format("opac","marcxml"); #detail view
my $output = Koha::Biblio::format("opac","isomarc"); #exporting as isomarc
my $output = Koha::Biblio::format("intranet","marcxml"); #detail view
my $output = Koha::Biblio::format("intranet","dc"); #detail view as dublin core

--

In my latest OAI stuff, I have DBIC objects for OAI records and OAI repositories. I then have a Harvester.pm (which has methods for getting active repositories, querying the repositories, queueing the records, etc.) and an Importer.pm (gets the queued records, analyzes them, transforms metadata, calls the internal C4::Biblio API for adding records to the database). 

Biblio is actually pretty interesting... because you can get biblios in all sorts of ways. From Zebra, from MySQL, from Z39.50, from OAI, from importing through other tools. You should be able to feed ISOMARC or MARCXML into Koha::Biblio and have that take care of everything thereafter.

Actually, Koha::Biblio might not even be the most appropriate name in that case. Maybe it should be Koha::Record, and that becomes the one and only access point for that record.

I suppose though... if you were doing a DBIC search, you'd have to feed your result into Koha::Record before you could work on it... and you might be tempted to avoid Koha::Record. Whereas if you're using Koha::Records or Koha::Biblios and that returns the DBIC object wrapped in a Koha object... then you're forced to use that (unless you bypass Koha::Biblios and just use DBIC on its own).

But what is a "biblio"... when do we use it... 

1) We add biblios to the catalogue.

We might do this by hand, by Z39.50, from OAI, or some other means. But basically, we have a metadata record in MARC format.

2) We search for biblios in the catalogue

We query Zebra and it returns biblio records. We see a list of search results.

3) We examine individual records

We click on the results and look at the records

4) We edit the metadata records

We load the MARCXML into a form that can be edited by humans, and then save it again into the relational database (where it gets stored in biblioitems.marcxml, and processed into the biblio and biblioitems tables)

5) We export records (to Zebra, to browsers for download, via OAI, via Z39.50, etc)

We output the MARCXML to other sources

--

I'm sure there are other use cases of biblios, but the thing I continually notice is that most of these probably wouldn't need the DBIC object. Adding and editing would, as the MARCXML would need to be stored in biblioitems.marcxml, and the MARCXML would need to be parsed into the other database fields.

Most of the `biblio` and `biblioitems` fields are calculated fields rather than data in their own right.

--

That all being said... I like the idea of the code being more DRY.

So maybe it does make sense to have a Koha::Object base class that defines methods for adding, modifying, and deleting database data.

Hmm... but I'm not sure that can work either.

Thinking about "Biblio" or "Borrower", when you delete one of these, you don't want to just nuke it from the database. You want to move it to the "Deletedbiblio" or "Deletedborrower" then nuke it.

I suppose you could always have standard methods and then override them in these cases.

Likewise for moving issues into old_issues, reserves into old_reserves, etc.

We'd probably always want a single Acquisitions::Budgets object even though there are multiple tables. 

--

Ultimately... I don't think the approach of wrapping DBIC objects with Koha objects will work. I think it would work in some cases, but I think it wouldn't be flexible enough to accommodate all cases.

I think they get a bit repetitive and automagical at times as well, which might be confusing.

That said, maybe we should try them out a bit in the wild and see how it goes.
Comment 49 David Cook 2014-12-01 02:09:37 UTC
Actually, I'm going to put this one to "in discussion", as I don't think it's ready to be pushed to master. I think we really should spend some more time thinking through how we want the next generation of Koha::* to progress.

Perhaps this should find its way to the listserv as well, as I think it's a conversation that is applicable to more than just the 5 or 6 of us.
Comment 50 Jonathan Druart 2014-12-01 08:47:18 UTC
(In reply to David Cook from comment #49)
> Actually, I'm going to put this one to "in discussion", as I don't think
> it's ready to be pushed to master. I think we really should spend some more
> time thinking through how we want the next generation of Koha::* to progress.
> 
> Perhaps this should find its way to the listserv as well, as I think it's a
> conversation that is applicable to more than just the 5 or 6 of us.

I personally don't want to discuss this implementation for the following reasons:
1/ We already have discussed (or at least tried) for years.
2/ We already have different proposals (At least from Kyle, Yohann and me).
3/ This code is functional and we will able to improve it according to our further needs.
4/ Everybody knows that putting a bug report to the 'in discussion' status without any other actions (mail on mailing list, discussion on #koha, etc) don't permit to move it forward.

I strongly believe that the next step is to get the RM point of view.

You can provide a patch on another report to improve the code and show us what could be better, that would be great :)
Comment 51 David Cook 2014-12-07 22:14:42 UTC
(In reply to Jonathan Druart from comment #50)
> I personally don't want to discuss this implementation for the following
> reasons:
> 1/ We already have discussed (or at least tried) for years.
> 2/ We already have different proposals (At least from Kyle, Yohann and me).
> 3/ This code is functional and we will able to improve it according to our
> further needs.
> 4/ Everybody knows that putting a bug report to the 'in discussion' status
> without any other actions (mail on mailing list, discussion on #koha, etc)
> don't permit to move it forward.
> 
> I strongly believe that the next step is to get the RM point of view.
> 
> You can provide a patch on another report to improve the code and show us
> what could be better, that would be great :)

Fair enough. Maybe the next step is to get the RM point of view.

While I do have ideas for what I think would be better code and a better proposal, I think it would need to be a more concerted team effort  (admittedly, that has historically been extraordinarily difficult to achieve). As for individual work, I'm not willing to dedicate the time in my personal life at the moment, and I don't have the time to do it professionally, so I suppose it's not fair for me to be too critical if I can't provide any work myself.

Feel free to switch it back to Passed QA.
Comment 52 Kyle M Hall 2014-12-12 14:51:58 UTC
I agree with Jonathan. This has been discussed in great detail. This *has* been a team effort with much input from Jonathan and Martin. This is also the third iteration of this idea that I've proposed. It would be great if we could get input from every single person, but there will always be opposing opinions on how things should be done. At this point, I ( and probably others ) just want something, anything, we can get and use. This does not by any means prevent us from making improvements in the future! But we need to start somewhere.

(In reply to David Cook from comment #51)
> (In reply to Jonathan Druart from comment #50)
> > I personally don't want to discuss this implementation for the following
> > reasons:
> > 1/ We already have discussed (or at least tried) for years.
> > 2/ We already have different proposals (At least from Kyle, Yohann and me).
> > 3/ This code is functional and we will able to improve it according to our
> > further needs.
> > 4/ Everybody knows that putting a bug report to the 'in discussion' status
> > without any other actions (mail on mailing list, discussion on #koha, etc)
> > don't permit to move it forward.
> > 
> > I strongly believe that the next step is to get the RM point of view.
> > 
> > You can provide a patch on another report to improve the code and show us
> > what could be better, that would be great :)
> 
> Fair enough. Maybe the next step is to get the RM point of view.
> 
> While I do have ideas for what I think would be better code and a better
> proposal, I think it would need to be a more concerted team effort 
> (admittedly, that has historically been extraordinarily difficult to
> achieve). As for individual work, I'm not willing to dedicate the time in my
> personal life at the moment, and I don't have the time to do it
> professionally, so I suppose it's not fair for me to be too critical if I
> can't provide any work myself.
> 
> Feel free to switch it back to Passed QA.
Comment 53 David Cook 2014-12-23 00:54:37 UTC
(In reply to Kyle M Hall from comment #52)
> I agree with Jonathan. This has been discussed in great detail. This *has*
> been a team effort with much input from Jonathan and Martin. This is also
> the third iteration of this idea that I've proposed. It would be great if we
> could get input from every single person, but there will always be opposing
> opinions on how things should be done. At this point, I ( and probably
> others ) just want something, anything, we can get and use. This does not by
> any means prevent us from making improvements in the future! But we need to
> start somewhere.
> 

With deference to Jonathan and Martin, I was referring to the overarching community when I wrote "team". I know there has been some discussion here and on the lists, but I don't think there was ever any sort of consensus on the best way to move forward. That said, I know you have proposed a lot of different ideas, and kudos for that. I also know that the perfect is the enemy of the good.

While I would like to avoid technical debt, I suppose it can't hurt to try out multiple ideas. Since the majority of code also comes from yourself and Jonathan, it probably makes sense to use a system that you both find useful.

So long as Koha is working well for librarians and patrons, I can't really complain.
Comment 54 Kyle M Hall 2015-02-10 18:37:28 UTC
Created attachment 35813 [details] [review]
Bug 13019 [QA Followup] - Allow chaining

By returning the object itself instead of a boolean, we can chain
methods together while retaining the exact same functionality.
Comment 55 Kyle M Hall 2015-02-10 19:02:17 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2015-02-10 19:12:23 UTC
Created attachment 35815 [details] [review]
Bug 13019 [QA Followup] - Fix stale unit test
Comment 57 Tomás Cohen Arazi 2015-02-12 18:31:54 UTC
Patchset pushed to master.

Thanks Kyle, Jonathan and Marcel!
Comment 58 Kyle M Hall 2015-02-13 12:43:59 UTC
(In reply to Tomás Cohen Arazi from comment #57)
> Patchset pushed to master.
> 
> Thanks Kyle, Jonathan and Marcel!

Documentation for using Koha::Object and Koha::Objects can be found on the community wiki ( http://wiki.koha-community.org/wiki/Koha_Objects ).
Comment 59 Kyle M Hall 2015-02-17 16:08:57 UTC Comment hidden (obsolete)
Comment 60 Tomás Cohen Arazi 2015-02-17 16:19:44 UTC
Kyle, maybe a new bug report?
Comment 61 Olli-Antti Kivilahti 2015-03-12 13:54:25 UTC
Just wanted to say that good job with this one Kyle!
I just subclassed a subclass of one of your Koha::Objects :)
Works like a charm!
I updated the wiki page for that regard.