Skip to content
Snippets Groups Projects

Issue #3439833: Remove $usesSuperUserAccessPolicy Varibale.

Closed Issue #3439833: Remove $usesSuperUserAccessPolicy Varibale.
issue/drupal-3439833:3439833-fix-content-translation into 11.x
4 unresolved threads
Closed Pradhuman Jain requested to merge issue/drupal-3439833:3439833-fix-content-translation into 11.x
4 unresolved threads

Closes #3439833

Merge request reports

Merge request pipeline #273189 passed

Stage: 🏗️ Build
Stage: 🪄 Lint
Stage: 🗜️ Test
Pipeline: drupal-3439833

#273191

    Merge request pipeline passed for eef7aba5

    Approval is optional
    Code Quality is loading
    Test summary results are being parsed
    Full report

    Closed by quietonequietone 9 months ago (Sep 5, 2024 8:07am UTC)

    Merge details

    • The changes were not merged into .

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
    core/modules/content_translation/tests/src/Functional/ContentTranslationEnableTest.php
    55 58 $this->drupalGet('admin/reports/status');
    56 59 $this->assertSession()->elementTextEquals('css', "details.system-status-report__entry summary:contains('Entity/field definitions') + div", 'Up to date');
    57 60
    61 $all_rids = $web_user->getRoles();
    • Stephen Mustgrave
      Stephen Mustgrave @smustgrave
      Developer

      This is all out of scope

    • Pooja Sharma
      Pooja Sharma @pooja_sharma

      I have verified Permission can be assign: if module installed before user login, here we can see at line no. 42-47 module install, in this scenario way to assign permission by creating role with respective perms (tried to check in the core test files also).

      Edited by Pooja Sharma
    • Stephen Mustgrave
      Stephen Mustgrave @smustgrave
      Developer

      Not sure what you mean but the goal of these tickets are just replace user 1 with needed permissions. If we have to update the logic then the ticket scope has expanded.

      but anyway this test isn't testing the Permissions page so don't think we need to visit it to create permissions.

    • Pooja Sharma
      Pooja Sharma @pooja_sharma

      I agree goal of ticket is adding permissions only, however permission can be assign to user only if module install code added before user login, if module install (can notice at line no. 42-47) & respective config install (if required/added) code added after user login code in that case scenario permission can be assign only by adding role (checked in core as well)

      Edited by Pooja Sharma
    • Pooja Sharma changed this line in version 14 of the diff

      changed this line in version 14 of the diff

    • Please register or sign in to reply
  • Stephen Mustgrave
    Stephen Mustgrave @smustgrave started a thread on an old version of the diff
    • core/modules/content_translation/tests/src/Functional/ContentTranslationUntranslatableFieldsTest.php
    169 171 // Enable content moderation and verify that widgets are hidden despite them
    170 172 // being configured to be displayed.
    171 173 $this->enableContentModeration();
    174 $this->drupalLogin($this->administrator);
  • Pooja Sharma added 1 commit

    added 1 commit

    • 493ed840 - issue/3439833: Updated minimum perms ContentTranslationEnableTest

    Compare with previous version

  • Pooja Sharma added 1 commit

    added 1 commit

    • 58089444 - issue/3439833: Updated minimum perms ContentTranslationOutdatedRevisionTranslationTest

    Compare with previous version

  • Pooja Sharma added 1 commit

    added 1 commit

    • f479e6cd - issue/3439833: Updated minimum perms ContentTranslationUntranslatableFieldsTest

    Compare with previous version

  • Pooja Sharma added 1 commit

    added 1 commit

    • 1ddd01e1 - issue/3439833: Updated minimum perms ContentTranslationRevisionTranslationDeletionTest

    Compare with previous version

  • Pooja Sharma added 1 commit

    added 1 commit

    • daf8ec33 - issue/3439833: Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Pooja Sharma added 1 commit

    added 1 commit

    • 4da53101 - issue/3439833: Updated ContentTranslationEnableTest

    Compare with previous version

  • Pooja Sharma
    Pooja Sharma @pooja_sharma started a thread on an old version of the diff
    • core/modules/content_translation/tests/src/Functional/ContentTranslationEnableTest.php
    46 43 ];
    47 44 $this->drupalGet('admin/modules');
    48 45 $this->submitForm($edit, 'Install');
    46 $this->rebuildContainer();
    47
    48 $web_user->addRole($this->drupalCreateRole([
    49 'administer content translation',
    50 'administer languages',
    51 ]))->save();
    • Comment on lines +46 to +51
      Pooja Sharma
      Pooja Sharma @pooja_sharma

      Added API() to add perms, but there needs to be rebuidContainer() otherwise invalid perms error display

      even I tried to debug like: \Drupal::service('user.permissions')->getPermissions();before using rebuildContainer() but did not able to see perms but after adding it , using again getPermissions() can able to see perms & also able to assign to the user.

      used drupalCreateRole() for assign perms, as it use current user object, but in grantPermissions() role need to pass like this :

      $this->grantPermissions(Role::load(Role::AUTHENTICATED_ID), [
      'administer content translation',
      'administer languages',
]);
      Edited by Pooja Sharma
    • Stephen Mustgrave
      Stephen Mustgrave @smustgrave
      Developer

      Looking at the current test, the previous didn't login another user so these permissions can probably be added to the initial creation.

    • Pooja Sharma
      Pooja Sharma @pooja_sharma

      Permissions are available to assign only after module install code (otherwise error display invalid permissions - correct behaviour), Here user login code added before module install , so in that scenario permissions can be added to current login user by addRole/getPermissions().

    • Stephen Mustgrave
      Stephen Mustgrave @smustgrave
      Developer

      Also not understanding why we need a new role when a module is installed. Wouldn't do that on a real site.

    • Pooja Sharma
      Pooja Sharma @pooja_sharma

      I believe may be something is missing from my end, however Checked in core files, there are two ways to assign the perms for logged in user one is by addRole() & other is getPermissions() in both we pass perms that are available after module install. (if module install code added after user login)

      should I use grantPermissions()?

      Edited by Pooja Sharma
    • Pooja Sharma changed this line in version 16 of the diff

      changed this line in version 16 of the diff

    • Please register or sign in to reply
  • Pooja Sharma added 1 commit

    added 1 commit

    • caa85073 - issue/3439833: Applied suggestios.

    Compare with previous version

  • Pooja Sharma
    Pooja Sharma @pooja_sharma started a thread on an old version of the diff
    • core/modules/content_translation/tests/src/Functional/ContentTranslationRevisionTranslationDeletionTest.php
    52 44 }
    53 45 }
    54 46
    47 /**
    48 * {@inheritdoc}
    49 */
    50 protected function getEditorPermissions(): array {
    51 return array_merge(parent::getEditorPermissions(), [
    52 "view latest version",
    53 ]);
    54 }
    55
    • Comment on lines +52 to +55
      Pooja Sharma
      Pooja Sharma @pooja_sharma

      Override it & added this specific function "view latest version" as this perms required to access revision page w.r.t. suggested approach.

      If not include this code , then test not break but when I compare page output of this test page (page no. 24 generated by test) with old test (with superuser code added) then this page not accessible.

      Edited by Pooja Sharma
    • quietone
      quietone @quietone
      Maintainer

      Very nice to test so thoroughly. I did the same with and without the diff applied. The results for this tests are different, unfortunately. The original test has 22 pages that have the text 'Access denied' while this new version has 6. The new version should have the same output as the original.

    • Pooja Sharma changed this line in version 17 of the diff

      changed this line in version 17 of the diff

    • Pooja Sharma
      Pooja Sharma @pooja_sharma

      Tried to analysed in original this test(ContentTranslationRevisionTranslationDeletionTest) found only 8 pages that have the text 'Access denied, their list : 40, 110, 126, 161, 196, 212, 247, 271

      if you think I 'm missing anything please correct.

    • quietone
      quietone @quietone
      Maintainer

      @pooja_sharma, thanks for changing this so quickly. I adjusted my command to print the number of files with 'access denied'. I then retested and get the same pages with 'access denied' with and with the MR. And they are same pages reported above.

      So this is fixed, thanks

    • Please register or sign in to reply
  • quietone
    quietone @quietone
    Maintainer

    This looks much better and is easier to read and understand. Thanks!

  • Pooja Sharma added 1 commit

    added 1 commit

    • eef7aba5 - issue/3439833: fixed permissions

    Compare with previous version

  • quietone closed

    closed

    • Please register or sign in to reply