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

mgr/dashboard: Role management from the UI #23409

Merged
merged 1 commit into from Sep 5, 2018

Conversation

Projects
None yet
5 participants
@ricardoasmarques
Member

ricardoasmarques commented Aug 3, 2018

This PR adds the role management feature into the UI.

screenshot from 2018-08-03 10-41-47

screenshot from 2018-08-03 10-42-01

@votdev

This comment has been minimized.

Contributor

votdev commented Aug 6, 2018

What about implementing the system role column in the roles tab like this in RGW?
auswahl_009

@votdev

This comment has been minimized.

Contributor

votdev commented Aug 6, 2018

I suggest to use a details page with the icons fa-check-square(-o)? and fa-square-o.
auswahl_011
auswahl_010

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-role-management branch from 622d5e2 to ece58f4 Aug 7, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Aug 7, 2018

@votdev addressed your comments

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-role-management branch from ece58f4 to 32cb9c0 Aug 13, 2018

@LenzGr LenzGr added the needs-review label Aug 28, 2018

@LenzGr LenzGr requested review from tspmelo, Devp00l, votdev and Exotelis Aug 28, 2018

@tspmelo

Please run prettier and increase the code coverage.

for="name">Description
</label>
<div class="col-sm-9">
<input class="form-control"

This comment has been minimized.

@tspmelo

tspmelo Aug 28, 2018

Contributor

It seems this input is being focused when you open the page. Would be nice if the name was the 1st input focused.

() => {
this.getRoles();
this.modalRef.hide();
this.notificationService.show(

This comment has been minimized.

@tspmelo

tspmelo Aug 28, 2018

Contributor

Please change the notifications text.
It would be nice if these notifications had the same wording as the ones used for tasks.
p.e.:
image
image

@votdev votdev added the needs-rebase label Aug 31, 2018

<thead>
<tr>
<th></th>
<th class="text-center">read</th>

This comment has been minimized.

@votdev

votdev Aug 31, 2018

Contributor

Please make the column header text (Read, Create, Update, Delete) uppercase.

<div class="col-sm-9">
<input class="form-control"
type="text"
placeholder="Description..."

This comment has been minimized.

@votdev

votdev Aug 31, 2018

Contributor

i18n-placeholder is missing

<div class="col-sm-9">
<input class="form-control"
type="text"
placeholder="Name..."

This comment has been minimized.

@votdev

votdev Aug 31, 2018

Contributor

i18n-placeholder is missing

createForm() {
this.roleForm = new CdFormGroup({
name: new FormControl('', {

This comment has been minimized.

@votdev

votdev Aug 31, 2018

Contributor

Consider to use the CdValidators.unique in the role add formular to check if the role name already exists. See #23802 and https://github.com/votdev/ceph/blob/631a9c9844225ec47bb175a893da39795ec50e7b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-user-form/rgw-user-form.component.ts#L54 how to implement it.

this.notificationService.show(
NotificationType.success,
`Role "${roleFormModel.name}" has been updated.`,
'Edit Role'

This comment has been minimized.

@votdev

votdev Aug 31, 2018

Contributor

Remove this line according to hacking.rst and

* @param {string} [message] The message to be displayed. Note, use this field
.

this.notificationService.show(
NotificationType.success,
`Role "${roleFormModel.name}" has been created.`,
'Create Role'

This comment has been minimized.

@votdev

votdev Aug 31, 2018

Contributor

Remove this line according to hacking.rst and

* @param {string} [message] The message to be displayed. Note, use this field
.

() => {
this.notificationService.show(
NotificationType.success,
`Role "${roleFormModel.name}" has been created.`,

This comment has been minimized.

@votdev

votdev Aug 31, 2018

Contributor

Use Created role "${roleFormModel.name}" according to hacking.rst here.

() => {
this.notificationService.show(
NotificationType.success,
`Role "${roleFormModel.name}" has been updated.`,

This comment has been minimized.

@votdev

votdev Aug 31, 2018

Contributor

Use Updated role "${roleFormModel.name}" according to hacking.rst here.

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-role-management branch from 32cb9c0 to f5f8361 Sep 3, 2018

@ricardoasmarques ricardoasmarques changed the title from [After #23322] mgr/dashboard: Role management from the UI to mgr/dashboard: Role management from the UI Sep 3, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Sep 3, 2018

@votdev @tspmelo I've addressed your comments

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-role-management branch from f5f8361 to 1ac923e Sep 3, 2018

<thead>
<tr>
<th></th>
<th class="text-center">read</th>

This comment has been minimized.

@votdev

votdev Sep 4, 2018

Contributor

I suggest to start the text with uppercase: Read/Write/Update/Delete

@@ -0,0 +1,3 @@
export enum RoleFormMode {
editing = 'editing'

This comment has been minimized.

@votdev

votdev Sep 4, 2018

Contributor

I suggest to transform this variable from string -> boolean.

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Sep 4, 2018

Member

For now, I'll keep it as string for consistency with user-form.*

<thead>
<tr>
<th></th>
<th class="text-center">READ</th>

This comment has been minimized.

@votdev

votdev Sep 4, 2018

Contributor

Another solution how to write the column header text. Please use the Read/Write/... style from the details page or adapt the text there and use READ/WRITE/... Finally the typing in the column headers should be the same.

</div>
</div>
<cd-role-details cdTableDetail
[selection]="selection"

This comment has been minimized.

@votdev

votdev Sep 4, 2018

Contributor

Indention is incorrect

() => {
this.getRoles();
this.modalRef.hide();
this.notificationService.show(NotificationType.success, `Deleted Role '${role}'`);

This comment has been minimized.

@votdev

votdev Sep 4, 2018

Contributor

I'm not sure how we handle this, but if we use correct english it should be Deleted role .... Maybe another topic for our style guide.

const name = this.selection.first().name;
this.modalRef.content.setUp({
metaType: 'Role',
pattern: `${name}`,

This comment has been minimized.

@votdev

votdev Sep 4, 2018

Contributor

This is obsolete if PR #23897 is merged before yours.

}
exists(name: string): Observable<boolean> {
return this.list().pipe(

This comment has been minimized.

@votdev

votdev Sep 4, 2018

Contributor

Please add at least one unit test for this service method.

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-role-management branch from 1ac923e to 65b839d Sep 4, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Sep 4, 2018

@votdev I've addressed your comments

@callithea callithea removed the needs-rebase label Sep 4, 2018

@votdev

votdev approved these changes Sep 5, 2018

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Sep 5, 2018

A minor nit: would it be possible to change the toplevel menu entry from "Users" to "Users and Roles" or "Users / Roles"?

roles-screen

I see that the breadcrumb changes to "Administration -> Users -> Roles" when I click the Roles tab", but I wonder if at least the toplevel menu could mention both options right away?

@votdev

This comment has been minimized.

Contributor

votdev commented Sep 5, 2018

Naming the menu 'Access management' is another option.

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Sep 5, 2018

When editing an existing user, I entered a few characters in the first password field and then removed them again, as I did not want to change the existing password. However, I was not able to proceed, as the dialogue insisted that the "Password confirmation doesn't match the password." - this does not happen if I make the same change to the "Confirm password" change.

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Sep 5, 2018

When editing an existing user, I entered a few characters in the first password field and then removed them again, as I did not want to change the existing password. However, I was not able to proceed, as the dialogue insisted that the "Password confirmation doesn't match the password." - this does not happen if I make the same change to the "Confirm password" change.

This PR is for Role management only, if we have an issue in user management I suggest to fix it in a separate issue / PR.

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Sep 5, 2018

Naming the menu 'Access management' is another option.

It might, but that is a bit too vague in my opinion.

Also, I noticed that the cog icon currently does not provide any tooltip when I hover above it with the mouse pointer. It should display "Dashboard Settings" (but that probably deserves a separate PR). I filed issue#35686 to keep track of this.

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Sep 5, 2018

This PR is for Role management only, if we have an issue in user management I suggest to fix it in a separate issue / PR.

Good point; I submitted issue#35685 about this.

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Sep 5, 2018

A minor nit: would it be possible to change the toplevel menu entry from "Users" to "Users and Roles" or "Users / Roles"?

@LenzGr We will have more tabs here (e.g., the SSO settings), so I agree with @votdev that a more generic menu entry is a better approach (instead of adding all tab names to the toplevel menu item).

Also, now that I've revisited this topic, I no longer think that "Roles" should be a child of "Users", and probably it doesn't make sense to have Administration in the breadcrumb.

My new proposal is the following:

Users
screenshot from 2018-09-05 11-27-07

Roles
screenshot from 2018-09-05 11-27-23

@LenzGr @votdev does this works for you?

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Sep 5, 2018

@LenzGr @votdev does this works for you?

This works for me. Thank you!

mgr/dashboard: Role management from the UI
Fixes: https://tracker.ceph.com/issues/24447

Signed-off-by: Ricardo Marques <rimarques@suse.com>

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-role-management branch from 65b839d to 5d2b0eb Sep 5, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Sep 5, 2018

@LenzGr @votdev I've updated this PR with the breadcrumb improvement discussed in the previous comment.

The requested changes have been addressed.

@LenzGr

LenzGr approved these changes Sep 5, 2018

LGTM - thank you!

@LenzGr LenzGr merged commit 2c1f8b6 into ceph:master Sep 5, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment