Just an fyi, I this idea came to me while reading the reports for bug 9448 and bug 11463.

Not bug 9448, I meant bug 9532

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;
};

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

(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?

(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.

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.

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?

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>

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>

Created attachment 27347 [details] [review]
[PASSED QA] Bug 11518 [QA Followup]

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

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>

Pushed to master.  Thanks, Kyle!

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.

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.

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>

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?

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??

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 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).

(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?

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

(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

2 last patches moved to bug 13713.