We have columns.def that contains the descriptions for DB columns and some json files that are used for syspref's values selection. 1. We duplicate them 2. columns.def is translatable but json files are not 3. columns.def is a XML file we are parsing with regexs We should have a single and better way to deal with those descriptions.
Created attachment 128522 [details] [review] Bug 29695: Remove GetColumnDefs C4::Templates::GetColumnDefs can easily be replaced using the new Koha::Database::Columns module. Test plan: Go to the import patron tool and confirm that you see the same list of patron's attribute on the import patron form.
Created attachment 128523 [details] [review] Bug 29695: Remove C4::Reports::Guided::_get_column_defs This subroutine was mostly the same as GetColumnDefs, we replace it identically as in the previous patch. Test plan: Translate some strings in another language % gulp po:update --lang es-ES % cd misc/translate # Translate the relevant strings in po/es-ES-messages.po # For instance "Alternate contact: Surname" % perl translate install es-ES Select the language for the interface (enable it in the 'language' syspref first) Create a new guided report and confirm that the columns for the borrowers table are translated
Created attachment 128524 [details] [review] Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs Same as the previous patch but for the inventory. Test plan: Same as before but test the inventory tool
Created attachment 128525 [details] [review] Bug 29695: Make "modalselect" syspref's values translatable We are using json files to retrieve the list of the DB columns. We can reuse what we have done in the previous patch and display translated strings. Test plan: Search for "unwanted" in the sysprefs
Created attachment 128526 [details] [review] Bug 29695: Remove json files We do no longer need them. Test plan: Confirm that their content are in Koha::Database::Columns
Created attachment 128527 [details] [review] Bug 29695: Remove columns.def No longer in use Test plan: % git grep 'columns.def' does not return relevant occurrences
Created attachment 128528 [details] [review] Bug 29695: Add new module Koha::Database::Columns This module will contain new strings and make them translatable using Koha::I18N. We will need to improve this module to prevent the dozens of __ calls each time we retrieve the column list. We could either use memcached or add a method to only retrieve columns for a given table.
Created attachment 128529 [details] [review] Bug 29695: Remove columns.def No longer in use Test plan: % git grep 'columns.def' does not return relevant occurrences
Note that we could reuse this to remove the different includes/csv_headers files. We are generating the CSVs using TT to make the headers translatable, but it would be better to generate them using Text::CSV.
Created attachment 128559 [details] [review] Bug 29695: Add new module Koha::Database::Columns This module will contain new strings and make them translatable using Koha::I18N. We will need to improve this module to prevent the dozens of __ calls each time we retrieve the column list. We could either use memcached or add a method to only retrieve columns for a given table.
Created attachment 128560 [details] [review] Bug 29695: Remove GetColumnDefs C4::Templates::GetColumnDefs can easily be replaced using the new Koha::Database::Columns module. Test plan: Go to the import patron tool and confirm that you see the same list of patron's attribute on the import patron form.
Created attachment 128561 [details] [review] Bug 29695: Remove C4::Reports::Guided::_get_column_defs This subroutine was mostly the same as GetColumnDefs, we replace it identically as in the previous patch. Test plan: Translate some strings in another language % gulp po:update --lang es-ES % cd misc/translate # Translate the relevant strings in po/es-ES-messages.po # For instance "Alternate contact: Surname" % perl translate install es-ES Select the language for the interface (enable it in the 'language' syspref first) Create a new guided report and confirm that the columns for the borrowers table are translated
Created attachment 128562 [details] [review] Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs Same as the previous patch but for the inventory. Test plan: Same as before but test the inventory tool
Created attachment 128563 [details] [review] Bug 29695: Make "modalselect" syspref's values translatable We are using json files to retrieve the list of the DB columns. We can reuse what we have done in the previous patch and display translated strings. Test plan: Search for "unwanted" in the sysprefs
Created attachment 128564 [details] [review] Bug 29695: Remove json files We do no longer need them. Test plan: Confirm that their content are in Koha::Database::Columns
Created attachment 128565 [details] [review] Bug 29695: Remove columns.def No longer in use Test plan: % git grep 'columns.def' does not return relevant occurrences
Created attachment 130065 [details] [review] Bug 29695: Add new module Koha::Database::Columns This module will contain new strings and make them translatable using Koha::I18N. We will need to improve this module to prevent the dozens of __ calls each time we retrieve the column list. We could either use memcached or add a method to only retrieve columns for a given table.
Created attachment 130066 [details] [review] Bug 29695: Remove GetColumnDefs C4::Templates::GetColumnDefs can easily be replaced using the new Koha::Database::Columns module. Test plan: Go to the import patron tool and confirm that you see the same list of patron's attribute on the import patron form.
Created attachment 130067 [details] [review] Bug 29695: Remove C4::Reports::Guided::_get_column_defs This subroutine was mostly the same as GetColumnDefs, we replace it identically as in the previous patch. Test plan: Translate some strings in another language % gulp po:update --lang es-ES % cd misc/translate # Translate the relevant strings in po/es-ES-messages.po # For instance "Alternate contact: Surname" % perl translate install es-ES Select the language for the interface (enable it in the 'language' syspref first) Create a new guided report and confirm that the columns for the borrowers table are translated
Created attachment 130068 [details] [review] Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs Same as the previous patch but for the inventory. Test plan: Same as before but test the inventory tool
Created attachment 130069 [details] [review] Bug 29695: Make "modalselect" syspref's values translatable We are using json files to retrieve the list of the DB columns. We can reuse what we have done in the previous patch and display translated strings. Test plan: Search for "unwanted" in the sysprefs
Created attachment 130070 [details] [review] Bug 29695: Remove json files We do no longer need them. Test plan: Confirm that their content are in Koha::Database::Columns
Created attachment 130071 [details] [review] Bug 29695: Remove columns.def No longer in use Test plan: % git grep 'columns.def' does not return relevant occurrences
I think this change is great, could you have a look?
Created attachment 130107 [details] [review] Bug 29695: Add new module Koha::Database::Columns This module will contain new strings and make them translatable using Koha::I18N. We will need to improve this module to prevent the dozens of __ calls each time we retrieve the column list. We could either use memcached or add a method to only retrieve columns for a given table. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 130108 [details] [review] Bug 29695: Remove GetColumnDefs C4::Templates::GetColumnDefs can easily be replaced using the new Koha::Database::Columns module. Test plan: Go to the import patron tool and confirm that you see the same list of patron's attribute on the import patron form. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 130109 [details] [review] Bug 29695: Remove C4::Reports::Guided::_get_column_defs This subroutine was mostly the same as GetColumnDefs, we replace it identically as in the previous patch. Test plan: Translate some strings in another language % gulp po:update --lang es-ES % cd misc/translate # Translate the relevant strings in po/es-ES-messages.po # For instance "Alternate contact: Surname" % perl translate install es-ES Select the language for the interface (enable it in the 'language' syspref first) Create a new guided report and confirm that the columns for the borrowers table are translated Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 130110 [details] [review] Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs Same as the previous patch but for the inventory. Test plan: Same as before but test the inventory tool Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 130111 [details] [review] Bug 29695: Make "modalselect" syspref's values translatable We are using json files to retrieve the list of the DB columns. We can reuse what we have done in the previous patch and display translated strings. Test plan: Search for "unwanted" in the sysprefs Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 130112 [details] [review] Bug 29695: Remove json files We do no longer need them. Test plan: Confirm that their content are in Koha::Database::Columns Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 130113 [details] [review] Bug 29695: Remove columns.def No longer in use Test plan: % git grep 'columns.def' does not return relevant occurrences Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 131918 [details] [review] Bug 29695: Add new module Koha::Database::Columns This module will contain new strings and make them translatable using Koha::I18N. We will need to improve this module to prevent the dozens of __ calls each time we retrieve the column list. We could either use memcached or add a method to only retrieve columns for a given table. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131919 [details] [review] Bug 29695: Remove GetColumnDefs C4::Templates::GetColumnDefs can easily be replaced using the new Koha::Database::Columns module. Test plan: Go to the import patron tool and confirm that you see the same list of patron's attribute on the import patron form. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131920 [details] [review] Bug 29695: Remove C4::Reports::Guided::_get_column_defs This subroutine was mostly the same as GetColumnDefs, we replace it identically as in the previous patch. Test plan: Translate some strings in another language % gulp po:update --lang es-ES % cd misc/translate # Translate the relevant strings in po/es-ES-messages.po # For instance "Alternate contact: Surname" % perl translate install es-ES Select the language for the interface (enable it in the 'language' syspref first) Create a new guided report and confirm that the columns for the borrowers table are translated Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131921 [details] [review] Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs Same as the previous patch but for the inventory. Test plan: Same as before but test the inventory tool Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131922 [details] [review] Bug 29695: Make "modalselect" syspref's values translatable We are using json files to retrieve the list of the DB columns. We can reuse what we have done in the previous patch and display translated strings. Test plan: Search for "unwanted" in the sysprefs Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131923 [details] [review] Bug 29695: Remove json files We do no longer need them. Test plan: Confirm that their content are in Koha::Database::Columns Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131924 [details] [review] Bug 29695: Remove columns.def No longer in use Test plan: % git grep 'columns.def' does not return relevant occurrences Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131925 [details] [review] Bug 29695: Add missing POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ok, it works. Tests pass. The question is: is this what we want? We are basically moving column descriptions handling into a better place (i.e. a Koha class that is more normal that what we had before), we are getting rid of a mix XML (columns.def) or json where those descriptions lived. But on doing so we are: - Moving translatable strings to Koha::* - Adding some overhead by the way we use __. It feels like overall is ok, but the descriptions should live in an external per-language file that is translated 'the usual way' and loaded only once when the class is instantiated (class?, it should be a proper class, so each process that instantiates it can initialize it with the current language).
(In reply to Tomás Cohen Arazi from comment #40) > Ok, it works. Tests pass. > > The question is: is this what we want? > > We are basically moving column descriptions handling into a better place > (i.e. a Koha class that is more normal that what we had before), we are > getting rid of a mix XML (columns.def) or json where those descriptions > lived. > > But on doing so we are: > - Moving translatable strings to Koha::* > - Adding some overhead by the way we use __. > > It feels like overall is ok, but the descriptions should live in an external > per-language file that is translated 'the usual way' and loaded only once > when the class is instantiated (class?, it should be a proper class, so each > process that instantiates it can initialize it with the current language). Why is it bad to use __()? It will get translated the usual way by translators.
(In reply to Jonathan Druart from comment #41) > Why is it bad to use __()? > It will get translated the usual way by translators. I might be wrong, but have the feeling that any place in which all (say) borrower table columns need to be displayed, will imply N file accesses if there's no caching of the file. I tested it to work correctly and didn't notice any drawbacks, but got me thinking of the approach.
(In reply to Tomás Cohen Arazi from comment #42) > (In reply to Jonathan Druart from comment #41) > > Why is it bad to use __()? > > It will get translated the usual way by translators. > > I might be wrong, but have the feeling that any place in which all (say) > borrower table columns need to be displayed, will imply N file accesses if > there's no caching of the file. You are wrong. Translations are cached, see Koha::I18N->init > I tested it to work correctly and didn't notice any drawbacks, but got me > thinking of the approach. This is the correct way, we need to let perl code access the translation of columns. Think CSV headers for instance, now we have to use TT for that, and build the CSV manually when we could use a dedicated library (Text::CSV_XS in our case). See also the code for reports to see how easy it is now.
(In reply to Jonathan Druart from comment #43) > (In reply to Tomás Cohen Arazi from comment #42) > > (In reply to Jonathan Druart from comment #41) > > > Why is it bad to use __()? > > > It will get translated the usual way by translators. > > > > I might be wrong, but have the feeling that any place in which all (say) > > borrower table columns need to be displayed, will imply N file accesses if > > there's no caching of the file. > > You are wrong. Translations are cached, see Koha::I18N->init Thanks for explaining! > > I tested it to work correctly and didn't notice any drawbacks, but got me > > thinking of the approach. > > This is the correct way, we need to let perl code access the translation of > columns. Think CSV headers for instance, now we have to use TT for that, and > build the CSV manually when we could use a dedicated library (Text::CSV_XS > in our case). See also the code for reports to see how easy it is now. Gotcha. This is ready then :-D
Pushed to master for 22.05, thanks to everybody involved [U+1F984]