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.
Created attachment 35569 [details] [review] Bug 13632 - Use a translateable file instead of the database for managing permission descriptions This patch defines a couple of blocks in "permissions.inc", which is included/processed in member-flags.tt to show the descriptions for the permissions and sub-permissions. _TEST PLAN_ Before applying: 1) Look at the descriptions for permissions and subpermissions 2) Open another tab/window and load the same page Apply the patch 3) Reload the second tab/window 4) Note that the descriptions are exactly the same as before If you're feeling bold, you can change the text in `permissions.description` and `userflags.flagdesc` and notice that the text on the page doesn't change. However, you should not delete the `permissions.description` column or change the text to NULL or blank. C4::Auth::get_all_subpermissions() uses the description for creating a hashref of subpermissions. If it can't get the column, Koha will explode. If there is nothing in the column, your subpermissions won't work correctly.
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 :)
Created attachment 35595 [details] [review] Bug 13632 - Use a translateable file instead of the database for managing permission descriptions This patch defines a couple of blocks in "permissions.inc", which is included/processed in member-flags.tt to show the descriptions for the permissions and sub-permissions. _TEST PLAN_ Before applying: 1) Look at the descriptions for permissions and subpermissions 2) Open another tab/window and load the same page Apply the patch 3) Reload the second tab/window 4) Note that the descriptions are exactly the same as before If you're feeling bold, you can change the text in `permissions.description` and `userflags.flagdesc` and notice that the text on the page doesn't change. However, you should not delete the `permissions.description` column or change the text to NULL or blank. C4::Auth::get_all_subpermissions() uses the description for creating a hashref of subpermissions. If it can't get the column, Koha will explode. If there is nothing in the column, your subpermissions won't work correctly.
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.
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.
(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.
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.
Created attachment 35761 [details] [review] Bug 13632 - Use a translateable file instead of the database for managing permission descriptions This patch defines a couple of blocks in "permissions.inc", which is included/processed in member-flags.tt to show the descriptions for the permissions and sub-permissions. _TEST PLAN_ Before applying: 1) Look at the descriptions for permissions and subpermissions 2) Open another tab/window and load the same page Apply the patch 3) Reload the second tab/window 4) Note that the descriptions are exactly the same as before If you're feeling bold, you can change the text in `permissions.description` and `userflags.flagdesc` and notice that the text on the page doesn't change. However, you should not delete the `permissions.description` column or change the text to NULL or blank. C4::Auth::get_all_subpermissions() uses the description for creating a hashref of subpermissions. If it can't get the column, Koha will explode. If there is nothing in the column, your subpermissions won't work correctly.
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.
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 ;)
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.
Weird, I opened the same bug report on the same day!
And I commented on both... I am getting old :(
Created attachment 35776 [details] [review] [SIGNED OFF] Bug 13632 - Use a translateable file instead of the database for managing permission descriptions In reply to David Cook from Comment #9: >[...] here's the updated English version. Thanks, David. I've applied the patch against master 3.19.00.010 This was the only difference I saw: applied: Add and delete budgets (but can't modify budgets) master : Add and delete budgets (but can't modifiy budgets) So I pass the patch to the "Signed Off" status. > > [...] change the text in > `permissions.description` and `userflags.flagdesc` and notice > that the text on the page doesn't change. > Yes, as required.
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
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 :)
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.
Since I suggest the same patch I completely agree with it :) This patch should also remove all the slq files too.
*** Bug 13637 has been marked as a duplicate of this bug. ***
(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 :).
(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.
(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 :).
Created attachment 42756 [details] [review] [DO NOT PUSH] Script to generate the permissions include file perl build_permissions_file.pl > koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
Created attachment 42757 [details] [review] 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).
Created attachment 42758 [details] [review] Bug 13632: Do not use userflags.flagdesc and permissions.description These 2 fields are not used anymore, but we want to keep them anyway.
Created attachment 42759 [details] [review] Bug 13632: Populate user_flags and permissions tables in the web installer Move the 2 specific 'en' files outside of the language directory.
Created attachment 42760 [details] [review] Bug 13632: Remove specific files
*** Bug 13633 has been marked as a duplicate of this bug. ***
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)
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
Created attachment 43529 [details] [review] [SIGNED-OFF] 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>
Created attachment 43530 [details] [review] [SIGNED-OFF] 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>
Created attachment 43531 [details] [review] [SIGNED-OFF] 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>
Created attachment 43532 [details] [review] [SIGNED-OFF] Bug 13632: Remove specific files Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 43561 [details] [review] 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
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>
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>
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>
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>
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'
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?
Katrin, thanks for the follow-up. Yes of course it makes sense to remove the txt files!
Long-standing missing feature, good job Jonathan!
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>
Thanks Jonathan! I've been so busy this year, so I'm really glad that you kept going with this one!
Translation changes, held for 3.20.6