Bug 17835 - Move C4::Koha::GetItemTypes to Koha::ItemTypes
Summary: Move C4::Koha::GetItemTypes to Koha::ItemTypes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 18464 (view as bug list)
Depends on: 17627
Blocks: 22882 15779 17843 19671 22883
  Show dependency treegraph
 
Reported: 2017-01-03 11:04 UTC by Jonathan Druart
Modified: 2019-05-10 09:53 UTC (History)
7 users (show)

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


Attachments
Bug 17835: Create a ItemtypeLocalization view (7.10 KB, patch)
2017-01-03 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Do not reselect translated_description if comes from search_with_localization (1.47 KB, patch)
2017-01-03 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Replace GetItemTypes with Koha::ItemTypes (48.63 KB, patch)
2017-01-03 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Remove the subroutine GetItemTypes (4.90 KB, patch)
2017-01-03 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class (4.78 KB, patch)
2017-01-03 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: follow-up (1.77 KB, patch)
2017-01-04 18:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class (5.47 KB, patch)
2017-02-08 17:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Mock language pref value (1.18 KB, patch)
2017-02-09 08:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Create a ItemtypeLocalization view (7.15 KB, patch)
2017-02-09 10:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17835: Do not reselect translated_description if comes from search_with_localization (1.52 KB, patch)
2017-02-09 10:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17835: Replace GetItemTypes with Koha::ItemTypes (48.69 KB, patch)
2017-02-09 10:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17835: Remove the subroutine GetItemTypes (4.95 KB, patch)
2017-02-09 10:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class (5.53 KB, patch)
2017-02-09 10:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17835: Mock language pref value (1.23 KB, patch)
2017-02-09 10:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17835: Replace GetItemTypes with Koha::ItemTypes (48.70 KB, patch)
2017-03-08 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Remove the subroutine GetItemTypes (4.95 KB, patch)
2017-03-08 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class (5.53 KB, patch)
2017-03-08 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Mock language pref value (1.23 KB, patch)
2017-03-08 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Create a ItemtypeLocalization view (7.15 KB, patch)
2017-03-29 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Do not reselect translated_description if comes from search_with_localization (1.52 KB, patch)
2017-03-29 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Replace GetItemTypes with Koha::ItemTypes (48.67 KB, patch)
2017-03-29 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Remove the subroutine GetItemTypes (4.95 KB, patch)
2017-03-29 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class (5.53 KB, patch)
2017-03-29 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Mock language pref value (1.23 KB, patch)
2017-03-29 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17835: Create a ItemtypeLocalization view (7.20 KB, patch)
2017-04-07 10:07 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17835: Do not reselect translated_description if comes from search_with_localization (1.57 KB, patch)
2017-04-07 10:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17835: Replace GetItemTypes with Koha::ItemTypes (48.74 KB, patch)
2017-04-07 10:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17835: Remove the subroutine GetItemTypes (5.00 KB, patch)
2017-04-07 10:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class (5.58 KB, patch)
2017-04-07 10:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17835: Mock language pref value (1.28 KB, patch)
2017-04-07 10:27 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17835: Create a ItemtypeLocalization view (7.30 KB, patch)
2017-04-14 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17835: Do not reselect translated_description if comes from search_with_localization (1.67 KB, patch)
2017-04-14 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17835: Replace GetItemTypes with Koha::ItemTypes (48.80 KB, patch)
2017-04-14 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17835: Remove the subroutine GetItemTypes (5.10 KB, patch)
2017-04-14 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class (5.67 KB, patch)
2017-04-14 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17835: Mock language pref value (1.38 KB, patch)
2017-04-14 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17835: [QA Follow-up] Fix opac-search.pl (1.28 KB, patch)
2017-04-14 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17835: (followup) Make TestBuilder skip views (941 bytes, patch)
2017-04-19 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17835: Re-add the use Koha in orderreceive (693 bytes, patch)
2017-04-20 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-01-03 11:04:00 UTC

    
Comment 1 Jonathan Druart 2017-01-03 12:41:36 UTC
Created attachment 58561 [details] [review]
Bug 17835: Create a ItemtypeLocalization view

