Tags policy permissions #2738
Tags policy permissions #2738
Conversation
CDimonaco
added
enhancement
CDimonaco
force-pushed
the
tags_policy_permissions
branch
from
e83f663 to
92ef013
Compare
There was a problem hiding this comment.
Great work!
Just some few comments (some are personal opinions).
Besides that 2 things:
-
In the acceptance criteria, we have the entry of
remove cross from existing tags, which you didn't implement. We can merge now, and wait to see what @jagabomb says. Changing in next PR is simple -
I don't really like the tendency of adding new "corner case" tests (like this, the forbidden disabled scenario), in the top of the test files. The test files should have some logical order. In my opinion, the normal or even first written tests should be on top, and the new ones after that. I find it pretty "ilogical" to add the new test cases on the top of the file, when they are the last things we implemented. My OCD I guess hehe
| @@ -13,31 +13,6 @@ defmodule TrentoWeb.V1.HostControllerTest do | |||
| setup :setup_api_spec_v1 | |||
| setup :setup_user | |||
|
|
|||
| describe "forbidden routes" do | |||
There was a problem hiding this comment.
Let's remove this change. I have already done it in other PR, and it will conflict otherwise.
There was a problem hiding this comment.
@CDimonaco I mean, you didn't do any change here as far as I see, just move the describe to the bottom, which I did in the latest PR i merged some minutes ago
| @@ -16,6 +16,9 @@ const middlewares = []; | |||
| const mockStore = configureStore(middlewares); | |||
|
|
|||
| export const defaultInitialState = { | |||
| user: { | |||
There was a problem hiding this comment.
This will conflict with my PR as well 🙈
CDimonaco
force-pushed
the
tags_policy_permissions
branch
from
f6e29aa to
aeafe74
Compare
| @@ -13,31 +13,6 @@ defmodule TrentoWeb.V1.HostControllerTest do | |||
| setup :setup_api_spec_v1 | |||
| setup :setup_user | |||
|
|
|||
| describe "forbidden routes" do | |||
There was a problem hiding this comment.
@CDimonaco I mean, you didn't do any change here as far as I see, just move the describe to the bottom, which I did in the latest PR i merged some minutes ago
assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx
Outdated
Show resolved
Hide resolved
CDimonaco
force-pushed
the
tags_policy_permissions
branch
from
aeafe74 to
3500cd9
Compare
CDimonaco
force-pushed
the
tags_policy_permissions
branch
from
3500cd9 to
2f5e6bf
Compare
| ); | ||
|
|
||
| expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50'); | ||
| // grab the X |
| @@ -183,4 +188,60 @@ defmodule TrentoWeb.V1.TagsControllerTest do | |||
| assert 404 == conn.status | |||
| end | |||
| end | |||
|
|
|||
| describe "forbidden actions" do | |||
| test "should not return forbidden on any controller action if the user have the right ability for the tag resource", | |||
There was a problem hiding this comment.
I guess it is a typo if the user have --> if the user has
Description
Permissions on tags creation/delete
Abilities
all:host_tags- All operations on host tagsall:cluster_tags- All operations on cluster tagsall:database_tags- All operations on database tagsall:sap_systems_tags- All operations on sap_systems tagsThe Tags component prevent the click on delete/add tag functionality when the abilities are not compatible with the one configured in the component.
How was this tested?
Automated tests