Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
22.85 KB
13.86 KB
8.91 KB
26.95 KB

While there's no consensus on #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)

Patch add form and list controllers.
- UI normalized to all admin listings (there's no Add role within table)
- Introduced Add role local action
- Edit/Delete now local tabs
- Removed theme_user_admin_roles()

Role list
Role add
Role edit

andypost’s picture

@fubhy what's left here?

fubhy’s picture

I guess we can get this committed and refactor it once #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) lands. I don't consider that to be a blocker for this issue anymore.

RTBC anyone?

alexpott’s picture

#1: 1872870-roles-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1872870-roles-1.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
23.04 KB
3.77 KB

Chasing HEAD:

  • List controller implemented on top of FormInterface
  • Default operation is 'Edit permissions' - I think it more useful
alexpott’s picture

Status: Needs review » Needs work

Some minor nits...

+++ b/core/modules/user/lib/Drupal/user/RoleListController.phpundefined
@@ -0,0 +1,156 @@
+/**
+ * Contains \Drupal\user\RoleListController.
+ */

Missing @file

+++ b/core/modules/user/lib/Drupal/user/RoleListController.phpundefined
@@ -0,0 +1,156 @@
+   *
+   * Form constructor for manipulating role weights.
+   *
+   * @param array $form
+   *   An associative array containing the structure of the form.
+   * @param array $form_state
+   *   A reference to a keyed array containing the current state of the form.
+   *
+   * @return array
+   *   The array containing the complete form.

Unnecessary documentation.

+++ b/core/modules/user/lib/Drupal/user/RoleListController.phpundefined
@@ -0,0 +1,156 @@
+   *
+   * Form submission handler for the roles overview form.
+   *
+   * @param array $form
+   *   An associative array containing the structure of the form.
+   * @param array $form_state
+   *   A reference to a keyed array containing the current state of the form.

Unnecessary documentation.

+++ b/core/modules/user/user.admin.incundefined
@@ -849,208 +849,34 @@ function theme_user_permission_description($variables) {
+  return \Drupal::service('plugin.manager.entity')

Afaik unnecessary leading \

/**
 * Page callback: Lists roles and allows to reorder them.
 *
 * @see user_menu()
 */
function user_admin_roles_list() {
  return \Drupal::service('plugin.manager.entity')
    ->getListController('user_role')->render();
}

/**
 * Page callback: Presents the role creation form.
 *
 * @return array
 *   A form array as expected by drupal_render().
 *
 * @see user_menu()
 */
function user_admin_role_add() {
  drupal_set_title(t('Add role'));
  $role = entity_create('user_role', array());
  return entity_get_form($role);
}

Either we need to document the @return or not... but this seems inconsistent.

andypost’s picture

Status: Needs work » Needs review
FileSize
22.38 KB
2.13 KB

addressed #7
Currently core does not document result of render array

alexpott’s picture

+++ b/core/modules/user/lib/Drupal/user/Tests/UserRoleAdminTest.phpundefined
@@ -37,38 +37,38 @@ function testRoleAdministration() {
-    $this->drupalPost("admin/people/roles/edit/{$role->id()}", $edit, t('Save role'));
-    $this->assertText(t('The role has been renamed.'), 'The role has been renamed.');
+    $edit = array('label' => $role_name);
+    $this->drupalPost("admin/people/roles/manage/{$role->id()}", $edit, t('Save role'));

Why have we changed the url from admin/people/roles/edit/{$role->id()} to admin/people/roles/manage/{$role->id()}?

I guess that's because you can also delete the role from this page? But is that necessary now that you can delete from the table using the operations column?

andypost’s picture

@alexpott This is a common pattern for core, so this issue is a proper place to make this conversion. You can see the same in other issues when ConfigListController involved

sun’s picture

What @andypost said. Wherever we can and technically possible, we should use the de facto standard URL pattern.

This looks pretty good already. However, I found a range of problems in reviewing and testing this, which need to be addressed:

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.php
@@ -0,0 +1,95 @@
+      '#machine_name' => array(
+        'exists' => 'user_role_load',
+        'source' => array('label'),

Didn't we default the 'source' to 'label' already?

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.php
@@ -0,0 +1,95 @@
+    // Adjust actions.
+    $actions['submit']['#value'] = !$entity->isNew() ? t('Save role') : t('Add role');
+    $actions['delete']['#value'] = t('Delete role');

Do we still need these two?

Happy to defer it to a separate issue, but I don't really understand why we don't have the regular "Save" and "Delete" buttons here... (I perfectly understand it from a UX perspective, but these one-off adjustments are disturbing)

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.php
@@ -0,0 +1,95 @@
+    if ($entity->save() == SAVED_UPDATED) {
+      drupal_set_message(t('Role %label has been renamed.', array('%label' => $entity->label())));
...
+    }
+    else {
+      drupal_set_message(t('Role %label has been added.', array('%label' => $entity->label())));

The conditional control structure logic for the save status is not exactly right here. It seems it wasn't right before already.

When just hitting the "Save role" button without any changes, the submit handler prints "has been renamed".

I believe we should at least change this to "has been updated".

+++ b/core/modules/user/lib/Drupal/user/RoleListController.php
@@ -0,0 +1,140 @@
+    $operations['permissions'] = array(
+      'title' => t('Edit permissions'),
...
+      'weight' => 5,

The "Edit" operation should be the default operation.

People should not look at the isolated permissions of a single role by default. Security always needs to be evaluated for all roles in combination, because users have multiple roles.

The "Edit permissions" operation is to roles like the "List terms" operation is to taxonomy vocabularies — i.e., the operation is not about the direct entity at hand.

I still believe we badly need to split the dropbuttons in these cases into two dropbuttons; one for the direct entity at hand (role/vocabulary), and another one for the related/inherited entities (permissions/terms).

+++ b/core/modules/user/lib/Drupal/user/RoleListController.php
@@ -0,0 +1,140 @@
+    // Build-in roles could not be deleted.
+    if (in_array($entity->id(), array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
+      unset($operations['delete']);
+    }

In testing this, I get a "Disable" operation for each role.

1) Unless I missed a recent change, then user roles cannot be disabled.

2) In any case, the built-in roles cannot be disabled.

Also, typo in code comment:
English irregular past tense: After building something in, it is built and "built-in".

+++ b/core/modules/user/user.admin.inc
@@ -849,208 +849,31 @@ function theme_user_permission_description($variables) {
-function user_admin_role($form, $form_state, $role) {
...
+  $role = entity_create('user_role', array());
+  return entity_get_form($role);

API change to be stated in the change notice:

The $form_id changed from user_admin_role to user_role_form.

+++ b/core/modules/user/user.module
@@ -961,23 +957,39 @@ function user_menu() {
+  $items['admin/people/roles/add'] = array(
+    'title' => 'Add role',
+    'page callback' => 'user_admin_role_add',
+    'access arguments' => array('administer permissions'),
+    'type' => MENU_LOCAL_ACTION,
+    'weight' => 1,
+    'file' => 'user.admin.inc',
+  );

Hm. I fear the addition of a local action and removal of the embedded add form belongs into a separate issue, and requires sign-off and testing from the usability team.

If you manage to pull in @Bojhan or @yoroy into this issue and get a sign-off for this change, then we can do it right here.

Otherwise, let's do the straight conversion to entity controllers here only.

+++ b/core/modules/user/user.module
@@ -961,23 +957,39 @@ function user_menu() {
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+    'weight' => -10,

The weight can be omitted for default local tasks now.

+++ b/core/modules/user/user.module
@@ -961,23 +957,39 @@ function user_menu() {
+  $items['admin/people/roles/manage/%user_role/delete'] = array(
     'title' => 'Delete role',
...
+    'type' => MENU_LOCAL_TASK,

This task appears as tab but should not. It needs a context of MENU_CONTEXT_INLINE.

sun’s picture

Tagging to loop in the usability team.

To clarify why this technical patch needs UX review/sign-off:

  1. This patch attempts to remove the embedded form for creating a new user role from the page that is listing user roles.
  2. Instead, it adds an "Add role" local action link to that listing page, which points to the user role add/edit form page.
  3. In other words, the embedded role addition form is nixed altogether and replaced with the de-facto standard UI pattern we're using everywhere else.
  4. You can manually test this patch very easily via simplytest.me.
yoroy’s picture

Although the current way is a nice and quick way to add a new role, I agree we should make this more predictable and thus apply the standard pattern. Go ahead.

(Gee, quite a lot of help text on this page! #1952064: Shorten help text on User Roles page )

andypost’s picture

FileSize
22.25 KB
4.51 KB

New patch addressed #11 changes

Status: Needs review » Needs work

The last submitted patch, 1872870-roles-14.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
22.27 KB
2.63 KB

Fix tests

andypost’s picture

#16: 1872870-roles-16.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs usability review, +Usability

The last submitted patch, 1872870-roles-16.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
22.2 KB
754 bytes

Removed Delete tab

andypost’s picture

FileSize
22.5 KB
1.05 KB
tim.plunkett’s picture

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.phpundefined
@@ -0,0 +1,91 @@
+  public function form(array $form, array &$form_state, EntityInterface $entity) {
+    $form = parent::form($form, $form_state, $entity);

I think this now has to go at the bottom of the method. I've seen that mentioned in other issues.

Otherwise, this is perfect! Awesome work @andypost.

andypost’s picture

FileSize
22.48 KB
889 bytes

Sure, to allow language and other play with form items!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

+1! Thanks.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs usability review, -Usability

The last submitted patch, 1872870-roles-22.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review, +Usability

#22: 1872870-roles-22.patch queued for re-testing.

decafdennis’s picture

In #22, the role labels link to admin/people/roles/manage/ROLE. This path happens to render the role editing form, so it's not necessarily broken. However, the role label being a link is inconsistent with other pages, such as taxonomy and content type administration pages.

I propose the role labels are not links. Otherwise patch looks great!

Status: Needs review » Needs work

The last submitted patch, interdiff.do-no-test.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
22.48 KB

@naquah Your patch makes no sense! Bot sometimes gives wrong testing results and as you see #22 passes tests.

Link should be a render array and point to role edit form.

Bojhan’s picture

@tim,plunkett Is right in that this shouldn't be linked. We tend to avoid linking to things that aren't "pages" like content items, or listings. Since this isn't and all it does is go to edit, just using a dropbutton for that would suffice.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

So RTBC #26 patch by @naquah

decafdennis’s picture

@Bojhan, andypost: thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Reviewing patch in #26... needs work for a new docs standard. NOTE the patch to work with is: https://drupal.org/files/1872870-roles-24.patch

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/Role.phpundefined
@@ -57,4 +61,17 @@ class Role extends ConfigEntityBase {
+   * Overrides \Drupal\Core\Entity\Entity::uri().

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.phpundefined
@@ -0,0 +1,89 @@
+   * Overrides EntityFormController::form().
...
+   * Overrides EntityFormController::actions().
...
+   * Overrides EntityFormController::save().
...
+   * Overrides EntityFormController::delete().

+++ b/core/modules/user/lib/Drupal/user/RoleListController.phpundefined
@@ -0,0 +1,139 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Overrides \Drupal\Core\Entity\EntityListController::buildHeader().
...
+   * Overrides \Drupal\Core\Entity\EntityListController::getOperations().
...
+   * Overrides \Drupal\Core\Entity\EntityListController::buildRow().
...
+   * Overrides \Drupal\Core\Entity\EntityListController::render().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

{@inheritdoc}

andypost’s picture

Status: Needs work » Needs review
FileSize
21.85 KB
23.35 KB
5.56 KB

re-roll to fix docs
While on tris fixed a controller doc-blocks
if no reason or out of scope the commiter will decide (the patch with "no-comments" skiping the controller changes

alexpott’s picture

Adding interdiff of 1872870-roles-33.patch and 1872870-roles-33-no-comments.patch to show what @andypost is going on about in #33...

Basically he has also fixed \Drupal\user\RoleStorageController for the latest coding standards. Which I think is fine.

andypost’s picture

FileSize
23.29 KB

Re-roll after #1807042: Reorganizie entity storage/list/form/render controller annotation keys
doc-block comments for storage in patch

Status: Needs review » Needs work

The last submitted patch, 1872870-roles-35.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
23.38 KB
642 bytes

Missed the comma

andypost’s picture

tagged

andypost’s picture

FileSize
23.39 KB

re-roll

Status: Needs review » Needs work

The last submitted patch, 1872870-roles-38.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
23.31 KB
2.03 KB

Re-roll with new EntityFormController

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#41 looks good for me

alexpott’s picture

Title: Implement a RoleListController and RoleFormController » Change notice: Implement a RoleListController and RoleFormController
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -Configurables

Committed 0bdc4cc and pushed to 8.x. Thanks!

Now we just need to convert all this to WSCII :)

And I guess we need a change notice to explain the parts of this change that affect site builders.

andypost’s picture

Filed follow-up #1992428: Convert Roles management to a Controller and delete form conversion issue #1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route

The change notice should point about:
Form builders:
user_admin_roles() => user_admin_roles_form() and introduced local action to add role
user_admin_role() => uses EntityFormController

theme_user_admin_roles() removed

Path normalized to follow core standard admin/people/roles/manage

andypost’s picture

Status: Active » Needs review
Issue tags: -Needs usability review +Needs change record, +Configurables
FileSize
902 bytes
772 bytes

Updated http://drupal.org/node/1818734 and http://drupal.org/node/1734556
Probably it needs a separate change notice for Form name changes and path

Also here follow-up patch

andypost’s picture

FileSize
981 bytes
850 bytes

needs stronger condition

andypost’s picture

This code added to follow-up issue #1992428-6: Convert Roles management to a Controller

andypost’s picture

Issue tags: -Needs change record
amateescu’s picture

Title: Change notice: Implement a RoleListController and RoleFormController » [Followup] Implement a RoleListController and RoleFormController
Priority: Critical » Normal

The change notice looks good to me, demoting to normal for the followup.

amateescu’s picture

Issue summary: View changes

Added related

andypost’s picture

Title: [Followup] Implement a RoleListController and RoleFormController » Implement a RoleListController and RoleFormController
Status: Needs review » Fixed

Follow-up to improve test merged in upcoming #1992428-9: Convert Roles management to a Controller

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

Anonymous’s picture

Issue summary: View changes

follow-up