To properly move C4::Koha::GetItemTypes to Koha::ItemTypes we need
DBIx::Class to make a join on the localization table to retrieve the
possible translated description of the item types.
To do so there are 2 possibilities. The first one would have been to
rename the localization table to something like itemtype_localization.
That way we could have had a relationship between
itemtype_localization.code and itemtypes.itemtype
That would have meant to create one table per "entity" (here an entity
is itemtype) we allow the translability. There are pros and cons for
this choice, so I opt for another solution.
The other solution is to create a view on top of this localization
table. With this new view we can define the missing relationship.

That sounds like a quite clean solution and easy to implement.
Once we have this relationship, the
Koha::ItemTypes->search_with_localization will join on this view an
return the same result as GetItemTypes( style => 'array' ).
To replace
    GetItemtypes( style => 'hash' )
which is the default behavior of this subroutine, we can do something like:
    my $itemtypes = Koha::ItemTypes->search_with_localization;
    my %itemtypes = map { $_->{itemtype} => $_ } @{ $itemtypes->unblessed };

This patchset must not introduce big changes but it changes certain
behaviors (which were wrong) in some scripts. Indeed sometimes the
descriptions of the item types were not the translated ones. Moreover it
happens that the item types displayed in a dropdown list were not
ordered by translated description, but by description of code
(itemtypes.itemtype value). These 2 behaviors are what we expect.

Test plan:
Bugs will be hard to catch since these patches change a lot of file, it
will be easier to read the diff and catch possible typos or logic
errors.
However signoffers can focus on modified files and the item types
values.
Comment 2 Jonathan Druart 2017-01-03 12:41:39 UTC
Created attachment 58562 [details] [review]
Bug 17835: Do not reselect translated_description if comes from search_with_localization

If the Koha::ItemType object has been instanciated from a call to
Koha::ItemTypes->search_with_localization, we already have the
translated_description value. So there is no need to fetch it again from
the DB. This is what this trick does: if the translated_description
column exist in the DBIx::Class result source's column list, that means
the value has already been retrieved.
Comment 3 Jonathan Druart 2017-01-03 12:41:43 UTC
Created attachment 58563 [details] [review]
Bug 17835: Replace GetItemTypes with Koha::ItemTypes
Comment 4 Jonathan Druart 2017-01-03 12:41:48 UTC
Created attachment 58564 [details] [review]
Bug 17835: Remove the subroutine GetItemTypes

At this point the subroutine is no longer in used.
Comment 5 Jonathan Druart 2017-01-03 12:41:51 UTC
Created attachment 58565 [details] [review]
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class

The previous query was wrong. If an item type did not contain the
translation in the interface's language, the ->search_with_localization
did not return it at all.

What we need is definitely to add a second condition on the join.

For reference:
http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Relationship/Base.pm#condition
https://blog.afoolishmanifesto.com/posts/dbix-class-parameterized-relationships/

That sounds hacky but seems to be the DBIx::Class path to follow.
Comment 6 Owen Leonard 2017-01-04 17:54:16 UTC
I tested this by looking at all the pages modified and making sure the item type information or form fields displayed correctly on each. I found two problems:

- No item types display in the dropdown on reports/reserves_stats.pl
- Error on suggestion/suggestion.pl: "Undefined subroutine &main::GetAuthorisedValues"

Everywhere else I could see no problems.
Comment 7 Jonathan Druart 2017-01-04 18:18:40 UTC
Created attachment 58600 [details] [review]
Bug 17835: follow-up
Comment 8 Josef Moravec 2017-02-08 16:24:06 UTC
test t/db_dependent/Koha/ItemTypes.t is failing:
#   Failed test 'item types should be sorted by translated description'
#   at t/db_dependent/Koha/ItemTypes.t line 120.
#          got: 'description'
#     expected: 'a translated itemtype desc'
# Looks like you failed 1 test of 20.

