Steps to reproduce and test

  1. Fresh Drupal install (with either Seven or Claro as admin theme)
  2. Disable JavaScript aggregation in "admin/config/development/performance", and clear cache.
  3. Go to "admin/people/permissions"
  4. Check the network tab -> misc/checkbox.js won't be loaded
  5. Confirm the above by ticking any unticked permission checkbox for the "Authenticated" role:
    • actual behavior: the respective checkboxes for the permissions for all other roles disappear, except for the Anonymous role
    • expected behavior: the respective checkboxes for the permissions should be ticked for all other roles, except Anonymous
  6. Apply the patch, clear cache and check the network tab again -> misc/checkbox.js should be loaded.
  7. Try clicking any unticked checkbox for the "Authenticated" role -> the respective checkboxes for the permissions should be ticked for all other roles, except Anonymous

In fact, we need to register the lib explicitly: (above reproduce steps isn't important)
https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/user...

This change may improve/reduce the performance: #3244564: Improve performance of /admin/people/permissions

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

andypost’s picture

Fix line-endings in patch

andypost’s picture

Issue tags: +Needs tests

needs some test to extend

droplet’s picture

Title: "core/drupal.checkbox" (user.permission.js) has never loaded on user permission page » "core/drupal.checkbox" (misc/checkbox.js) has never loaded on user permission page

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Kristen Pol’s picture

Thanks for the issue and patch. Tagging for testing, updating the issue summary to use the template, and add screenshots.

Seems like it would be hard to add a manual test but I'll leave that to someone who writes tests to decide :)

longwave’s picture

Confirmed the behaviour manually, and added a functional JS test.

What should happen is that disabled pre-selected checkboxes appear for other roles when you select the "authenticated user" role for any permission, as that overrides all other roles. Without the patches the dummy checkboxes don't appear; instead the per-role checkboxes just disappear, which isn't as clear to the user as to what it means.

The last submitted patch, 7: 3244737-8-test-only.patch, failed testing. View results

Satyajit1990’s picture

Issue summary: View changes
FileSize
437.34 KB
166.89 KB

Hi posting my QA observation before the patch

Testing steps before patch :
1. Go to the admin site.
2. Go to admin/appearance >> Enable the Seven theme as default
3. Go to the people >> Permission tab
4. Now check the checkbox for the Authenticated user and observe for other checkbox permission
5. Now issue is after checking the Authenticated user other checkbox are getting disappear.

Testing Result after applying patch :
1. After applying the patch and after checking the AUTHENTICATED USER checkbox now other checkbox is visible and by default CONTENT EDITOR checkbox also getting checked and disabled.
2. Secondly there is no load response upon check and uncheck on network tab.

Kristen Pol’s picture

Thanks for testing @Satyajit1990, but you don't need to check any boxes.

The issue is that the file misc/checkbox.js isn't loaded when you are on this page, which you should be able to see in the Network tab. After the patch, you should be able to see the misc/checkbox.js file loaded on this page in the Network tab.

joshua1234511’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
41.93 KB
51.61 KB

Tested the patch with the following steps.
- Fresh Drupal install with admin theme set as seven.
- Check the network tab on permission page, misc/checkbox.js won't be loaded
- Apply the patch, clear cache and check the net work tab again.
- misc/checkbox.js should be loaded.
Note: -You will need to disable Aggregate and clear cache to see file list.
Before Patch
Before Patch
After Patch
After Patch
Updated the issue summary with test steps.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3244737-8.patch, failed testing. View results

Kristen Pol’s picture

Issue tags: -Needs screenshots

Looks like an unrelated failure so retesting.

longwave’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC, test failures were unrelated.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3244737-8.patch, failed testing. View results

Kristen Pol’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Unrelated failure. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3244737-8.patch, failed testing. View results

longwave’s picture

Status: Needs work » Reviewed & tested by the community

And again :(

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3244737-8.patch, failed testing. View results

yogeshmpawar’s picture

Status: Needs work » Reviewed & tested by the community

Looks like test failures are unrelated.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3244737-8.patch, failed testing. View results

immaculatexavier’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

Fixed the below issue
Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-147.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.19 �[44m#StandWith�[0m�[43mUkraine�[0m

Warning: Your XML configuration validates against a deprecated schema.
Suggestion: Migrate your XML configuration using "--migrate-configuration"!

Attaching the updated patch

longwave’s picture

Status: Needs review » Needs work

#22 is out of scope, phpunit.xml needs to remain compatible with both PHPUnit 8 and 9 for now.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs work » Reviewed & tested by the community

Patch #7 appears to still work on 10.1.x and was marked RTBC before #22 so putting back and hiding patch #22

klonos’s picture

Issue summary: View changes

Cleaned up the issue summary, and removed explicit reference to the Seven theme, as this happens with Claro as well (tested before/after patch and confirmed).

Checking the network tab in the browser for presence of the misc/checkbox.js file is one thing to test for, but testing for the actual checkbox behavior being restored is also a valid, additional thing to check and confirm.

klonos’s picture

Issue summary: View changes
larowlan’s picture

+++ b/core/modules/user/tests/src/FunctionalJavascript/UserPermissionsTest.php
@@ -0,0 +1,79 @@
+    $this->assertFalse($real_checkbox->isVisible());
+    $this->assertTrue($dummy_checkbox->isVisible());
+    $this->assertTrue($dummy_checkbox->isChecked());

I was going to ask that we also add another assertTrue for the real checkbox (i.e that it remains checked) but then read the code of user.permissions.js and saw that it doesn't do anything to the checked state, only the style properties.

Updating issue credits

  • larowlan committed 2455ba0f on 10.1.x
    Issue #3244737 by longwave, droplet, andypost, joshua1234511,...

  • larowlan committed 24e6a492 on 10.0.x
    Issue #3244737 by longwave, droplet, andypost, joshua1234511,...

  • larowlan committed 3afe0e47 on 9.5.x
    Issue #3244737 by longwave, droplet, andypost, joshua1234511,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 10.1.x and backported to 10.0.x and 9.5.x

larowlan’s picture

Version: 10.1.x-dev » 9.5.x-dev

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.