Bug 13632 - Make permission descriptions translatable
Summary: Make permission descriptions translatable
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 13633 13637 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-01-28 03:46 UTC by David Cook
Modified: 2016-06-21 21:40 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13632 - Use a translateable file instead of the database for managing permission descriptions (9.72 KB, patch)
2015-01-28 04:02 UTC, David Cook
Details | Diff | Splinter Review
Bug 13632 - Use a translateable file instead of the database for managing permission descriptions (9.82 KB, patch)
2015-01-29 01:17 UTC, David Cook
Details | Diff | Splinter Review
Bug 13632 - Use a translateable file instead of the database for managing permission descriptions (9.78 KB, patch)
2015-02-10 03:41 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13632 - Use a translateable file instead of the database for managing permission descriptions (10.10 KB, patch)
2015-02-10 12:39 UTC, Paola Rossi
Details | Diff | Splinter Review
[DO NOT PUSH] Script to generate the permissions include file (1.48 KB, patch)
2015-09-22 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13632: Use an include file to display permissions (11.12 KB, patch)
2015-09-22 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13632: Do not use userflags.flagdesc and permissions.description (3.25 KB, patch)
2015-09-22 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13632: Populate user_flags and permissions tables in the web installer (2.64 KB, patch)
2015-09-22 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13632: Remove specific files (67.15 KB, patch)
2015-09-22 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
[DO NOT PUSH] [SIGNED-OFF] Script to generate the permissions include file (1.61 KB, patch)
2015-10-16 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13632: Use an include file to display permissions (11.20 KB, patch)
2015-10-16 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13632: Do not use userflags.flagdesc and permissions.description (3.32 KB, patch)
2015-10-16 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13632: Populate user_flags and permissions tables in the web installer (2.68 KB, patch)
2015-10-16 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13632: Remove specific files (68.24 KB, patch)
2015-10-16 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Script to generate the permissions include file (1.57 KB, patch)
2015-10-18 10:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13632: Use an include file to display permissions (11.25 KB, patch)
2015-10-18 10:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13632: Do not use userflags.flagdesc and permissions.description (3.37 KB, patch)
2015-10-18 10:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13632: Populate user_flags and permissions tables in the web installer (2.77 KB, patch)
2015-10-18 10:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13632: Remove specific files (68.26 KB, patch)
2015-10-18 10:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13632: QA follow-up: Tidy installer directories / update permissions.inc (7.21 KB, patch)
2015-10-18 10:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13632: (QA followup) remove xt/permissions.t (4.90 KB, patch)
2015-10-19 15:45 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 David Cook 2015-01-28 03:46:54 UTC
At the moment, we rely on strings in `permissions.description` and `userflags.flagdesc` to provide the descriptions of the permissions on the member-flags.pl page.

It would make more sense to keep these descriptions in a file that can be translated and changed without changing the database.
Comment 1 David Cook 2015-01-28 04:02:43 UTC Comment hidden (obsolete)
Comment 2 David Cook 2015-01-28 04:40:53 UTC
Actually, as Robin has pointed out, it would probably be better to use a hash lookup instead of a giant case statement.

I suppose the way to do that would be with a TT plugin as Tomas was suggesting.