and in the fifth patch in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt line 1202 is typo I think: extra "XX" on the ond of line
Comment 9 Josef Moravec 2017-02-08 16:24:39 UTC
but otherwise looks good for me ;) good job
Comment 10 Jonathan Druart 2017-02-08 17:14:03 UTC
Created attachment 60033 [details] [review]
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class

The previous query was wrong. If an item type did not contain the
translation in the interface's language, the ->search_with_localization
did not return it at all.

What we need is definitely to add a second condition on the join.

For reference:
http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Relationship/Base.pm#condition
https://blog.afoolishmanifesto.com/posts/dbix-class-parameterized-relationships/

That sounds hacky but seems to be the DBIx::Class path to follow.

Bug 17835: follow-up
Comment 11 Jonathan Druart 2017-02-08 17:15:00 UTC
(In reply to Josef Moravec from comment #8)
> test t/db_dependent/Koha/ItemTypes.t is failing:
> #   Failed test 'item types should be sorted by translated description'
> #   at t/db_dependent/Koha/ItemTypes.t line 120.
> #          got: 'description'
> #     expected: 'a translated itemtype desc'
> # Looks like you failed 1 test of 20.

Weird, I not recreate, it passes for me...

> and in the fifth patch in
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt line 1202 is typo I
> think: extra "XX" on the ond of line

Yes indeed, fixed in the last patch (and I have squashed the last two ones).
Comment 12 Josef Moravec 2017-02-09 06:50:01 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Josef Moravec from comment #8)
> > test t/db_dependent/Koha/ItemTypes.t is failing:
> > #   Failed test 'item types should be sorted by translated description'
> > #   at t/db_dependent/Koha/ItemTypes.t line 120.
> > #          got: 'description'
> > #     expected: 'a translated itemtype desc'
> > # Looks like you failed 1 test of 20.
> 
> Weird, I not recreate, it passes for me...
> 

I figured it out, the translation is done for "en" language, but when subroutine C4::Languages::getlanguage returns something else (it could happen - I have installed two additional languages for now), the original description is selected and test fails.
Comment 13 Jonathan Druart 2017-02-09 08:16:12 UTC
Created attachment 60046 [details] [review]
Bug 17835: Mock language pref value

That way if prefs contain other languages, the test will still pass.
Comment 14 Jonathan Druart 2017-02-09 08:16:51 UTC
It failed if opaclanguages contained something else than just 'en'.
Comment 15 Josef Moravec 2017-02-09 10:28:26 UTC
Created attachment 60048 [details] [review]
Bug 17835: Create a ItemtypeLocalization view

To properly move C4::Koha::GetItemTypes to Koha::ItemTypes we need
DBIx::Class to make a join on the localization table to retrieve the
possible translated description of the item types.
To do so there are 2 possibilities. The first one would have been to
rename the localization table to something like itemtype_localization.
That way we could have had a relationship between
itemtype_localization.code and itemtypes.itemtype
That would have meant to create one table per "entity" (here an entity
is itemtype) we allow the translability. There are pros and cons for
this choice, so I opt for another solution.
The other solution is to create a view on top of this localization
table. With this new view we can define the missing relationship.

That sounds like a quite clean solution and easy to implement.
Once we have this relationship, the
Koha::ItemTypes->search_with_localization will join on this view an
return the same result as GetItemTypes( style => 'array' ).
To replace
    GetItemtypes( style => 'hash' )
which is the default behavior of this subroutine, we can do something like:
    my $itemtypes = Koha::ItemTypes->search_with_localization;
    my %itemtypes = map { $_->{itemtype} => $_ } @{ $itemtypes->unblessed };

This patchset must not introduce big changes but it changes certain
behaviors (which were wrong) in some scripts. Indeed sometimes the
descriptions of the item types were not the translated ones. Moreover it
happens that the item types displayed in a dropdown list were not
ordered by translated description, but by description of code
(itemtypes.itemtype value). These 2 behaviors are what we expect.

Test plan:
Bugs will be hard to catch since these patches change a lot of file, it
will be easier to read the diff and catch possible typos or logic
errors.
However signoffers can focus on modified files and the item types
values.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Josef Moravec 2017-02-09 10:28:33 UTC
Created attachment 60049 [details] [review]
Bug 17835: Do not reselect translated_description if comes from search_with_localization

If the Koha::ItemType object has been instanciated from a call to
Koha::ItemTypes->search_with_localization, we already have the
translated_description value. So there is no need to fetch it again from
the DB. This is what this trick does: if the translated_description
column exist in the DBIx::Class result source's column list, that means
the value has already been retrieved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Josef Moravec 2017-02-09 10:28:45 UTC
Created attachment 60050 [details] [review]
Bug 17835: Replace GetItemTypes with Koha::ItemTypes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Josef Moravec 2017-02-09 10:28:55 UTC
Created attachment 60051 [details] [review]
Bug 17835: Remove the subroutine GetItemTypes

At this point the subroutine is no longer in used.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Josef Moravec 2017-02-09 10:29:02 UTC
Created attachment 60052 [details] [review]
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class

The previous query was wrong. If an item type did not contain the
translation in the interface's language, the ->search_with_localization
did not return it at all.

What we need is definitely to add a second condition on the join.

For reference:
http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Relationship/Base.pm#condition
https://blog.afoolishmanifesto.com/posts/dbix-class-parameterized-relationships/

That sounds hacky but seems to be the DBIx::Class path to follow.

Bug 17835: follow-up

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Josef Moravec 2017-02-09 10:29:09 UTC
Created attachment 60053 [details] [review]
Bug 17835: Mock language pref value

That way if prefs contain other languages, the test will still pass.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Jonathan Druart 2017-03-08 13:58:40 UTC
Created attachment 60914 [details] [review]
Bug 17835: Replace GetItemTypes with Koha::ItemTypes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 22 Jonathan Druart 2017-03-08 13:58:45 UTC
Created attachment 60915 [details] [review]
Bug 17835: Remove the subroutine GetItemTypes

At this point the subroutine is no longer in used.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 23 Jonathan Druart 2017-03-08 13:58:50 UTC
Created attachment 60916 [details] [review]
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class

The previous query was wrong. If an item type did not contain the
translation in the interface's language, the ->search_with_localization
did not return it at all.

What we need is definitely to add a second condition on the join.

For reference:
http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Relationship/Base.pm#condition
https://blog.afoolishmanifesto.com/posts/dbix-class-parameterized-relationships/

That sounds hacky but seems to be the DBIx::Class path to follow.

Bug 17835: follow-up

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 24 Jonathan Druart 2017-03-08 13:58:53 UTC
Created attachment 60917 [details] [review]
Bug 17835: Mock language pref value

That way if prefs contain other languages, the test will still pass.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 25 Tomás Cohen Arazi 2017-03-29 18:16:51 UTC
Patches don't apply cleanly
Comment 26 Jonathan Druart 2017-03-29 18:21:30 UTC
Created attachment 61695 [details] [review]
Bug 17835: Create a ItemtypeLocalization view

To properly move C4::Koha::GetItemTypes to Koha::ItemTypes we need
DBIx::Class to make a join on the localization table to retrieve the
possible translated description of the item types.
To do so there are 2 possibilities. The first one would have been to
rename the localization table to something like itemtype_localization.
That way we could have had a relationship between
itemtype_localization.code and itemtypes.itemtype
That would have meant to create one table per "entity" (here an entity
is itemtype) we allow the translability. There are pros and cons for
this choice, so I opt for another solution.
The other solution is to create a view on top of this localization
table. With this new view we can define the missing relationship.

That sounds like a quite clean solution and easy to implement.
Once we have this relationship, the
Koha::ItemTypes->search_with_localization will join on this view an
return the same result as GetItemTypes( style => 'array' ).
To replace
    GetItemtypes( style => 'hash' )
which is the default behavior of this subroutine, we can do something like:
    my $itemtypes = Koha::ItemTypes->search_with_localization;
    my %itemtypes = map { $_->{itemtype} => $_ } @{ $itemtypes->unblessed };

This patchset must not introduce big changes but it changes certain
behaviors (which were wrong) in some scripts. Indeed sometimes the
descriptions of the item types were not the translated ones. Moreover it
happens that the item types displayed in a dropdown list were not
ordered by translated description, but by description of code
(itemtypes.itemtype value). These 2 behaviors are what we expect.

Test plan:
Bugs will be hard to catch since these patches change a lot of file, it
will be easier to read the diff and catch possible typos or logic
errors.
However signoffers can focus on modified files and the item types
values.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 27 Jonathan Druart 2017-03-29 18:21:34 UTC
Created attachment 61696 [details] [review]
Bug 17835: Do not reselect translated_description if comes from search_with_localization

If the Koha::ItemType object has been instanciated from a call to
Koha::ItemTypes->search_with_localization, we already have the
translated_description value. So there is no need to fetch it again from
the DB. This is what this trick does: if the translated_description
column exist in the DBIx::Class result source's column list, that means
the value has already been retrieved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Jonathan Druart 2017-03-29 18:21:39 UTC
Created attachment 61697 [details] [review]
Bug 17835: Replace GetItemTypes with Koha::ItemTypes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Jonathan Druart 2017-03-29 18:21:43 UTC
Created attachment 61698 [details] [review]
Bug 17835: Remove the subroutine GetItemTypes

At this point the subroutine is no longer in used.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 30 Jonathan Druart 2017-03-29 18:21:47 UTC
Created attachment 61699 [details] [review]
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class

The previous query was wrong. If an item type did not contain the
translation in the interface's language, the ->search_with_localization
did not return it at all.

What we need is definitely to add a second condition on the join.

For reference:
http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Relationship/Base.pm#condition
https://blog.afoolishmanifesto.com/posts/dbix-class-parameterized-relationships/

That sounds hacky but seems to be the DBIx::Class path to follow.

Bug 17835: follow-up

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 31 Jonathan Druart 2017-03-29 18:21:51 UTC
Created attachment 61700 [details] [review]
Bug 17835: Mock language pref value

That way if prefs contain other languages, the test will still pass.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 32 Marcel de Rooy 2017-04-07 09:31:39 UTC
Just a suggestion: If you rebase this one next Thursday, I will resume next Friday here?
Comment 33 Lari Taskula 2017-04-07 10:07:49 UTC
Created attachment 61966 [details] [review]
Bug 17835: Create a ItemtypeLocalization view

To properly move C4::Koha::GetItemTypes to Koha::ItemTypes we need
DBIx::Class to make a join on the localization table to retrieve the
possible translated description of the item types.
To do so there are 2 possibilities. The first one would have been to
rename the localization table to something like itemtype_localization.
That way we could have had a relationship between
itemtype_localization.code and itemtypes.itemtype
That would have meant to create one table per "entity" (here an entity
is itemtype) we allow the translability. There are pros and cons for
this choice, so I opt for another solution.
The other solution is to create a view on top of this localization
table. With this new view we can define the missing relationship.

That sounds like a quite clean solution and easy to implement.
Once we have this relationship, the
Koha::ItemTypes->search_with_localization will join on this view an
return the same result as GetItemTypes( style => 'array' ).
To replace
    GetItemtypes( style => 'hash' )
which is the default behavior of this subroutine, we can do something like:
    my $itemtypes = Koha::ItemTypes->search_with_localization;
    my %itemtypes = map { $_->{itemtype} => $_ } @{ $itemtypes->unblessed };

This patchset must not introduce big changes but it changes certain
behaviors (which were wrong) in some scripts. Indeed sometimes the
descriptions of the item types were not the translated ones. Moreover it
happens that the item types displayed in a dropdown list were not
ordered by translated description, but by description of code
(itemtypes.itemtype value). These 2 behaviors are what we expect.

Test plan:
Bugs will be hard to catch since these patches change a lot of file, it
will be easier to read the diff and catch possible typos or logic
errors.
However signoffers can focus on modified files and the item types
values.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 34 Lari Taskula 2017-04-07 10:08:10 UTC
Created attachment 61967 [details] [review]
Bug 17835: Do not reselect translated_description if comes from search_with_localization

If the Koha::ItemType object has been instanciated from a call to
Koha::ItemTypes->search_with_localization, we already have the
translated_description value. So there is no need to fetch it again from
the DB. This is what this trick does: if the translated_description
column exist in the DBIx::Class result source's column list, that means
the value has already been retrieved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 35 Lari Taskula 2017-04-07 10:08:30 UTC
Created attachment 61968 [details] [review]
Bug 17835: Replace GetItemTypes with Koha::ItemTypes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 36 Lari Taskula 2017-04-07 10:08:42 UTC
Created attachment 61969 [details] [review]
Bug 17835: Remove the subroutine GetItemTypes

At this point the subroutine is no longer in used.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 37 Lari Taskula 2017-04-07 10:08:53 UTC
Created attachment 61970 [details] [review]
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class

The previous query was wrong. If an item type did not contain the
translation in the interface's language, the ->search_with_localization
did not return it at all.

What we need is definitely to add a second condition on the join.

For reference:
http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Relationship/Base.pm#condition
https://blog.afoolishmanifesto.com/posts/dbix-class-parameterized-relationships/

That sounds hacky but seems to be the DBIx::Class path to follow.

Bug 17835: follow-up

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 38 Lari Taskula 2017-04-07 10:26:23 UTC
Rebased on top of master (fixed 3 "use" conflicts, 1 per file in catalogue/detail.pl, opac/opac-detail.pl, opac/opac-user.pl). Switching back to Signed Off.
Comment 39 Lari Taskula 2017-04-07 10:27:30 UTC
Created attachment 61971 [details] [review]
Bug 17835: Mock language pref value

That way if prefs contain other languages, the test will still pass.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 40 Marcel de Rooy 2017-04-14 06:57:04 UTC
QA: Working on this one now.
Comment 41 Marcel de Rooy 2017-04-14 08:49:14 UTC
Created attachment 62156 [details] [review]
Bug 17835: Create a ItemtypeLocalization view

To properly move C4::Koha::GetItemTypes to Koha::ItemTypes we need
DBIx::Class to make a join on the localization table to retrieve the
possible translated description of the item types.
To do so there are 2 possibilities. The first one would have been to
rename the localization table to something like itemtype_localization.
That way we could have had a relationship between
itemtype_localization.code and itemtypes.itemtype
That would have meant to create one table per "entity" (here an entity
is itemtype) we allow the translability. There are pros and cons for
this choice, so I opt for another solution.
The other solution is to create a view on top of this localization
table. With this new view we can define the missing relationship.

That sounds like a quite clean solution and easy to implement.
Once we have this relationship, the
Koha::ItemTypes->search_with_localization will join on this view an
return the same result as GetItemTypes( style => 'array' ).
To replace
    GetItemtypes( style => 'hash' )
which is the default behavior of this subroutine, we can do something like:
    my $itemtypes = Koha::ItemTypes->search_with_localization;
    my %itemtypes = map { $_->{itemtype} => $_ } @{ $itemtypes->unblessed };

This patchset must not introduce big changes but it changes certain
behaviors (which were wrong) in some scripts. Indeed sometimes the
descriptions of the item types were not the translated ones. Moreover it
happens that the item types displayed in a dropdown list were not
ordered by translated description, but by description of code
(itemtypes.itemtype value). These 2 behaviors are what we expect.

Test plan:
Bugs will be hard to catch since these patches change a lot of file, it
will be easier to read the diff and catch possible typos or logic
errors.
However signoffers can focus on modified files and the item types
values.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2017-04-14 08:49:18 UTC
Created attachment 62157 [details] [review]
Bug 17835: Do not reselect translated_description if comes from search_with_localization

If the Koha::ItemType object has been instanciated from a call to
Koha::ItemTypes->search_with_localization, we already have the
translated_description value. So there is no need to fetch it again from
the DB. This is what this trick does: if the translated_description
column exist in the DBIx::Class result source's column list, that means
the value has already been retrieved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2017-04-14 08:49:23 UTC
Created attachment 62158 [details] [review]
Bug 17835: Replace GetItemTypes with Koha::ItemTypes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2017-04-14 08:49:28 UTC
Created attachment 62159 [details] [review]
Bug 17835: Remove the subroutine GetItemTypes

At this point the subroutine is no longer in used.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2017-04-14 08:49:32 UTC
Created attachment 62160 [details] [review]
Bug 17835: Add an additional LEFT JOIN condition using DBIx::Class

The previous query was wrong. If an item type did not contain the
translation in the interface's language, the ->search_with_localization
did not return it at all.

What we need is definitely to add a second condition on the join.

For reference:
http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Relationship/Base.pm#condition
https://blog.afoolishmanifesto.com/posts/dbix-class-parameterized-relationships/

That sounds hacky but seems to be the DBIx::Class path to follow.

Bug 17835: follow-up

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2017-04-14 08:49:37 UTC
Created attachment 62161 [details] [review]
Bug 17835: Mock language pref value

That way if prefs contain other languages, the test will still pass.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2017-04-14 08:49:41 UTC
Created attachment 62162 [details] [review]
Bug 17835: [QA Follow-up] Fix opac-search.pl

Resolves:
Global symbol "$itemtypes_nocategory" requires explicit package name.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested the added line with a debug statement. See itemtype facets in the
search results.
Comment 48 Marcel de Rooy 2017-04-14 08:52:10 UTC
I am not really impressed by the DBIx constructs here. Still trying if we could simplify a bit here..
Comment 49 Marcel de Rooy 2017-04-14 09:27:06 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2017-04-14 09:28:13 UTC Comment hidden (obsolete)
Comment 51 Marcel de Rooy 2017-04-14 09:56:43 UTC
QA Comment:
Passing QA on this patch set.
I am not really a fan of the resulting DBIx tric to get this working. Now each search with localization will need an additional view and an ugly relationship hack in the schema file. But I have no other solution too. Tested the sub with no, one or several languages in localization and it works perfect.

Also want to mention that the unit test t/db_dependent/Koha/ItemTypes.t is much too simplistic. Adding 2/3 English localization is not sufficient to really prove that the sub works. You cannot test localization with only one language, and English is a bad example in Koha :) We need to mock getlanguage and put a few different languages in localization to really see if it works. (I did that now manually.)
Comment 52 Kyle M Hall 2017-04-14 14:44:27 UTC
Pushed to master for 17.05, thanks Jonathan, Marcel!
Comment 53 Katrin Fischer 2017-04-16 12:49:44 UTC
This won't get ported back to 16.11.x as it is an enhancement.
Comment 54 Tomás Cohen Arazi 2017-04-19 14:20:04 UTC
Created attachment 62370 [details] [review]
Bug 17835: (followup) Make TestBuilder skip views

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 55 Jonathan Druart 2017-04-19 14:24:09 UTC
(In reply to Tomás Cohen Arazi from comment #54)
> Created attachment 62370 [details] [review] [review]
> Bug 17835: (followup) Make TestBuilder skip views
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master!
Comment 56 Jonathan Druart 2017-04-20 13:00:38 UTC
Created attachment 62470 [details] [review]
Bug 17835: Re-add the use Koha in orderreceive

It is still used for getitemtypeinfo, at least.
Comment 57 Jonathan Druart 2017-04-20 13:02:48 UTC
(In reply to Jonathan Druart from comment #56)
> Created attachment 62470 [details] [review] [review]
> Bug 17835: Re-add the use Koha in orderreceive
> 
> It is still used for getitemtypeinfo, at least.

Pushed to master!
Comment 58 Nick Clemens 2017-04-20 13:17:04 UTC
*** Bug 18464 has been marked as a duplicate of this bug. ***