Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Role column to admin user grid #10891

Closed
wants to merge 3 commits into from

Conversation

mpchadwick
Copy link
Contributor

@mpchadwick mpchadwick commented Sep 14, 2017

Description

image

Adds a column to the admin user grid to show the users role

This can be useful for example to see all the users with "admin" privileges on a store with many users. Without this, you need to click into each manually

See: #9557

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

)->join(
['detail_role' => $this->getTable('authorization_role')],
'user_role.parent_id = detail_role.role_id',
['role_name']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will affect every user collection instantiated, isn't it?

Something like https://magento.stackexchange.com/a/124204, a separate collection just for grid, looks like a better option. Not sure if there is to way do it in more declarative way.

Just my 2 cents, before proceeding please wait for somebody to actually confirm such feature fits well into core.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the JOINs will happen on every collection load. This was based on the sales rule grid

<argument name="dataSource" xsi:type="object">Magento\SalesRule\Model\ResourceModel\Rule\Quote\Collection</argument>

https://github.com/magento/magento2/blob/develop/app/code/Magento/SalesRule/Model/ResourceModel/Rule/Quote/Collection.php#L10

https://github.com/magento/magento2/blob/develop/app/code/Magento/SalesRule/Model/ResourceModel/Rule/Collection.php#L279-L288

Not sure what the concern would be. The performance cost should be negligible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I searched for the usages of this Collection, it is not used in any performance-critical flows. Join should be fine here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrann glad to know, thanks! Hope you checked EE/B2B codebases as well.

Is it correct that such approach with _initSelect is up-to-date? Like, there is no declarative way to declare join for the grid etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrann @orlangur also note that there are indexes on both the columns used in the JOIN so I really wouldn't be worried about perf.

@vrann
Copy link
Contributor

vrann commented Sep 18, 2017

please merge with the latest from develop

@vrann
Copy link
Contributor

vrann commented Sep 18, 2017

@mpchadwick ^^

@mpchadwick
Copy link
Contributor Author

@vrann Should be merge develop back in tonight hopefully.

@mpchadwick
Copy link
Contributor Author

@vrann I've merged develop back to this branch.

@vrann vrann self-assigned this Sep 19, 2017
@vrann vrann added this to the September 2017 milestone Sep 19, 2017
@vrann vrann added the develop label Sep 19, 2017
@mpchadwick
Copy link
Contributor Author

@vrann Just pushed an update to fix visibility on _initSelect. Viewing Locked Users page in admin was resulting in 500 error. Noticed that while looking at the Travis results.

@vrann
Copy link
Contributor

vrann commented Sep 25, 2017

@mpchadwick thank you, I also fixed that internally.

Another issue needed to resolve was with the b2b version crashing when filtered by user_id column: now it is ambiguous which table user_id should come from.

magento-team pushed a commit that referenced this pull request Sep 27, 2017
@vrann
Copy link
Contributor

vrann commented Sep 28, 2017

@mpchadwick thank you, the feature is successfully accepted. It is not indicated as merge because we did the same commit.

@vrann vrann closed this Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants