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

mgr/dashboard: Provide user enable/disable capability #29046

Merged
merged 6 commits into from Aug 2, 2019

Conversation

@p-na
Copy link
Contributor

commented Jul 15, 2019

Enables disabling/enabling of a user. Introduces two new CLI commands to do so, as well as an additional enabled field in the UI.

CLI commands:

ceph dashboard ac-user-disable <user>
ceph dashboard ac-user-enable <user>

UI:

User list:
user-list

Editing a user:
user-edit

The user is logged out if the disabled field changes it's value on the CLI or UI.

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

@p-na p-na requested a review from rjfd Jul 15, 2019

@p-na p-na force-pushed the p-na:wip-pna-disable-user branch from d17643f to 30e6d21 Jul 15, 2019

p-na added a commit to p-na/ceph that referenced this pull request Jul 15, 2019

mgr/dashboard: Fix logout of user when `enabled` field is adapted
Fixes: ceph#29046

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>

p-na added a commit to p-na/ceph that referenced this pull request Jul 15, 2019

mgr/dashboard: Add `enabled` field to frontend code
Fixes: ceph#29046

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>

p-na added a commit to p-na/ceph that referenced this pull request Jul 15, 2019

mgr/dashboard: Create property for `enabled` field in backend
Fixes: ceph#29046

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>

p-na added a commit to p-na/ceph that referenced this pull request Jul 15, 2019

mgr/dashboard: Add documentation
Fixes: ceph#29046

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>
@p-na

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@rjfd I wonder if I need to adapt the VERSION field of AccessControlDB. And could you also have a look at this tests, please?

@p-na

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

For readers which weren't in the call: We've talked about this and the version field needs to be adapted as well as a migration needs to be written for version 1 to version 2.

@p-na p-na force-pushed the p-na:wip-pna-disable-user branch 3 times, most recently from b729fba to 60cc1be Jul 18, 2019

@p-na p-na added needs-review and removed DNM labels Jul 19, 2019

@p-na p-na assigned s0nea and unassigned s0nea Jul 19, 2019

@p-na p-na requested review from votdev, epuertat and s0nea Jul 19, 2019

@p-na

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

The backend checks now if the request to disable a user is equal to the user currently logged in and will prevent that a user can disable itself. The fronted does the same. By simply not showing the enable/disable radio buttons, when editing the own user.

@p-na p-na added the feature label Jul 19, 2019

@p-na p-na force-pushed the p-na:wip-pna-disable-user branch from 60cc1be to 7375cc8 Jul 19, 2019


<!-- Enabled -->

This comment has been minimized.

Copy link
@votdev

votdev Jul 22, 2019

Contributor

Please use a common checkbox here, a radio group button does not display more information to the user here.

This comment has been minimized.

Copy link
@p-na

p-na Jul 23, 2019

Author Contributor

It definitely makes sense to have that unified and if we currently use checkboxes in Ceph Dashboard instead of radio group buttons, I should have implemented checkboxes.

But because I personally prefer to have radio buttons instead of standard checkboxes, I'd like to ask you if we could reconsider using them instead of checkboxes for simple on/off toggles.

Material design has a page dedicated to this topic. On that page it is explained when to use checkboxes, radio buttons and switches (I consider two radio buttons in a group of the Bootstrap framework to be a switch). The document says:

Use switches to:

Toggle a single option on or off, on mobile and tablet
Immediately activate or deactivate something

About radio buttons they say:

Radio buttons should be used instead of checkboxes if only one item can be selected from a list.

This leaves some room for interpretation and I know that we do not follow the material design guide, but as I personally think they are doing a great job and that it is a great resource, we could at least give it a thought.

Let me know what you think and please excuse me starting that discussion in this PR.

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 24, 2019

Contributor

Immediately activate or deactivate something

If we follow this guideline, I would only use a switch on checkboxes like the ones in table column filtering. Since those are immediately changing the table.

In the case of forms, we are not immediately changing anything, the change only occur when you click the submit button.

name: this.i18n('Enabled'),
prop: 'enabled',
flexGrow: 1,
cellTemplate: this.userEnabledTpl

This comment has been minimized.

Copy link
@votdev

votdev Jul 22, 2019

Contributor

Plesae use the checkIconTpl from the table component or

pipe: this.booleanTextPipe

in the table column definition. It is not necessary to define a new template.

@p-na p-na requested a review from tspmelo Jul 24, 2019

p-na and others added some commits Jul 11, 2019

mgr/dashboard: frontend: use checkIcon for enabled column in user list
Signed-off-by: Ricardo Dias <rdias@suse.com>
mgr/dashboard: frontend: use checkbox for enabled field in user form
Signed-off-by: Ricardo Dias <rdias@suse.com>

rjfd added some commits Aug 1, 2019

mgr/dashboard: tests: fix test_access_control due to the merge of 79b…
…9021

Signed-off-by: Ricardo Dias <rdias@suse.com>
mgr/dashboard: removing dead code in upgrade user database
Signed-off-by: Ricardo Dias <rdias@suse.com>

@rjfd rjfd force-pushed the p-na:wip-pna-disable-user branch from 7375cc8 to 5664217 Aug 1, 2019

@rjfd rjfd added needs-qa and removed needs-review labels Aug 1, 2019

@rjfd

rjfd approved these changes Aug 1, 2019

Copy link
Contributor

left a comment

lgtm

@rjfd rjfd dismissed stale reviews from votdev and tspmelo Aug 1, 2019

Comments have been addressed

@s0nea

s0nea approved these changes Aug 1, 2019

Copy link
Member

left a comment

LGTM - tested it locally

@callithea

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

QA run failed:
http://pulpito.ceph.com/laura-2019-08-01_21:23:14-rados:mgr-wip-lpaduano-testing-29088-29046-distro-basic-smithi/
due to:
Test failure: test_get (tasks.mgr.dashboard.test_mgr_module.MgrModuleTelemetryTest)
See: https://tracker.ceph.com/issues/41055

@callithea

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

As @rjfd mentioned in the stand-up all relevant dashboard related QA tests passed and the failure is not related to the PR itself, will remove the QA labels.

@rjfd rjfd merged commit 29edead into ceph:master Aug 2, 2019

5 of 6 checks passed

ceph dashboard tests ceph dashboard tests failed
Details
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
Projects
None yet
6 participants
You can’t perform that action at this time.