Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Customer has requested the ability to give access to the Translations > Manage page without giving full module admin access (meaning they don't want the user to have access to the Settings tab). This would be an additional user permissions type.
Comment | File | Size | Author |
---|---|---|---|
#76 | 2876128-manage-translations-permission-76.patch | 253.41 KB | penyaskito |
|
Comments
Comment #2
mdahl328151 CreditAttribution: mdahl328151 commentedComment #3
t.murphy CreditAttribution: t.murphy at Lingotek commentedComment #4
mdahl328151 CreditAttribution: mdahl328151 commentedComment #6
mdahl328151 CreditAttribution: mdahl328151 commentedComment #7
mdahl328151 CreditAttribution: mdahl328151 commentedComment #8
mdahl328151 CreditAttribution: mdahl328151 commentedComment #9
mdahl328151 CreditAttribution: mdahl328151 commentedComment #10
mdahl328151 CreditAttribution: mdahl328151 commentedComment #11
penyaskitoHi McCann!
I think we should just add a new permission in
lingotek.permissions.yml
and modifylingotek.routing.yml
for using that one. Is all of this really necessary?If it is, I fail to see how at the moment.
Comment #12
mdahl328151 CreditAttribution: mdahl328151 commentedComment #13
mdahl328151 CreditAttribution: mdahl328151 commentedComment #14
penyaskitoI would rename this to "Manage Lingotek translations"
A lot of tests here are unrelated, I guess copied from somewhere else. Let's remove those.
Where are these used? We can do the same more easily just with
Comment #15
penyaskitoAlso, we will need changes to
\Drupal\lingotek\Routing\LingotekRouteSubscriber::alterRoutes
, which creates the manage routes dinamically for each enabled entity.Comment #16
mdahl328151 CreditAttribution: mdahl328151 commentedComment #17
mdahl328151 CreditAttribution: mdahl328151 commentedComment #18
penyaskitoThis feature needs testing. We need to test that a user without the proper permission gets a "Not allowed" error when going to that url, and that the permissions gives access back to this feature.
We need to fix the spacing issues.
Comment #19
mdahl328151 CreditAttribution: mdahl328151 commentedComment #20
mdahl328151 CreditAttribution: mdahl328151 commentedWhen un-authorized user attempts to go to /admin/lingotek/settings this is displayed:
Access denied
You are not authorized to access this page.
The URL can only be accessed after that if a user with the "settings tab" permission logs in.
Comment #21
mdahl328151 CreditAttribution: mdahl328151 commentedSpacing issues fixed.
Comment #22
mdahl328151 CreditAttribution: mdahl328151 commentedComment #23
penyaskitoI can fix this at commit, but take into account for other patches.
Did you forget to include the automated tests in the patch? Ping me if you need help with that.
Comment #24
mdahl328151 CreditAttribution: mdahl328151 commentedComment #25
mdahl328151 CreditAttribution: mdahl328151 commentedComment #26
mdahl328151 CreditAttribution: mdahl328151 commentedComment #27
penyaskitoLet's call this "manage lingotek translations".
The description is "Allow users to send content for translations and download translations when they are ready" or something alike. Feel free to improve :-)
No, "administer lingotek" will mean access to lingotek.settings.
"manage lingotek translations" will mean, access to the operations in the translate tab + access to the bulk management pages.
"administer lingotek" should not be required for accessing these pages. They are the bulk management tab.
This test is a great start but we need more I guess :-)
We don't want to access all the permissions. Also, see comment #14:
Comment #28
mdahl328151 CreditAttribution: mdahl328151 commentedComment #29
mdahl328151 CreditAttribution: mdahl328151 commentedComment #30
mdahl328151 CreditAttribution: mdahl328151 commentedComment #31
mdahl328151 CreditAttribution: mdahl328151 commentedComment #32
mdahl328151 CreditAttribution: mdahl328151 commentedThis should be:
'administer lingotek','assign lingotek translation profiles'
Comment #33
mdahl328151 CreditAttribution: mdahl328151 commentedComment #34
mdahl328151 CreditAttribution: mdahl328151 commentedComment #35
mdahl328151 CreditAttribution: mdahl328151 commentedComment #36
mdahl328151 CreditAttribution: mdahl328151 commentedComment #37
mdahl328151 CreditAttribution: mdahl328151 commentedComment #38
mdahl328151 CreditAttribution: mdahl328151 commentedComment #40
mdahl328151 CreditAttribution: mdahl328151 commentedComment #42
mdahl328151 CreditAttribution: mdahl328151 commentedComment #43
mdahl328151 CreditAttribution: mdahl328151 commentedComment #45
mdahl328151 CreditAttribution: mdahl328151 commentedComment #46
mdahl328151 CreditAttribution: mdahl328151 commentedComment #47
mdahl328151 CreditAttribution: mdahl328151 commentedComment #48
mdahl328151 CreditAttribution: mdahl328151 commentedComment #50
mdahl328151 CreditAttribution: mdahl328151 commentedComment #51
mdahl328151 CreditAttribution: mdahl328151 commentedComment #52
penyaskitoWhy is this needed? Flush all caches must need a very good reason to happen, as it can really slow down a site.
Comment #53
penyaskitoWe need an upgrade function for this issue. When a site upgrades from a previous version of the module, any user role with the permission for "administer lingotek'" should have this new permission too.
We ideally need tests for that upgrade.
Comment #54
mdahl328151 CreditAttribution: mdahl328151 commentedI could not re-create the issue I was having before, so the user login hook is not needed. When a module is upgraded, is there a function that is called within the module, or would I have to write code that compares different module versions?
Comment #55
penyaskitoSee https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension... and https://www.drupal.org/docs/7/testing/writing-upgrade-path-tests.
There are some examples in
lingotek.install
already.Comment #56
mdahl328151 CreditAttribution: mdahl328151 commentedComment #57
mdahl328151 CreditAttribution: mdahl328151 commentedComment #59
mdahl328151 CreditAttribution: mdahl328151 commentedComment #60
mdahl328151 CreditAttribution: mdahl328151 commentedComment #61
penyaskitoThis is not what this permission is aiming for.
User with 'administer Lingotek' should be able of seeing the settings page.
The 'manage lingotek translations' is required for the bulk translation tabs, to see the Lingotek operations in the Translate tab of content or config. 'administer lingotek' is for accessing settings of the module.
Just because of this test I'm assuming this permission is not worked as expected.
Comment #62
penyaskitoBest practices say we should move the hook_update_N to a post-update:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
Comment #63
mdahl328151 CreditAttribution: mdahl328151 commentedThat makes sense. So this is what the routing should look like?
Comment #64
mdahl328151 CreditAttribution: mdahl328151 commentedI forgot the Role namespace
Comment #65
mdahl328151 CreditAttribution: mdahl328151 commentedComment #67
mdahl328151 CreditAttribution: mdahl328151 commentedComment #68
mdahl328151 CreditAttribution: mdahl328151 commentedComment #69
penyaskitoChanged more permissions, the previous patch could be functionally correct, but it wasn't possible to navigate through the admin pages for reaching the desired functionality.
Implemented more tests:
We still need tests for the "Translate pages". Users shouldn't upload without the right permissions there.
We still need tests for ensuring the links on the bulk management page and the bulk actions work for a user with restricted permissions (only translate).
Comment #70
penyaskitoChanged existing tests so we add the following scenarios:
* Tests for ensuring the links on the bulk management page and the bulk actions work for a user with restricted permissions (only translate).
Comment #72
penyaskito* Tests for ensuring the links on the translation pages are not available without the manage translations permission.
Comment #74
penyaskitoFixed failing tests.
Comment #76
penyaskitoFixed tests. Added post-upgrade test.
Comment #77
penyaskitoLooks like everything is good now :-)
Comment #78
penyaskitoFixed some phpcs issues on commit, attached interdiff.
Comment #79
penyaskitoCommitted 440c1ed and pushed to 8.x-2.x. Thanks!
Comment #81
penyaskitoThe interdiff
Comment #82
penyaskito