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.
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.
Created attachment 58563 [details] [review] Bug 17835: Replace GetItemTypes with Koha::ItemTypes
Created attachment 58564 [details] [review] Bug 17835: Remove the subroutine GetItemTypes At this point the subroutine is no longer in used.
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.
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.
Created attachment 58600 [details] [review] Bug 17835: follow-up
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
but otherwise looks good for me ;) good job
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
(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).
(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.
Created attachment 60046 [details] [review] Bug 17835: Mock language pref value That way if prefs contain other languages, the test will still pass.
It failed if opaclanguages contained something else than just 'en'.
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>
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>
Created attachment 60050 [details] [review] Bug 17835: Replace GetItemTypes with Koha::ItemTypes Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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>
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>
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>
Created attachment 60914 [details] [review] Bug 17835: Replace GetItemTypes with Koha::ItemTypes Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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>
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>
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>
Patches don't apply cleanly
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>
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>
Created attachment 61697 [details] [review] Bug 17835: Replace GetItemTypes with Koha::ItemTypes Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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>
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>
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>
Just a suggestion: If you rebase this one next Thursday, I will resume next Friday here?
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>
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>
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>
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>
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>
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.
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>
QA: Working on this one now.
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>
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>
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>
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>
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>
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>
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.
I am not really impressed by the DBIx constructs here. Still trying if we could simplify a bit here..
Another observation: we should also translate the item type description in the items table of additem.pl. Now you have an English description in the items table and a vernacular description in the edited item.
(In reply to Marcel de Rooy from comment #49) > Another observation: we should also translate the item type description in > the items table of additem.pl. > Now you have an English description in the items table and a vernacular > description in the edited item. Well, that was too fast. Some refresh pops up the right language now. So forget this one.
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.)
Pushed to master for 17.05, thanks Jonathan, Marcel!
This won't get ported back to 16.11.x as it is an enhancement.
Created attachment 62370 [details] [review] Bug 17835: (followup) Make TestBuilder skip views Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(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!
Created attachment 62470 [details] [review] Bug 17835: Re-add the use Koha in orderreceive It is still used for getitemtypeinfo, at least.
(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!
*** Bug 18464 has been marked as a duplicate of this bug. ***