cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CLOUDSTACK-9663) updateRole should return updated role as json
Date Sat, 10 Feb 2018 17:26:00 GMT

    [ https://issues.apache.org/jira/browse/CLOUDSTACK-9663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16359522#comment-16359522
] 

ASF GitHub Bot commented on CLOUDSTACK-9663:
--------------------------------------------

rhtyd closed pull request #2406: CLOUDSTACK-9663 return role on updateRole
URL: https://github.com/apache/cloudstack/pull/2406
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java
new file mode 100644
index 00000000000..9054ff5fada
--- /dev/null
+++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java
@@ -0,0 +1,36 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.acl;
+
+import org.apache.cloudstack.acl.Role;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.response.RoleResponse;
+
+public abstract class RoleCmd extends BaseCmd {
+
+    protected void setupResponse(final Role role) {
+        final RoleResponse response = new RoleResponse();
+        response.setId(role.getUuid());
+        response.setRoleName(role.getName());
+        response.setRoleType(role.getRoleType());
+        response.setDescription(role.getDescription());
+        response.setResponseName(getCommandName());
+        response.setObjectName("role");
+        setResponseObject(response);
+    }
+}
diff --git a/api/src/org/apache/cloudstack/acl/RoleService.java b/api/src/org/apache/cloudstack/acl/RoleService.java
index 98d170b5853..721bef08c20 100644
--- a/api/src/org/apache/cloudstack/acl/RoleService.java
+++ b/api/src/org/apache/cloudstack/acl/RoleService.java
@@ -31,7 +31,7 @@
     boolean isEnabled();
     Role findRole(final Long id);
     Role createRole(final String name, final RoleType roleType, final String description);
-    boolean updateRole(final Role role, final String name, final RoleType roleType, final
String description);
+    Role updateRole(final Role role, final String name, final RoleType roleType, final String
description);
     boolean deleteRole(final Role role);
 
     RolePermission findRolePermission(final Long id);
diff --git a/api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java b/api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java
index 994573d1796..87f0288dd01 100644
--- a/api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java
+++ b/api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java
@@ -34,7 +34,7 @@
         requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
         since = "4.9.0",
         authorized = {RoleType.Admin})
