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

#2762 guard add users button on team details view #3736

Merged

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented May 19, 2020

🔩 Description: What code changed, and why?

Before, the Add Users button on the team details page has always been shown.

With this change, it will only be shown of the user has got sufficient privileges to actually do that.

Definition of done

  • hide the add users button if they don't have permission to add users to a team
  • if they happen to navigate to the add users page of a team, display this message on the page: It looks like you don't have permission to add users to this team. Reach out to your administrator or contact Chef Support for help. with Chef Support being an open in new tab link to our support page
  • changed/added tests pass

⛓️ Related Resources

#2762

👟 How to Build and Test the Change

  • rebuild automate-ui in your preferred way
  • sign in as admin, go to a team's details:

Screenshot 2020-05-19 at 15 58 15

Screenshot 2020-05-19 at 15 59 20

image

@srenatus srenatus added WIP automate-ui auth-team anything that needs to be on the auth team board labels May 19, 2020
@srenatus srenatus self-assigned this May 19, 2020
@srenatus srenatus force-pushed the sr/issue-2762/guard-add-users-button-on-team-details-view branch 2 times, most recently from 5732098 to 7d18381 Compare May 19, 2020 15:04
@srenatus srenatus marked this pull request as ready for review May 19, 2020 15:07
@srenatus
Copy link
Contributor Author

⚠️ the "no access" page is wrong, there should not be a heading and buttons.

@srenatus srenatus force-pushed the sr/issue-2762/guard-add-users-button-on-team-details-view branch from 7d18381 to 5c71b3d Compare May 19, 2020 15:10
@srenatus
Copy link
Contributor Author

I've tried something... but now the close button (top right) is gone:

image

I suppose I could use some help here 😅

@msorens
Copy link
Contributor

msorens commented May 20, 2020

There are several ways to resolve your "no permissions" page issue. I have pushed a commit with what is probably the simplest approach.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
@srenatus srenatus force-pushed the sr/issue-2762/guard-add-users-button-on-team-details-view branch from 6835196 to 491bdd7 Compare May 20, 2020 09:03
srenatus and others added 4 commits May 20, 2020 11:07
…eam-details

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Leverage the existing full-page rendering.
1. Setting the confirm button text to empty suppresses the lower buttons.
2. Use the same close handler to return to team details on exit.
3. A bit of a kludge: using the `heading` attribute to trigger the full page settings.

Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
@srenatus srenatus force-pushed the sr/issue-2762/guard-add-users-button-on-team-details-view branch from 491bdd7 to 0cd1e4a Compare May 20, 2020 09:08
@srenatus srenatus removed the WIP label May 20, 2020
@srenatus
Copy link
Contributor Author

@msorens thanks again -- I've updated the preamble, this should be good to go now.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
@srenatus
Copy link
Contributor Author

✔️ Verified "add users" request payload (no unknown fields sent) and updated the API docs example payloads.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
@susanev susanev added the ui label May 20, 2020
Copy link
Contributor

@susanev susanev left a comment

Choose a reason for hiding this comment

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

works great, thank you!!

@@ -1,11 +1,11 @@
<app-authorized [allOf]="getPermissionsPath" [overrideVisible]="overridePermissionsCheck">
<app-authorized [allOf]="getPermissionsPath">
<ng-container *ngIf="showEmptyMessage">
<div class="empty-state">
<p>Add some users to get started!</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed I still see this message when I'm logged in as the test user and I navigate to a team without any users
Screen Shot 2020-05-20 at 4 52 35 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch, we should leverage the similar msg we display on projects. ill add that here later tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ I'm looking into this now (Tuesday morning)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

❓ I'm not sure I see what "similar msg" you've been referring to, @susanev.

When browsing a project's details as the test user, I find this, no message at all:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💡 You've meant this one:

image

I'll draft a similar message here that we can discuss later :)

Copy link
Contributor

Choose a reason for hiding this comment

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

the messages look good to me, thank you everyone for the assists!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msorens good catches! Let's tackle those separately, please. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[...] but for the purposes of this PR we might want to address why the Users list is blank and is not showing the message (with or without my commit).

👍 I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msorens can't reproduce. I've created a policy giving my user ALLOW on *, and DENY on iam:users:create, and see this:
image

Copy link
Contributor

@msorens msorens May 28, 2020

Choose a reason for hiding this comment

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

@sr You exercised a different test case than the one I specified:

...when logged in as a user having no permissions then navigating directly to each page

(No strong preference whether that gets fixed in this PR, though.)

Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

Looks good, although as the test user, I noticed one weird behavior when I was on a team page that did not already have users. (see comment for screenshot)

Signed-off-by: michael sorens <msorens@chef.io>
It is occasionally "more correct" than the standard test runner

Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
Copy link
Contributor

@msorens msorens left a comment

Choose a reason for hiding this comment

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

Pushed a few minor commits, including "fixes" for unit tests caught by wallaby that were not noticed by the regular test runner.
Also thought I found an odd bug. I was examining the rendered permissions in <app-authorized> for the two paths to get to the user table. Notice that the first one, for admins team, correctly shows a parameterized call BUT truncates the team id to "ad" in the <app-authorized> element. Turns out there is no bug in our code--rather it is a "bug" in Chrome DevTools: it truncates all attributes to 30 characters (not just on <app-authorized>) !!
image

srenatus and others added 4 commits May 26, 2020 10:03
The message to be shown when the user cannot add users is TBD.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Let the body of the `<app-user-table>` provide the text
of a "no-users-present-and-no-permissions-to-add-them" message.

Signed-off-by: michael sorens <msorens@chef.io>
@srenatus srenatus merged commit 22e5d7d into master May 29, 2020
@chef-expeditor chef-expeditor bot deleted the sr/issue-2762/guard-add-users-button-on-team-details-view branch May 29, 2020 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth-team anything that needs to be on the auth team board automate-ui ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants