CLOUDSTACK-8647: updated with review comments

karuturi · karuturi · commit ca8b37535a6a · 2015-09-01T10:44:30.000+05:30
made domainId compulsory in api LinkDomainToLdapCmd
used accountServive from BaseCmd in LinkDomainToLdapCmd
changed the allowed account type values to 0 and 2
diff --git a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java
@@ -21,7 +21,6 @@
 import javax.inject.Inject;
 
 import com.cloud.exception.InvalidParameterValueException;
-import com.cloud.user.AccountService;
 import com.cloud.user.User;
 import com.cloud.user.UserAccount;
 import org.apache.cloudstack.api.APICommand;
@@ -47,7 +46,8 @@ public class LinkDomainToLdapCmd extends BaseCmd {
     public static final Logger s_logger = Logger.getLogger(LinkDomainToLdapCmd.class.getName());
     private static final String s_name = "linkdomaintoldapresponse";
 
-    @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "The id of the domain which has to be linked to LDAP.")
+    @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = true, entityType = DomainResponse.class, description = "The id of the domain which has to be "
+            + "linked to LDAP.")
     private Long domainId;
 
     @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true, description = "type of the ldap name. GROUP or OU")
@@ -59,16 +59,13 @@ public class LinkDomainToLdapCmd extends BaseCmd {
     @Parameter(name = ApiConstants.ADMIN, type = CommandType.STRING, required = false, description = "domain admin username in LDAP ")
     private String admin;
 
-    @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user, 1 for root " +
-        "admin, and 2 for domain admin")
+    @Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, required = true, description = "Type of the account to auto import. Specify 0 for user and 2 for " +
+        "domain admin")
     private short accountType;
 
     @Inject
     private LdapManager _ldapManager;
 
-    @Inject
-    public AccountService _accountService;
-
     @Override
     public void execute() throws ServerApiException {
         try {
diff --git a/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java b/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java
@@ -269,8 +269,8 @@ public LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, Str
         Validate.notNull(type, "type cannot be null. It should either be GROUP or OU");
         Validate.notNull(domainId, "domainId cannot be null.");
         Validate.notEmpty(name, "GROUP or OU name cannot be empty");
-        //Account type constants in com.cloud.user.Account
-        Validate.isTrue(accountType>=0 && accountType<=5, "accountype should be a number from 0-5");
+        //Account type should be 0 or 2. check the constants in com.cloud.user.Account
+        Validate.isTrue(accountType==0 || accountType==2, "accountype should be either 0(normal user) or 2(domain admin)");
         LinkType linkType = LdapManager.LinkType.valueOf(type.toUpperCase());
         LdapTrustMapVO vo = _ldapTrustMapDao.persist(new LdapTrustMapVO(domainId, linkType, name, accountType));
         LinkDomainToLdapResponse response = new LinkDomainToLdapResponse(vo.getDomainId(), vo.getType().toString(), vo.getName(), vo.getAccountType());
diff --git a/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapManagerImplSpec.groovy b/plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapManagerImplSpec.groovy
@@ -484,7 +484,7 @@ class LdapManagerImplSpec extends spock.lang.Specification {
         then:
             thrown(IllegalArgumentException)
         where:
-            accountType << [-1, 6, 20000, -500000]
+            accountType << [-1, 1, 3, 4, 5, 6, 20000, -500000]
     }
     def "test linkDomainToLdap when all is well"(){
         def ldapManager = new LdapManagerImpl()

Original file line number	Diff line number	Diff line change
`@@ -484,7 +484,7 @@ class LdapManagerImplSpec extends spock.lang.Specification {`
`484`	`484`	`then:`
`485`	`485`	`thrown(IllegalArgumentException)`
`486`	`486`	`where:`
`487`		`- accountType << [-1, 6, 20000, -500000]`
	`487`	`+ accountType << [-1, 1, 3, 4, 5, 6, 20000, -500000]`
`488`	`488`	`}`
`489`	`489`	`def "test linkDomainToLdap when all is well"(){`
`490`	`490`	`def ldapManager = new LdapManagerImpl()`