Bug 11518 - Add new method to Koha::Schema::Result::Item that will always return the correct itemtype
Summary: Add new method to Koha::Schema::Result::Item that will always return the corr...
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: rel_3_16_candidate
Depends on:
Blocks: 11703 13713
  Show dependency treegraph
 
Reported: 2014-01-10 13:03 UTC by Kyle M Hall
Modified: 2015-12-03 22:00 UTC (History)
7 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 11518 - Add new method to Koha::Schema::Result::Item that will always return the correct itemtype (3.54 KB, patch)
2014-01-10 13:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11518 [Followup] - Change method name from 'itemtype' to 'effective_itemtype' for clarity (1.70 KB, patch)
2014-01-10 15:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11518 [Followup] - Change method name from 'itemtype' to 'effective_itemtype' for clarity (1.79 KB, patch)
2014-02-19 15:01 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 11518 [Followup] - Change method name from 'itemtype' to 'effective_itemtype' for clarity (1.79 KB, patch)
2014-02-20 15:13 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 11518 [QA Followup] (906 bytes, patch)
2014-04-10 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11518 [QA Followup] (903 bytes, patch)
2014-04-10 17:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11518 [QA Followup] (925 bytes, patch)
2014-04-10 18:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11518 [QA Followup] - Make unit tests pass (1.44 KB, patch)
2014-04-21 12:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11518 [QA Followup] - Make unit tests pass (1.42 KB, patch)
2014-04-21 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11518 - Add new method to Koha::Schema::Result::Item that will always return the correct itemtype (3.61 KB, patch)
2014-04-21 12:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11518 [Followup] - Change method name from 'itemtype' to 'effective_itemtype' for clarity (1.85 KB, patch)
2014-04-21 12:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11518 [QA Followup] (990 bytes, patch)
2014-04-21 12:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11518 [QA Followup] - Make unit tests pass (1.48 KB, patch)
2014-04-21 12:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
[NOT-PUSHED]Bug 11518: WIP - Remove useless constraints previously added (2.60 KB, patch)
2014-07-07 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11518: Follow-up removing biblioitem routines (2.67 KB, patch)
2014-08-28 10:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11518: WIP - Remove useless constraints previously added (2.75 KB, patch)
2014-08-28 10:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11518: Follow-up removing biblioitem routines (2.67 KB, patch)
2014-08-28 10:16 UTC, Marcel de Rooy
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-01-10 13:03:43 UTC
There are many disparate areas of Koha that deal with item level itemtypes vs record level itemtypes. We can take advantage of DBIx::Class to make smarter objects that automatically return the correct value depending on the system preference.
Comment 1 Kyle M Hall 2014-01-10 13:04:58 UTC
Just an fyi, I this idea came to me while reading the reports for bug 9448 and bug 11463.
Comment 2 Kyle M Hall 2014-01-10 13:05:57 UTC
Not bug 9448, I meant bug 9532
Comment 3 Kyle M Hall 2014-01-10 13:07:21 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2014-01-10 13:10:58 UTC
In the future we could even override $item->itype() to return the correct value via system preference. Not sure if this would be a good idea or a bad one. In some cases it would likely eliminate accidental bugs. From the DBIC FAQ:

How do I override a run time method (e.g. a relationship accessor)?
If you need access to the original accessor, then you must "wrap around" the original method. You can do that either with Moose::Manual::MethodModifiers or Class::Method::Modifiers. The code example works for both modules:

package Your::Schema::Group;
use Class::Method::Modifiers;

# ... declare columns ...

__PACKAGE__->has_many('group_servers', 'Your::Schema::GroupServer', 'group_id');
__PACKAGE__->many_to_many('servers', 'group_servers', 'server');

