CLOUDSTACK-9663 return role on updateRole

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);
+    }
+}

api/src/org/apache/cloudstack/acl/RoleService.java

@@ -31,7 +31,7 @@ public interface RoleService {
     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);

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());

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);
     }
 }

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());
+    }
+}

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

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):