-public class CreateRoleCmd extends BaseCmd {
+public class CreateRoleCmd extends RoleCmd {
     public static final String APINAME = "createRole";
 
     /////////////////////////////////////////////////////
@@ -83,16 +83,6 @@ public long getEntityOwnerId() {
         return Account.ACCOUNT_ID_SYSTEM;
     }
 
-    private void setupResponse(final Role role) {
-        final RoleResponse response = new RoleResponse();
-        response.setId(role.getUuid());
-        response.setRoleName(role.getName());
-        response.setRoleType(role.getRoleType());
-        response.setResponseName(getCommandName());
-        response.setObjectName("role");
-        setResponseObject(response);
-    }
-
     @Override
     public void execute() {
         CallContext.current().setEventDetails("Role: " + getRoleName() + ", type:" + getRoleType()
+ ", description: " + getRoleDescription());
diff --git a/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java b/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java
index e17fc6f5714..f9519aeec5a 100644
--- a/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java
+++ b/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java
@@ -22,21 +22,20 @@
 import org.apache.cloudstack.acl.Role;
 import org.apache.cloudstack.acl.RoleType;
 import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiArgValidator;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.api.ApiErrorCode;
 import org.apache.cloudstack.api.BaseCmd;
 import org.apache.cloudstack.api.Parameter;
 import org.apache.cloudstack.api.ServerApiException;
-import org.apache.cloudstack.api.ApiArgValidator;
 import org.apache.cloudstack.api.response.RoleResponse;
-import org.apache.cloudstack.api.response.SuccessResponse;
 import org.apache.cloudstack.context.CallContext;
 
-@APICommand(name = UpdateRoleCmd.APINAME, description = "Updates a role", responseObject
= SuccessResponse.class,
+@APICommand(name = UpdateRoleCmd.APINAME, description = "Updates a role", responseObject
= RoleResponse.class,
         requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
         since = "4.9.0",
         authorized = {RoleType.Admin})
-public class UpdateRoleCmd extends BaseCmd {
+public class UpdateRoleCmd extends RoleCmd {
     public static final String APINAME = "updateRole";
 
     /////////////////////////////////////////////////////
@@ -100,9 +99,7 @@ public void execute() {
             throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role id provided");
         }
         CallContext.current().setEventDetails("Role: " + getRoleName() + ", type:" + getRoleType()
+ ", description: " + getRoleDescription());
-        boolean result = roleService.updateRole(role, getRoleName(), getRoleType(), getRoleDescription());
-        SuccessResponse response = new SuccessResponse(getCommandName());
-        response.setSuccess(result);
-        setResponseObject(response);
+        role = roleService.updateRole(role, getRoleName(), getRoleType(), getRoleDescription());
+        setupResponse(role);
     }
 }
diff --git a/api/test/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java b/api/test/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java
new file mode 100644
index 00000000000..c0bd390c196
--- /dev/null
+++ b/api/test/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.test;
+
+import junit.framework.TestCase;
+import org.apache.cloudstack.acl.Role;
+import org.apache.cloudstack.acl.RoleService;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.command.admin.acl.UpdateRoleCmd;
+import org.apache.cloudstack.api.response.RoleResponse;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import static org.mockito.Mockito.when;
+
+
+public class UpdateRoleCmdTest extends TestCase{
+
+    private UpdateRoleCmd updateRoleCmd;
+    private RoleService roleService;
+    private Role role;
+
+    @Override
+    @Before
+    public void setUp() {
+        roleService = Mockito.spy(RoleService.class);
+        updateRoleCmd = new UpdateRoleCmd();
+        ReflectionTestUtils.setField(updateRoleCmd,"roleService",roleService);
+        ReflectionTestUtils.setField(updateRoleCmd,"roleId",1L);
+        ReflectionTestUtils.setField(updateRoleCmd,"roleName","user");
+        ReflectionTestUtils.setField(updateRoleCmd,"roleType", "User");
+        ReflectionTestUtils.setField(updateRoleCmd,"roleDescription","Description Initial");
+        role = Mockito.mock(Role.class);
+    }
+
+    @Test
+    public void testUpdateSuccess() {
+        when(roleService.findRole(updateRoleCmd.getRoleId())).thenReturn(role);
+        when(role.getId()).thenReturn(1L);
+        when(role.getUuid()).thenReturn("12345-abcgdkajd");
+        when(role.getDescription()).thenReturn("Defualt user");
+        when(role.getName()).thenReturn("User");
+        when(role.getRoleType()).thenReturn(RoleType.User);
+        when(roleService.updateRole(role,updateRoleCmd.getRoleName(),updateRoleCmd.getRoleType(),updateRoleCmd.getRoleDescription())).thenReturn(role);
+        when(role.getId()).thenReturn(1L);
+        when(role.getDescription()).thenReturn("Description Initial");
+        when(role.getName()).thenReturn("User");
+        updateRoleCmd.execute();
+        RoleResponse response = (RoleResponse) updateRoleCmd.getResponseObject();
+        assertEquals((String)ReflectionTestUtils.getField(response, "roleName"),role.getName());
+        assertEquals((String)ReflectionTestUtils.getField(response, "roleDescription"),role.getDescription());
+    }
+}
diff --git a/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java b/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java
index 5b3998e8de8..9410a721cf8 100644
--- a/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java
+++ b/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java
@@ -120,11 +120,9 @@ public RoleVO doInTransaction(TransactionStatus status) {
 
     @Override
     @ActionEvent(eventType = EventTypes.EVENT_ROLE_UPDATE, eventDescription = "updating Role")
-    public boolean updateRole(final Role role, final String name, final RoleType roleType,
final String description) {
+    public Role updateRole(final Role role, final String name, final RoleType roleType, final
String description) {
         checkCallerAccess();
-        if (role == null) {
-            return false;
-        }
+
         if (roleType != null && roleType == RoleType.Unknown) {
             throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Unknown is not a valid
role type");
         }
@@ -146,7 +144,9 @@ public boolean updateRole(final Role role, final String name, final RoleType
rol
         if (!Strings.isNullOrEmpty(description)) {
             roleVO.setDescription(description);
         }
-        return roleDao.update(role.getId(), roleVO);
+
+        roleDao.update(role.getId(), roleVO);
+        return role;
     }
 
     @Override
diff --git a/test/integration/smoke/test_dynamicroles.py b/test/integration/smoke/test_dynamicroles.py
index 5d105f6a427..61e1d199741 100644
--- a/test/integration/smoke/test_dynamicroles.py
+++ b/test/integration/smoke/test_dynamicroles.py
@@ -209,7 +209,8 @@ def test_role_lifecycle_update(self):
         """
         self.account.delete(self.apiclient)
         new_role_name = self.getRandomRoleName()
-        self.role.update(self.apiclient, name=new_role_name, type='Admin')
+        new_role_description = "Fake role description created after update"
+        self.role.update(self.apiclient, name=new_role_name, type='Admin', description=new_role_description)
         update_role = Role.list(self.apiclient, id=self.role.id)[0]
         self.assertEqual(
             update_role.name,
@@ -221,7 +222,11 @@ def test_role_lifecycle_update(self):
             'Admin',
             msg="Role type does not match updated role type"
         )
-
+        self.assertEqual(
+            update_role.description,
+            new_role_description,
+            msg="Role description does not match updated role description"
+            )
 
     @attr(tags=['advanced', 'simulator', 'basic', 'sg'], required_hardware=False)
     def test_role_lifecycle_update_role_inuse(self):


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> updateRole should return updated role as json
> ---------------------------------------------
>
>                 Key: CLOUDSTACK-9663
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9663
>             Project: CloudStack
>          Issue Type: Improvement
>      Security Level: Public(Anyone can view this level - this is the default.) 
>          Components: API
>    Affects Versions: 4.9.0
>            Reporter: René Moser
>            Priority: Trivial
>             Fix For: Future, 4.11.0.0
>
>
> updateRole does currently only return success but one would expect it returns the updated
role as json like other update APIs
> Acutal results:
> {code}
>  $ cs updateRole id=0d6f4eca-cf71-47ba-81d9-aa129b38911b name=test --region=simulator
> {
>   "success": "true"
> }
> {code}
> expected resutls:
> {code}
>  $ cs updateRole id=0d6f4eca-cf71-47ba-81d9-aa129b38911b name=test --region=simulator
> {
>   "role": {
>     "id": ...
>     "description": ...
>     "name": ...
>     "type": ...
>   }
> }
> {code}
> similar to other update api e.g. http://cloudstack.apache.org/api/apidocs-4.9/apis/updateUser.html.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message