# if the server group is a "super group", then return all servers
# otherwise return only servers that belongs to the given group
around 'servers' => sub {
    my $orig = shift;
    my $self = shift;

    return $self->$orig(@_) unless $self->is_super_group;
    return $self->result_source->schema->resultset('Server')->all;
};
Comment 5 Kyle M Hall 2014-01-10 15:23:43 UTC Comment hidden (obsolete)
Comment 6 Benjamin Rokseth 2014-02-19 15:01:28 UTC Comment hidden (obsolete)
Comment 7 Benjamin Rokseth 2014-02-20 15:13:45 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2014-03-13 12:09:09 UTC
QA Comment:
I like the idea, but I think that we should not route DBIC modules back to the "ugly C4::Context module" in order to get a preference. Why not do that via DBIC instead here to keep it clean?
Note that we could get into circular module dependencies trouble quite easily when we start using from Koha to C4..

Failed QA
Comment 9 Jonathan Druart 2014-03-19 15:27:11 UTC
(In reply to M. de Rooy from comment #8)
> QA Comment:
> I like the idea, but I think that we should not route DBIC modules back to
> the "ugly C4::Context module" in order to get a preference. Why not do that
> via DBIC instead here to keep it clean?
> Note that we could get into circular module dependencies trouble quite
> easily when we start using from Koha to C4..
> 
> Failed QA

I agree, it is too bad to depend on C4::Context but it is the way to get a syspref value. Are you suggesting to get the value directly from the DB?
Comment 10 Marcel de Rooy 2014-03-20 07:32:17 UTC
(In reply to Jonathan Druart from comment #9)
> I agree, it is too bad to depend on C4::Context but it is the way to get a
> syspref value. Are you suggesting to get the value directly from the DB?

Maybe it is a better idea to move the syspref function to Koha and DBICify it.
Comment 11 Kyle M Hall 2014-04-10 17:47:04 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2014-04-10 17:48:15 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-04-10 18:16:30 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-04-11 10:51:59 UTC
I should note the unit test for this will now fail until we get the database handles for DBI and DBIx::Class tied together somehow.
Comment 15 Katrin Fischer 2014-04-19 08:25:44 UTC
I am not sure what to do with this - as the tests currently not pass, we can't push it to master like it is. It can be tested only in combination with bug 11703, which is currently in status 'FQA' - so is this now 'blocked' - 'failed' or maybe 'in discussion'? What needs to be done to make the tests pass?
Comment 16 Kyle M Hall 2014-04-21 12:21:13 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-04-21 12:22:39 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2014-04-21 12:40:14 UTC
Created attachment 27345 [details] [review]
[PASSED QA] Bug 11518 - Add new method to Koha::Schema::Result::Item that will always return the correct itemtype

There are many disparate areas of Koha that deal with item level
itemtypes vs record level itemtypes. We can take advantage of
DBIx::Class to make smarter objects that automatically return the
correct value depending on the system preference.

Test Plan:
1) Apply this patch
2) Run t/db_dependent/Items.t

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 19 Katrin Fischer 2014-04-21 12:40:17 UTC
Created attachment 27346 [details] [review]
[PASSED QA] Bug 11518 [Followup] - Change method name from 'itemtype' to 'effective_itemtype' for clarity

Test runs without errors.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 20 Katrin Fischer 2014-04-21 12:40:19 UTC
Created attachment 27347 [details] [review]
[PASSED QA] Bug 11518 [QA Followup]

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 21 Katrin Fischer 2014-04-21 12:40:21 UTC
Created attachment 27348 [details] [review]
[PASSED QA] Bug 11518 [QA Followup] - Make unit tests pass

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 22 Galen Charlton 2014-04-29 15:08:18 UTC
Pushed to master.  Thanks, Kyle!
Comment 23 Jonathan Druart 2014-07-07 12:56:25 UTC
I really don't understand the complexity of this patch.

Why do we want to add extra constraints? In my opinion, the fk constraints exist in DB and we don't want to add more.

If we keep the constraints introduced by these patch, we get a 1-n and a 1-1 relations for the same field.
I know the current structure (biblio/biblioitems/items) has a lack but I don't think it is the good way to solve it.
Comment 24 Jonathan Druart 2014-07-07 12:59:50 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2014-08-28 10:16:17 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2014-08-28 10:16:50 UTC
Created attachment 31241 [details] [review]
Bug 11518: WIP - Remove useless constraints previously added

