Add a ''Manage Lingotek translations permission'' allowing operations related to translation but not module settings

Hi McCann!

I think we should just add a new permission in lingotek.permissions.yml and modify lingotek.routing.yml for using that one. Is all of this really necessary?

If it is, I fail to see how at the moment.

1. +++ b/lingotek.permissions.yml
   @@ -5,3 +5,5 @@ administer lingotek:
   +show lingotek settings tab:
   +  title: 'Show Lingotek Settings Tab'
   

   I would rename this to "Manage Lingotek translations"

2. +++ b/src/Tests/LingotekDashboardTestHideSettingsTab.php
   @@ -0,0 +1,517 @@
   +class LingotekDashboardTestHideSettingsTab extends LingotekTestBase {
   

   A lot of tests here are unrelated, I guess copied from somewhere else. Let's remove those.

3. +++ b/src/Tests/LingotekDashboardTestHideSettingsTab.php
   @@ -0,0 +1,517 @@
   +  public function makeNewRoleCalledSubAdmin(){
   ...
   +  public function makeNewUserWithSubAdminRole(){
   ...
   +  public function logIntoAdminRoleAndSeeSettingsTab($uid){
   ...
   +  public function logIntoSubAdminRoleAndDontSeeSettingsTab($uid){
   

   Where are these used? We can do the same more easily just with

   $userWithRoleX = $this->createUser([array of permissions])
   $this->drupalLogin($userWithRoleX);
   

Also, we will need changes to \Drupal\lingotek\Routing\LingotekRouteSubscriber::alterRoutes, which creates the manage routes dinamically for each enabled entity.

This 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.

+++ b/lingotek.permissions.yml
@@ -5,3 +5,7 @@ administer lingotek:
+  ¶
\ No newline at end of file

+++ b/src/Controller/LingotekDashboardController.php
@@ -106,7 +106,8 @@ class LingotekDashboardController extends LingotekControllerBase {
-    }
+    } ¶
+    ¶

+++ b/src/Routing/LingotekRouteSubscriber.php
@@ -33,7 +33,7 @@ class LingotekRouteSubscriber extends RouteSubscriberBase {
-    $this->entityManager = $entity_manager;
+  	 $this->entityManager = $entity_manager;

@@ -54,7 +54,7 @@ class LingotekRouteSubscriber extends RouteSubscriberBase {
-
+        ¶

@@ -67,11 +67,11 @@ class LingotekRouteSubscriber extends RouteSubscriberBase {
-
+        ¶

We need to fix the spacing issues.

1. +++ b/lingotek.permissions.yml
   @@ -5,3 +5,6 @@ administer lingotek:
   \ No newline at end of file
   

   I 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.

1. +++ b/lingotek.permissions.yml
   @@ -5,3 +5,6 @@ administer lingotek:
   +show lingotek settings tab:
   

   Let's call this "manage lingotek translations".

2. +++ b/lingotek.permissions.yml
   @@ -5,3 +5,6 @@ administer lingotek:
   +  description: 'Allow access to the settings tab in the Lingotek Translation Module.'
   

   The description is "Allow users to send content for translations and download translations when they are ready" or something alike. Feel free to improve :-)

3. +++ b/lingotek.routing.yml
   @@ -78,6 +78,7 @@ lingotek.settings:
        _permission: 'administer lingotek'
   +    _permission: 'show lingotek settings tab'
   

   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.

4. +++ b/src/Routing/LingotekRouteSubscriber.php
   @@ -67,7 +67,7 @@ class LingotekRouteSubscriber extends RouteSubscriberBase {
   +          array('_permission' => 'administer lingotek','_permission' => 'show lingotek settings tab'),
   

   "administer lingotek" should not be required for accessing these pages. They are the bulk management tab.

5. +++ b/src/Tests/LingotekHideSettingsTabTest.php
   @@ -0,0 +1,60 @@
   +class LingotekSettingsTabContentFormTest extends LingotekTestBase {
   

   This test is a great start but we need more I guess :-)

6. +++ b/src/Tests/LingotekHideSettingsTabTest.php
   @@ -0,0 +1,60 @@
   +    $permissions = \Drupal::service('user.permissions')->getPermissions();
   +    $role = \Drupal\user\Entity\Role::create(array('id' => 'client', 'label' => 'Client'));
   +    foreach( $permissions as $permission ) {
   +      $role->grantPermission( $permission );
   +    }
   +    $role->revokePermission('Manage Lingotek translations');
   +    $role->save();
   +
   +    // Create user object.
   +    $user = User::create();
   +
   +    //Mandatory settings
   +    $user->setPassword("password");
   +    $user->enforceIsNew();
   +    $user->setEmail("email");
   +    $user->setUsername("username");
   +    $user->addRole($role);
   +
   +    $this->drupalLogin($user);
   

   We don't want to access all the permissions. Also, see comment #14:

   $userWithRoleX = $this->createUser([array of permissions])
   $this->drupalLogin($userWithRoleX);
   

+++ b/lingotek.module
@@ -16,6 +16,15 @@ use Drupal\lingotek\Exception\LingotekApiException;
+ * Implements hook_user_login().
+ */
+function lingotek_user_login($account) {
+  if ($account->getLastAccessedTime() == 0) {
+    drupal_flush_all_caches();
+  }
+}
+
+/**

Why is this needed? Flush all caches must need a very good reason to happen, as it can really slow down a site.

We 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.

See 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.

+++ b/src/Tests/LingotekManageLingotekTranslationsPermissionTest.php
@@ -0,0 +1,51 @@
+    $user = $this->drupalCreateUser([
...
+      'assign lingotek translation profiles'
+    ]);
+    // Login as user.
+    $this->drupalLogin($user);
+    // Get the settings form.
+    $this->drupalGet('admin/lingotek/settings');
+    // Assert translation profile cannot be assigned.
+    $this->assertText('You are not authorized to access this page.');

This 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.

Best practices say we should move the hook_update_N to a post-update:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

In particular, loading, saving, or performing any other CRUD operation on an entity is never safe to do (they always involve hooks and services).

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

Executes an update which is intended to update data, like entities.

Changed 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:

• testCannotSeeSettingsTabWithoutRightPermission
• testDashboardAsTranslationsManager
• testNavigationThroughSiteForBulkContentTranslationAsTranslationsManager
• testNavigationThroughSiteForBulkConfigTranslationAsTranslationsManager
• testNavigationThroughSiteForBulkConfigTranslationAsTranslationsManagerWithTranslateConfigPermission

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).

Changed 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).

* Tests for ensuring the links on the translation pages are not available without the manage translations permission.

Fixed failing tests.

Fixed tests. Added post-upgrade test.

Looks like everything is good now :-)

Fixed some phpcs issues on commit, attached interdiff.

Committed 440c1ed and pushed to 8.x-2.x. Thanks!

The interdiff