Bug 29695 - Centralize columns' descriptions
Summary: Centralize columns' descriptions
Status: RESOLVED 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: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 26296 29591
Blocks: 32292 32312 32313 32314 32315
  Show dependency treegraph
 
Reported: 2021-12-14 16:04 UTC by Jonathan Druart
Modified: 2022-11-21 22:37 UTC (History)
5 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:
22.05.00


Attachments
Bug 29695: Remove GetColumnDefs (11.45 KB, patch)
2021-12-14 17:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove C4::Reports::Guided::_get_column_defs (5.02 KB, patch)
2021-12-14 17:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs (2.04 KB, patch)
2021-12-14 17:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Make "modalselect" syspref's values translatable (5.12 KB, patch)
2021-12-14 17:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove json files (5.44 KB, patch)
2021-12-14 17:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove columns.def (22.50 KB, patch)
2021-12-14 17:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Add new module Koha::Database::Columns (14.02 KB, patch)
2021-12-14 17:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove columns.def (9.85 KB, patch)
2021-12-14 17:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Add new module Koha::Database::Columns (14.09 KB, patch)
2021-12-15 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove GetColumnDefs (11.45 KB, patch)
2021-12-15 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove C4::Reports::Guided::_get_column_defs (5.02 KB, patch)
2021-12-15 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs (2.04 KB, patch)
2021-12-15 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Make "modalselect" syspref's values translatable (5.12 KB, patch)
2021-12-15 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove json files (5.49 KB, patch)
2021-12-15 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove columns.def (9.85 KB, patch)
2021-12-15 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Add new module Koha::Database::Columns (14.08 KB, patch)
2022-02-02 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove GetColumnDefs (11.45 KB, patch)
2022-02-02 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove C4::Reports::Guided::_get_column_defs (5.02 KB, patch)
2022-02-02 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs (2.04 KB, patch)
2022-02-02 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Make "modalselect" syspref's values translatable (5.12 KB, patch)
2022-02-02 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove json files (5.49 KB, patch)
2022-02-02 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Remove columns.def (9.85 KB, patch)
2022-02-02 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29695: Add new module Koha::Database::Columns (13.79 KB, patch)
2022-02-02 14:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29695: Remove GetColumnDefs (11.49 KB, patch)
2022-02-02 14:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29695: Remove C4::Reports::Guided::_get_column_defs (5.07 KB, patch)
2022-02-02 14:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs (2.09 KB, patch)
2022-02-02 14:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29695: Make "modalselect" syspref's values translatable (5.16 KB, patch)
2022-02-02 14:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29695: Remove json files (5.53 KB, patch)
2022-02-02 14:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29695: Remove columns.def (9.90 KB, patch)
2022-02-02 14:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29695: Add new module Koha::Database::Columns (13.84 KB, patch)
2022-03-18 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29695: Remove GetColumnDefs (11.55 KB, patch)
2022-03-18 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29695: Remove C4::Reports::Guided::_get_column_defs (5.12 KB, patch)
2022-03-18 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29695: (follow-up) Remove C4::Reports::Guided::_get_column_defs (2.15 KB, patch)
2022-03-18 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29695: Make "modalselect" syspref's values translatable (5.21 KB, patch)
2022-03-18 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29695: Remove json files (5.58 KB, patch)
2022-03-18 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29695: Remove columns.def (9.95 KB, patch)
2022-03-18 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29695: Add missing POD (1.36 KB, patch)
2022-03-18 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-12-14 16:04:12 UTC
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.
Comment 1 Jonathan Druart 2021-12-14 17:02:59 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2021-12-14 17:03:03 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2021-12-14 17:03:07 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2021-12-14 17:03:11 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2021-12-14 17:03:14 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2021-12-14 17:03:18 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2021-12-14 17:03:22 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2021-12-14 17:04:29 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2021-12-15 10:19:41 UTC
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.
Comment 10 Jonathan Druart 2021-12-15 12:42:15 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2021-12-15 12:42:19 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2021-12-15 12:42:23 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2021-12-15 12:42:27 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2021-12-15 12:42:31 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2021-12-15 12:42:35 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2021-12-15 12:42:39 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2022-02-02 09:47:43 UTC
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.
Comment 18 Jonathan Druart 2022-02-02 09:47:47 UTC
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.
Comment 19 Jonathan Druart 2022-02-02 09:47:51 UTC
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
Comment 20 Jonathan Druart 2022-02-02 09:47:56 UTC
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
Comment 21 Jonathan Druart 2022-02-02 09:48:00 UTC
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
Comment 22 Jonathan Druart 2022-02-02 09:48:05 UTC
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
Comment 23 Jonathan Druart 2022-02-02 09:48:10 UTC
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
Comment 24 Jonathan Druart 2022-02-02 09:48:56 UTC
I think this change is great, could you have a look?
Comment 25 Owen Leonard 2022-02-02 14:52:33 UTC
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>
Comment 26 Owen Leonard 2022-02-02 14:52:37 UTC
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>
Comment 27 Owen Leonard 2022-02-02 14:52:41 UTC
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>
Comment 28 Owen Leonard 2022-02-02 14:52:45 UTC
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>
Comment 29 Owen Leonard 2022-02-02 14:52:49 UTC
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>
Comment 30 Owen Leonard 2022-02-02 14:52:53 UTC
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>
Comment 31 Owen Leonard 2022-02-02 14:52:56 UTC
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>
Comment 32 Tomás Cohen Arazi 2022-03-18 15:15:07 UTC
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>
Comment 33 Tomás Cohen Arazi 2022-03-18 15:15:12 UTC
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>
Comment 34 Tomás Cohen Arazi 2022-03-18 15:15:18 UTC
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>
Comment 35 Tomás Cohen Arazi 2022-03-18 15:15:22 UTC
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>
Comment 36 Tomás Cohen Arazi 2022-03-18 15:15:27 UTC
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>
Comment 37 Tomás Cohen Arazi 2022-03-18 15:15:32 UTC
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>
Comment 38 Tomás Cohen Arazi 2022-03-18 15:15:37 UTC
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>
Comment 39 Tomás Cohen Arazi 2022-03-18 15:15:41 UTC
Created attachment 131925 [details] [review]
Bug 29695: Add missing POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2022-03-18 15:23:19 UTC
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).
Comment 41 Jonathan Druart 2022-03-18 16:31:47 UTC
(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.
Comment 42 Tomás Cohen Arazi 2022-03-18 18:54:48 UTC
(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.
Comment 43 Jonathan Druart 2022-03-21 10:27:59 UTC
(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.
Comment 44 Tomás Cohen Arazi 2022-04-04 17:45:25 UTC
(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
Comment 45 Fridolin Somers 2022-04-12 09:41:22 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