This patch removes the 3 constraints added by patch from bug 11518.
Having 1-n + 1-1 relations for the same field is a non-sense.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added a follow-up for removing some unneeded code.
Comment 27 Marcel de Rooy 2014-08-28 10:16:55 UTC
Created attachment 31242 [details] [review]
Bug 11518: Follow-up removing biblioitem routines

There is no need to define biblioitem routines, if we can access them
via biblioitemnumber.
Note that we actually need a FK for biblio in the items table. Now we do
need the intermediate level via biblioitems in the Items.t code. Even better,
we should move biblioitem to biblio. Formally it is 1-n, but in reality it
is 1-1.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2014-08-28 10:47:13 UTC
There still is some discussion about it (see IRC):

Why do we use $self->biblioitemnumber()->itemtype() ?
$self->biblioitem->itemtype would be nicer.

My answer would be: Since the relation is called biblioitemnumber, we need to use that. Using the default relation name from DBIx is easier than adding names everywhere. Same for e.g. $biblioitem->biblionumber() instead of biblio. The generated relation is called biblionumber..

My question is: can we somehow get DBIx to define these relations with the names we like more without adding relation or code in the custom sections everywhere?
Comment 29 Marcel de Rooy 2014-08-28 10:52:19 UTC
From the DBIx doc:
For relationships, belongs_to accessors are made from column names by stripping postfixes other than _id as well, for example just Id, _?ref, _?cd, _?code and _?num, case insensitively.

If we could a postfix id and number, we would be there??
Comment 30 Katrin Fischer 2014-08-31 07:26:43 UTC
I think our ids are named quite differently across tables, so I am not sure if number and id would be enough. Also a little worried that if we change too much about the DBIC configuration, this will make it harder to use especially for new developers at some point.
Comment 31 David Cook 2014-12-01 02:21:13 UTC
Comment on attachment 27345 [details] [review]
[PASSED QA] Bug 11518 - Add new method to Koha::Schema::Result::Item that will always return the correct itemtype

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

::: Koha/Schema/Result/Biblio.pm
@@ +331,4 @@
>  # Created by DBIx::Class::Schema::Loader v0.07025 @ 2013-10-14 20:56:21
>  # DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:0//8OGf7OteNnwT03g4QsA
>  
> +__PACKAGE__->belongs_to(

I think this already exists in master...

...and that it's not a good idea. biblio.biblionumber is a primary key. If anything, biblioitems.biblio should have a belongs_to() relationship to biblio.biblionumber rather than the other way around.

Of course, I think this is here so that we can grab the associated BiblioItems object using the 'biblioitem' accessor... but it seems like a hack to me.

::: Koha/Schema/Result/Item.pm
@@ +618,4 @@
>      { "foreign.biblionumber" => "self.biblionumber" }
>  );
>  
> +__PACKAGE__->belongs_to(

I think this relationship makes sense, but if we're adding a relationship in DBIC, we should probably be adding it in the database as well.

@@ +624,5 @@
> +  { biblioitemnumber => "biblioitemnumber" },
> +);
> +
> +use C4::Context;
> +sub itemtype {

I think this makes sense (insofar as it makes sense to have an item type stored at the biblio level that is).
Comment 32 Jonathan Druart 2014-12-01 08:54:49 UTC
(In reply to David Cook from comment #31)
> Comment on attachment 27345 [details] [review] [review]

Hello David,
Did you see that the belongs_to relations are removed in another patch?
Comment 33 Mason James 2015-01-19 03:40:20 UTC
this needs to be added to 3.16.x, as 'ajax-circ' feature (11703) depends on it

i'll only attempt to pull QAed patches from this set

keyword: rel_3_16_7_candidate
Comment 34 Mason James 2015-01-19 13:07:16 UTC
(In reply to Mason James from comment #33)
> this needs to be added to 3.16.x, as 'ajax-circ' feature (11703) depends on
> it
> 
> i'll only attempt to pull QAed patches from this set
> 
> keyword: rel_3_16_7_candidate

4 patches pushed to 3.16.7
Comment 35 Jonathan Druart 2015-02-16 15:42:04 UTC
2 last patches moved to bug 13713.