I'm open to whatever. I'm just keen to get the descriptions out of the database :)
Comment 3 David Cook 2015-01-29 01:17:35 UTC Comment hidden (obsolete)
Comment 4 David Cook 2015-01-29 01:18:58 UTC
Happy to change this patch to use some other mechanism if it's better for maintenance/translation, but hopefully this is a step forward away from storing the description strings in the database.
Comment 5 paxed 2015-01-29 08:00:17 UTC
The BLOCKs should perhaps just print out the name variable in the default case - in case a new permission is added elsewhere but not in these switches, it will then be shown to the user as-is instead of showing nothing.
Comment 6 David Cook 2015-01-29 22:40:53 UTC
(In reply to paxed from comment #5)
> The BLOCKs should perhaps just print out the name variable in the default
> case - in case a new permission is added elsewhere but not in these
> switches, it will then be shown to the user as-is instead of showing nothing.

I thought about doing that, but the name variable will already be printed out on the page, so it'll just be the same word printed out twice. I think showing nothing for the default is more striking, and hopefully will indicate that something is missing when visually scanning the page.
Comment 7 Paola Rossi 2015-02-04 11:44:24 UTC
I've applied the patch against master 3.19.00.006 head 13235.

(In reply to David Cook from comment #3)
> 4) Note that the descriptions are exactly the same as before

No, the descriptions were not exactly the same as before. 
These are the 9 differences :
now against before (i.e. master):
1) View Catalog (Librarian Interface)
<b>Required for staff login.</b> Staff access, allows viewing of catalogue in staff client.
2) Allow staff members to modify permissions for other staff members
Set user permissions
3) Edit Catalog (Modify bibliographic/holdings data)
Edit catalog (Modify bibliographic/holdings data)
4) Edit Items
Edit items
5) Upload patron images in batch or one at a time
Upload patron images in a batch or one at a time
6) Edit Authorities
Edit authorities
7) Create SQL Reports
Create SQL reports
8) <b>Required for staff login.</b> Staff access, allows viewing of catalogue in staff client.
Allow staff members to modify permissions for other staff members
9) Course Reserves
Course reserves

For now I pass the patch to "Failed QA" status.

> 
> [...] change the text in
> `permissions.description` and `userflags.flagdesc` and notice
> that the text on the page doesn't change.
> 
Yes, as required.
Comment 8 David Cook 2015-02-10 03:41:28 UTC Comment hidden (obsolete)
Comment 9 David Cook 2015-02-10 03:42:49 UTC
Thanks for testing this bug, Paola!

It turns out that the up-to-date strings from "userflags.sql" and "userpermissions.sql" weren't in my database, as the updated versions weren't included in upgrades via updatedatabase.pl!

I've remedied that now. So here's the updated English version.
Comment 10 David Cook 2015-02-10 07:23:46 UTC
Tomas:

I re-wrote my original script, which used the values in my dev database to generate the permissions.inc file, so that it would use the SQL scripts. You can find it here: https://github.com/minusdavid/koha-toolbox

However, in hindsight, you probably wanted something that would generate PO files? That would be easy enough to do as well... I just didn't think of it ;)
Comment 11 Katrin Fischer 2015-02-10 07:37:00 UTC
If the include is TT it might already be translatable, all you might have to do is add a little HTML, so the strings get parsed.

What I'd like to see is that the installer automatically picks up the permission sql file from outside the language specific directories (just like with sysprefs.sql) so we can remove all the other files and stop updating them.
Comment 12 Jonathan Druart 2015-02-10 08:08:26 UTC
Weird, I opened the same bug report on the same day!
Comment 13 Katrin Fischer 2015-02-10 08:48:52 UTC
And I commented on both... I am getting old :(
Comment 14 Paola Rossi 2015-02-10 12:39:40 UTC Comment hidden (obsolete)
Comment 15 Bernardo Gonzalez Kriegel 2015-02-10 14:09:50 UTC
Hi, good to see new strings to translate :)

But, can I suggest a little rewriting?

First) 
Can you put a <span>...</span> surrounding the text?
In that way each text is taken by translation script individually, if not we finish with a big and ugly string, difficult to translate or update

So I suggest changing this

[%- CASE 'superlibrarian' -%]
Access to all librarian functions
[%- CASE 'circulate' -%]
Check out and check in items
[%- CASE 'catalogue' -%]
<b>Required for staff login.</b> Staff access, allows viewing of catalogue in staff client.

to this

[%- CASE 'superlibrarian' -%]
<span>Access to all librarian functions</span>
[%- CASE 'circulate' -%]
<span>Check out and check in items</span>
[%- CASE 'catalogue' -%]
<span><b>Required for staff login.</b> Staff access, allows viewing of catalogue in staff client.</span>

Second) 
To improve readability, can you put the text next to the options?
And also add space to align things?

For example, change this

