Addressing review feedbacks

trento-project · Jul 3, 2024 · 2f5e6bf · 2f5e6bf
1 parent c162b4a
commit 2f5e6bf
Show file tree

Hide file tree

Showing 9 changed files with 128 additions and 154 deletions.
diff --git a/assets/js/common/Tags/Tags.jsx b/assets/js/common/Tags/Tags.jsx
@@ -16,7 +16,7 @@ const tagValidationDefaultMessage = (
   </>
 );
 
-function TagDeleteButton({ onClick, disabled }) {
+function ExistingTag({ onClick, disabled }) {
   return (
     <span
       aria-hidden="true"
@@ -101,7 +101,7 @@ function Tags({
             permitted={tagDeletionPermittedFor}
             tooltipWrap
           >
-            <TagDeleteButton
+            <ExistingTag
               onClick={() => {
                 const newTagsList = renderedTags.reduce(
                   (acc, current) => (current === tag ? acc : [...acc, current]),

diff --git a/assets/js/pages/ClusterDetails/ClustersList.test.jsx b/assets/js/pages/ClusterDetails/ClustersList.test.jsx
@@ -34,11 +34,9 @@ describe('ClustersList component', () => {
       const state = {
         ...cleanInitialState,
         clustersList: {
-          clusters: [].concat(
-            clusterFactory.buildList(1, {
-              tags: [{ value: 'Tag2' }, { value: 'Tag1' }],
-            })
-          ),
+          clusters: clusterFactory.buildList(1, {
+            tags: [{ value: 'Tag2' }, { value: 'Tag1' }],
+          }),
         },
         user: {
           abilities: [{ name: 'all', resource: 'a_resource' }],

diff --git a/assets/js/pages/DatabasesOverview/DatabasesOverview.jsx b/assets/js/pages/DatabasesOverview/DatabasesOverview.jsx
@@ -20,10 +20,10 @@ function DatabasesOverview({
   databases,
   databaseInstances,
   loading,
+  userAbilities,
   onTagAdd,
   onTagRemove,
   onInstanceCleanUp,
-  userAbilities,
 }) {
   const [searchParams, setSearchParams] = useSearchParams();
   const [cleanUpModalOpen, setCleanUpModalOpen] = useState(false);

diff --git a/assets/js/pages/HostsList/HostsList.test.jsx b/assets/js/pages/HostsList/HostsList.test.jsx
@@ -23,36 +23,6 @@ import { filterTable, clearFilter } from '@common/Table/Table.test';
 import HostsList from './HostsList';
 
 describe('HostsLists component', () => {
-  describe('tag operations', () => {
-    it('should disable tag creation and deletion if the user abilities are not compatible', async () => {
-      const host1 = hostFactory.build({
-        agent_version: '1.0.0',
-        tags: [{ value: 'Tag1' }, { value: 'Tag2' }],
-      });
-      const state = {
-        ...defaultInitialState,
-        hostsList: {
-          hosts: [host1],
-        },
-        user: {
-          abilities: [{ name: 'all', resource: 'a_resource' }],
-        },
-      };
-
-      const [StatefulHostsList] = withState(<HostsList />, state);
-
-      renderWithRouter(StatefulHostsList);
-
-      expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50');
-      // grab the X
-      expect(
-        screen.queryByText('Tag1').children.item(0).children.item(0)
-      ).toHaveClass('opacity-50');
-      expect(
-        screen.queryByText('Tag2').children.item(0).children.item(0)
-      ).toHaveClass('opacity-50');
-    });
-  });
   describe('list content', () => {
     [
       {
@@ -462,4 +432,34 @@ describe('HostsLists component', () => {
       );
     });
   });
+  describe('tag operations', () => {
+    it('should disable tag creation and deletion if the user abilities are not compatible', async () => {
+      const host1 = hostFactory.build({
+        agent_version: '1.0.0',
+        tags: [{ value: 'Tag1' }, { value: 'Tag2' }],
+      });
+      const state = {
+        ...defaultInitialState,
+        hostsList: {
+          hosts: [host1],
+        },
+        user: {
+          abilities: [{ name: 'all', resource: 'a_resource' }],
+        },
+      };
+
+      const [StatefulHostsList] = withState(<HostsList />, state);
+
+      renderWithRouter(StatefulHostsList);
+
+      expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50');
+      // grab the X
+      expect(
+        screen.queryByText('Tag1').children.item(0).children.item(0)
+      ).toHaveClass('opacity-50');
+      expect(
+        screen.queryByText('Tag2').children.item(0).children.item(0)
+      ).toHaveClass('opacity-50');
+    });
+  });
 });
diff --git a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx
@@ -19,34 +19,6 @@ import SapSystemsOverview from './SapSystemsOverview';
 const userAbilities = [{ name: 'all', resource: 'all' }];
 
 describe('SapSystemsOverviews component', () => {
-  describe('tag operations', () => {
-    it('should disable tag creation and deletion if the user abilities are not compatible', () => {
-      const abilities = [{ name: 'all', resource: 'another_resource' }];
-
-      const sapSystem = sapSystemFactory.build({
-        tags: [{ value: 'Tag1' }, { value: 'Tag2' }],
-      });
-
-      renderWithRouter(
-        <SapSystemsOverview
-          userAbilities={abilities}
-          sapSystems={[sapSystem]}
-          applicationInstances={[]}
-          databaseInstances={[]}
-        />
-      );
-
-      expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50');
-      // grab the X
-      expect(
-        screen.queryByText('Tag1').children.item(0).children.item(0)
-      ).toHaveClass('opacity-50');
-      expect(
-        screen.queryByText('Tag2').children.item(0).children.item(0)
-      ).toHaveClass('opacity-50');
-    });
-  });
-
   describe('overview content', () => {
     it('should display the correct number of SAP systems', () => {
       const sapSystemCount = 3;
@@ -408,4 +380,32 @@ describe('SapSystemsOverviews component', () => {
       );
     });
   });
+
+  describe('tag operations', () => {
+    it('should disable tag creation and deletion if the user abilities are not compatible', () => {
+      const abilities = [{ name: 'all', resource: 'another_resource' }];
+
+      const sapSystem = sapSystemFactory.build({
+        tags: [{ value: 'Tag1' }, { value: 'Tag2' }],
+      });
+
+      renderWithRouter(
+        <SapSystemsOverview
+          userAbilities={abilities}
+          sapSystems={[sapSystem]}
+          applicationInstances={[]}
+          databaseInstances={[]}
+        />
+      );
+
+      expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50');
+      // grab the X
+      expect(
+        screen.queryByText('Tag1').children.item(0).children.item(0)
+      ).toHaveClass('opacity-50');
+      expect(
+        screen.queryByText('Tag2').children.item(0).children.item(0)
+      ).toHaveClass('opacity-50');
+    });
+  });
 });
diff --git a/lib/trento/tags/policy.ex b/lib/trento/tags/policy.ex
@@ -6,16 +6,16 @@ defmodule Trento.Tags.Policy do
   User with the ability all:<resource_type>_tags can perform any operations on the tags of the permitted resource.
 
   Resource type can be one of:
-  host
-  cluster
-  sap_system
-  database
+  - host
+  - cluster
+  - sap_system
+  - database
   """
   @behaviour Bodyguard.Policy
 
   import Trento.Support.PolicyHelper
-  alias Trento.Users.User
   alias Trento.Tags.Tag
+  alias Trento.Users.User
 
   def authorize(action, %User{} = user, %{tag_resource: tag_resource, resource: Tag})
       when action in [:add_tag, :remove_tag],

diff --git a/test/trento/tags/policy_test.exs b/test/trento/tags/policy_test.exs
@@ -10,6 +10,7 @@ defmodule Trento.Tags.PolicyTest do
 
     Enum.each([:cluster, :host, :sap_system, :database], fn tag_resource ->
       assert Policy.authorize(:add_tag, user, %{tag_resource: tag_resource, resource: Tag})
+      assert Policy.authorize(:remove_tag, user, %{tag_resource: tag_resource, resource: Tag})
     end)
   end
 
@@ -21,11 +22,11 @@ defmodule Trento.Tags.PolicyTest do
     end)
   end
 
-  test "should not allow delete_tag action on the resource when the user does not have the right ability" do
+  test "should not allow remove_tag action on the resource when the user does not have the right ability" do
     user = %User{abilities: []}
 
     Enum.each([:cluster, :host, :sap_system, :database], fn tag_resource ->
-      refute Policy.authorize(:delete_tag, user, %{tag_resource: tag_resource, resource: Tag})
+      refute Policy.authorize(:remove_tag, user, %{tag_resource: tag_resource, resource: Tag})
     end)
   end
 

diff --git a/test/trento_web/controllers/v1/host_controller_test.exs b/test/trento_web/controllers/v1/host_controller_test.exs
@@ -384,29 +384,4 @@ defmodule TrentoWeb.V1.HostControllerTest do
       )
     end
   end
-
-  describe "forbidden routes" do
-    test "should return forbidden on any controller action if the user does not have the right permission",
-         %{conn: conn, api_spec: api_spec} do
-      %{id: user_id} = insert(:user)
-      %{id: host_id} = insert(:host)
-
-      conn =
-        conn
-        |> Pow.Plug.assign_current_user(%{"user_id" => user_id}, Pow.Plug.fetch_config(conn))
-        |> put_req_header("content-type", "application/json")
-
-      Enum.each(
-        [
-          post(conn, "/api/v1/hosts/#{host_id}/checks", %{}),
-          post(conn, "/api/v1/hosts/#{host_id}/checks/request_execution", %{})
-        ],
-        fn conn ->
-          conn
-          |> json_response(:forbidden)
-          |> assert_schema("Forbidden", api_spec)
-        end
-      )
-    end
-  end
 end
diff --git a/test/trento_web/controllers/v1/tags_controller_test.exs b/test/trento_web/controllers/v1/tags_controller_test.exs
@@ -10,62 +10,6 @@ defmodule TrentoWeb.V1.TagsControllerTest do
   setup :setup_api_spec_v1
   setup :setup_user
 
-  describe "forbidden actions" do
-    test "should not return forbidden on any controller action if the user have the right ability for the tag resource",
-         %{conn: conn} do
-      %{id: user_id} = insert(:user)
-
-      for tag_resource <- [:host, :sap_system, :cluster, :database] do
-        %{id: ability_id} = insert(:ability, name: "all", resource: "#{tag_resource}_tags")
-        insert(:users_abilities, user_id: user_id, ability_id: ability_id)
-        %{id: resource_id} = insert(tag_resource)
-
-        conn =
-          conn
-          |> Pow.Plug.assign_current_user(%{"user_id" => user_id}, Pow.Plug.fetch_config(conn))
-          |> put_req_header("content-type", "application/json")
-
-        resp =
-          post(conn, "/api/v1/#{tag_resource}s/#{resource_id}/tags", %{
-            "value" => "thetag"
-          })
-
-        assert resp.status == 201
-
-        resp =
-          delete(conn, "/api/v1/#{tag_resource}s/#{resource_id}/tags/thetag")
-
-        assert resp.status == 204
-      end
-    end
-
-    test "should return forbidden on any controller action if the user does not have the right permission",
-         %{conn: conn, api_spec: api_spec} do
-      %{id: user_id} = insert(:user)
-
-      conn =
-        conn
-        |> Pow.Plug.assign_current_user(%{"user_id" => user_id}, Pow.Plug.fetch_config(conn))
-        |> put_req_header("content-type", "application/json")
-
-      for tag_resource <- [:host, :sap_system, :cluster, :database] do
-        Enum.each(
-          [
-            post(conn, "/api/v1/#{tag_resource}s/#{Faker.UUID.v4()}/tags", %{
-              "value" => "thetag"
-            }),
-            delete(conn, "/api/v1/#{tag_resource}s/#{Faker.UUID.v4()}/tags/thetag")
-          ],
-          fn conn ->
-            conn
-            |> json_response(:forbidden)
-            |> assert_schema("Forbidden", api_spec)
-          end
-        )
-      end
-    end
-  end
-
   describe "Tag Validation" do
     test "should decline tag with whitespace", %{conn: conn} do
       conn =
@@ -244,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",
+         %{conn: conn} do
+      %{id: user_id} = insert(:user)
+
+      for tag_resource <- [:host, :sap_system, :cluster, :database] do
+        %{id: ability_id} = insert(:ability, name: "all", resource: "#{tag_resource}_tags")
+        insert(:users_abilities, user_id: user_id, ability_id: ability_id)
+        %{id: resource_id} = insert(tag_resource)
+
+        conn =
+          conn
+          |> Pow.Plug.assign_current_user(%{"user_id" => user_id}, Pow.Plug.fetch_config(conn))
+          |> put_req_header("content-type", "application/json")
+
+        resp =
+          post(conn, "/api/v1/#{tag_resource}s/#{resource_id}/tags", %{
+            "value" => "thetag"
+          })
+
+        assert resp.status == 201
+
+        resp =
+          delete(conn, "/api/v1/#{tag_resource}s/#{resource_id}/tags/thetag")
+
+        assert resp.status == 204
+      end
+    end
+
+    test "should return forbidden on any controller action if the user does not have the right permission",
+         %{conn: conn, api_spec: api_spec} do
+      %{id: user_id} = insert(:user)
+
+      conn =
+        conn
+        |> Pow.Plug.assign_current_user(%{"user_id" => user_id}, Pow.Plug.fetch_config(conn))
+        |> put_req_header("content-type", "application/json")
+
+      for tag_resource <- [:host, :sap_system, :cluster, :database] do
+        Enum.each(
+          [
+            post(conn, "/api/v1/#{tag_resource}s/#{Faker.UUID.v4()}/tags", %{
+              "value" => "thetag"
+            }),
+            delete(conn, "/api/v1/#{tag_resource}s/#{Faker.UUID.v4()}/tags/thetag")
+          ],
+          fn conn ->
+            conn
+            |> json_response(:forbidden)
+            |> assert_schema("Forbidden", api_spec)
+          end
+        )
+      end
+    end
+  end
 end