[%- CASE 'superlibrarian' -%]
<span>Access to all librarian functions</span>
[%- CASE 'circulate' -%]
<span>Check out and check in items</span>
[%- CASE 'catalogue' -%]
<span><b>Required for staff login.</b> Staff access, allows viewing of catalogue in staff client.</span>

to this

[%- CASE 'superlibrarian' -%]<span>Access to all librarian functions</span>
[%- CASE 'circulate'      -%]<span>Check out and check in items</span>
[%- CASE 'catalogue'      -%]<span><b>Required for staff login.</b> Staff access, allows viewing of catalogue in staff client.</span>

Bernardo
Comment 16 David Cook 2015-02-10 22:27:37 UTC
Jonathan: That is weird! 

Bernardo: Yep, that's certainly doable. I might not have time today but I'll look at this again. Happy for this to be set to Failed QA in the meantime...

Katrin: I suppose I'd have to look closer at the installation process. I don't know too much about how translation works, as only one of our libraries use other languages than English, so happy to receive any advice on that :)
Comment 17 Katrin Fischer 2015-02-10 22:39:56 UTC
It's not so much a translation thing - Bernardo already has provided the fix for that, but a maintenance/web installer problem.

The web installer will use the files in the language directory from the language you picked at the beginning of the installation process. There is no fallback to en if a file is missing.

So currently, when a new permission is added, we have to maintain not one central file, but the en file plus all translated files. Not good. 

For systempreferences the web installer knows that sysprefs.sql is mandatory, even tho it lives outside of the web installer files. I think maybe we can handle the permissions file the same way. One file - always installed independent of the picked language.
Comment 18 Jonathan Druart 2015-02-11 10:31:24 UTC
Since I suggest the same patch I completely agree with it :)

This patch should also remove all the slq files too.
Comment 19 Jonathan Druart 2015-02-11 10:32:12 UTC
*** Bug 13637 has been marked as a duplicate of this bug. ***
Comment 20 David Cook 2015-02-11 22:57:46 UTC
(In reply to Katrin Fischer from comment #17)
> It's not so much a translation thing - Bernardo already has provided the fix
> for that, but a maintenance/web installer problem.
> 
> The web installer will use the files in the language directory from the
> language you picked at the beginning of the installation process. There is
> no fallback to en if a file is missing.
> 
> So currently, when a new permission is added, we have to maintain not one
> central file, but the en file plus all translated files. Not good. 
> 
> For systempreferences the web installer knows that sysprefs.sql is
> mandatory, even tho it lives outside of the web installer files. I think
> maybe we can handle the permissions file the same way. One file - always
> installed independent of the picked language.

I think I understand what you're saying, but I'm not sure that will be an issue in this case?

Since the file will live in the regular "en" includes, won't it just be a regular candidate for translation?

If it gets translated, then users will see translated text. If it doesn't get translated, users will see English. Isn't that right?

That's where I'm confused :).
Comment 21 David Cook 2015-02-11 22:58:24 UTC
(In reply to Jonathan Druart from comment #18)
> Since I suggest the same patch I completely agree with it :)
> 
> This patch should also remove all the slq files too.

Hehe. Yay!

Good call! I suppose it should also remove any references to the SQL files in the installer. 

I'll have a look at this again when I have some free time.
Comment 22 David Cook 2015-03-26 23:19:44 UTC
(In reply to Katrin Fischer from comment #17)
> It's not so much a translation thing - Bernardo already has provided the fix
> for that, but a maintenance/web installer problem.
> 
> The web installer will use the files in the language directory from the
> language you picked at the beginning of the installation process. There is
> no fallback to en if a file is missing.
> 
> So currently, when a new permission is added, we have to maintain not one
> central file, but the en file plus all translated files. Not good. 
> 
> For systempreferences the web installer knows that sysprefs.sql is
> mandatory, even tho it lives outside of the web installer files. I think
> maybe we can handle the permissions file the same way. One file - always
> installed independent of the picked language.

I think I understand this problem now... and I don't have an immediate solution off the top of my head. 

I'm going to reset the assignee to default, as I probably won't have time to work on this again. 

Jonathan, if you're still interested in this bug, please feel free to take it over :).
Comment 23 Jonathan Druart 2015-09-22 09:00:45 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-09-22 09:00:53 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-09-22 09:00:57 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-09-22 09:01:00 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2015-09-22 09:01:04 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2015-09-22 09:02:12 UTC
*** Bug 13633 has been marked as a duplicate of this bug. ***
Comment 29 Paul Poulain 2015-10-09 12:14:29 UTC
changing severity : this is a bug for non english Koha (and when you demo & try to convince a library to choose Koha, this has a negative result)
Comment 30 Kyle M Hall 2015-10-16 15:31:33 UTC
Created attachment 43528 [details] [review]
[DO NOT PUSH] [SIGNED-OFF] Script to generate the permissions include file

perl build_permissions_file.pl > koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc

http://bugs.koha-community.org/show_bug.cgi?id=13632

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

http://bugs.koha-community.org/show_bug.cgi?id=13622
Comment 31 Kyle M Hall 2015-10-16 15:31:41 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2015-10-16 15:31:44 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2015-10-16 15:31:47 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2015-10-16 15:31:50 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2015-10-18 10:33:51 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2015-10-18 10:33:54 UTC
Created attachment 43562 [details] [review]
[PASSED QA] Bug 13632: Use an include file to display permissions

This patch set moves the permissions' descriptions to the po files
instead of having them in the DB.

Test plan:
0/ Apply all patches
1/ Create a new installation
2/ Confirm the userflags and permissions tables are correctly populated
3/ Update the po file for a translated language
4/ Confirm you are able to translate the permissions descriptions
5/ Install the template files for this language
6/ Switch the interface to use this language and confirm the string are
correctly translated.

QA Note: At this point we could remove the userflags.flagdesc and
permissions.description DB fields, but I would prefer to keep them:
developers will know what the permission do without the need to go and
see the include file (we have it in the sql files and so, in the DB).

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2015-10-18 10:33:58 UTC
Created attachment 43563 [details] [review]
[PASSED QA] Bug 13632: Do not use userflags.flagdesc and permissions.description

These 2 fields are not used anymore, but we want to keep them anyway.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2015-10-18 10:34:02 UTC
Created attachment 43564 [details] [review]
[PASSED QA] Bug 13632: Populate user_flags and permissions tables in the web installer

Move the 2 specific 'en' files outside of the language directory.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2015-10-18 10:34:06 UTC
Created attachment 43565 [details] [review]
[PASSED QA] Bug 13632: Remove specific files

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 Katrin Fischer 2015-10-18 10:34:10 UTC
Created attachment 43566 [details] [review]
Bug 13632: QA follow-up: Tidy installer directories / update permissions.inc

- removes permission related text files from translated
  web installer directories
- updates the permissions.inc file to include the newest
  permission 'self_checkout'
Comment 41 Katrin Fischer 2015-10-18 10:36:34 UTC
Really happy about this one - there was currently no way to update/fix permission descriptions apart from SQL. Especially annoying for older databases.

Jonathan - I added a follow-up. Can you check please?
Comment 42 Jonathan Druart 2015-10-19 07:14:57 UTC
Katrin, thanks for the follow-up.
Yes of course it makes sense to remove the txt files!
Comment 43 Tomás Cohen Arazi 2015-10-19 13:46:30 UTC
Long-standing missing feature,

good job Jonathan!
Comment 44 Tomás Cohen Arazi 2015-10-19 15:45:47 UTC
Created attachment 43599 [details] [review]
Bug 13632: (QA followup) remove xt/permissions.t

There's no longer a need for this tests, as their goal was
to make sure en (the basis) was in sync with the translated
files.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 David Cook 2015-10-19 23:54:57 UTC
Thanks Jonathan! I've been so busy this year, so I'm really glad that you kept going with this one!
Comment 46 Chris Cormack 2015-10-24 07:44:39 UTC
Translation changes, held for 3.20.6