nvazquez commented on code in PR #10505: URL: https://github.com/apache/cloudstack/pull/10505#discussion_r2393149340
########## framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaAccountStateFilter.java: ########## @@ -0,0 +1,31 @@ +// 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.quota; + +public enum QuotaAccountStateFilter { + ALL, ACTIVE, REMOVED; + + public static QuotaAccountStateFilter getValue(String value) { + for (QuotaAccountStateFilter state : values()) { + if (state.name().equalsIgnoreCase(value)) { + return state; + } + } + + return null; Review Comment: Possibly also add a `StringUtils.isBlank(value)` check and return null in that case before the for loop? ########## plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java: ########## @@ -16,60 +16,82 @@ //under the License. package org.apache.cloudstack.api.command; -import com.cloud.user.Account; import com.cloud.utils.Pair; + +import org.apache.cloudstack.api.ACL; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseListCmd; import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.response.AccountResponse; import org.apache.cloudstack.api.response.DomainResponse; -import org.apache.cloudstack.api.response.ListResponse; import org.apache.cloudstack.api.response.QuotaResponseBuilder; import org.apache.cloudstack.api.response.QuotaSummaryResponse; -import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.api.response.ProjectResponse; +import org.apache.cloudstack.api.response.ListResponse; +import org.apache.cloudstack.quota.QuotaAccountStateFilter; +import org.apache.cloudstack.quota.QuotaService; +import org.apache.commons.lang3.ObjectUtils; +import org.apache.commons.lang3.StringUtils; + import java.util.List; import javax.inject.Inject; -@APICommand(name = "quotaSummary", responseObject = QuotaSummaryResponse.class, description = "Lists balance and quota usage for all accounts", since = "4.7.0", requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) +@APICommand(name = "quotaSummary", responseObject = QuotaSummaryResponse.class, description = "Lists accounts' balance summary.", since = "4.7.0", + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class QuotaSummaryCmd extends BaseListCmd { + @Inject + QuotaResponseBuilder quotaResponseBuilder; + + @Inject + QuotaService quotaService; + + @ACL + @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "ID of the account for which balance will be listed. Can not be specified with projectId.", since = "4.21.0") Review Comment: Since parameter needs updating :) ########## plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java: ########## @@ -16,60 +16,82 @@ //under the License. package org.apache.cloudstack.api.command; -import com.cloud.user.Account; import com.cloud.utils.Pair; + +import org.apache.cloudstack.api.ACL; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseListCmd; import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.response.AccountResponse; import org.apache.cloudstack.api.response.DomainResponse; -import org.apache.cloudstack.api.response.ListResponse; import org.apache.cloudstack.api.response.QuotaResponseBuilder; import org.apache.cloudstack.api.response.QuotaSummaryResponse; -import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.api.response.ProjectResponse; +import org.apache.cloudstack.api.response.ListResponse; +import org.apache.cloudstack.quota.QuotaAccountStateFilter; +import org.apache.cloudstack.quota.QuotaService; +import org.apache.commons.lang3.ObjectUtils; +import org.apache.commons.lang3.StringUtils; + import java.util.List; import javax.inject.Inject; -@APICommand(name = "quotaSummary", responseObject = QuotaSummaryResponse.class, description = "Lists balance and quota usage for all accounts", since = "4.7.0", requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) +@APICommand(name = "quotaSummary", responseObject = QuotaSummaryResponse.class, description = "Lists accounts' balance summary.", since = "4.7.0", + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class QuotaSummaryCmd extends BaseListCmd { + @Inject + QuotaResponseBuilder quotaResponseBuilder; + + @Inject + QuotaService quotaService; + + @ACL + @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "ID of the account for which balance will be listed. Can not be specified with projectId.", since = "4.21.0") + private Long accountId; + @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = false, description = "Optional, Account Id for which statement needs to be generated") private String accountName; @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = false, entityType = DomainResponse.class, description = "Optional, If domain Id is given and the caller is domain admin then the statement is generated for domain.") private Long domainId; - @Parameter(name = ApiConstants.LIST_ALL, type = CommandType.BOOLEAN, required = false, description = "Optional, to list all accounts irrespective of the quota activity") + @Deprecated + @Parameter(name = ApiConstants.LIST_ALL, type = CommandType.BOOLEAN, description = "False (default) lists balance summary for account. True lists balance summary for " + + "accounts which the caller has access.") private Boolean listAll; - @Inject - QuotaResponseBuilder _responseBuilder; + @Parameter(name = ApiConstants.ACCOUNT_STATE_TO_SHOW, type = CommandType.STRING, description = "Possible values are [ALL, ACTIVE, REMOVED]. ALL will list summaries for " + + "active and removed accounts; ACTIVE will list summaries only for active accounts; REMOVED will list summaries only for removed accounts. The default value is ACTIVE.", + since = "4.21.0") + private String accountStateToShow; - public QuotaSummaryCmd() { - super(); - } + @ACL + @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "Project Id for which balance will be listed. Can not be specified with accountId.", since = "4.21.0") Review Comment: Also here ########## plugins/database/quota/src/main/java/org/apache/cloudstack/quota/QuotaServiceImpl.java: ########## @@ -275,6 +285,48 @@ public boolean saveQuotaAccount(final AccountVO account, final BigDecimal aggrUs } } + /** + * Returns the Id of the account that will be used when provided with either accountId, projectId or accountName and domainId. + */ + @Override + public Long finalizeAccountId(Long accountId, String accountName, Long domainId, Long projectId) { Review Comment: I think this logic could be extracted into submethods ########## plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java: ########## @@ -16,60 +16,82 @@ //under the License. package org.apache.cloudstack.api.command; -import com.cloud.user.Account; import com.cloud.utils.Pair; + +import org.apache.cloudstack.api.ACL; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseListCmd; import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.response.AccountResponse; import org.apache.cloudstack.api.response.DomainResponse; -import org.apache.cloudstack.api.response.ListResponse; import org.apache.cloudstack.api.response.QuotaResponseBuilder; import org.apache.cloudstack.api.response.QuotaSummaryResponse; -import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.api.response.ProjectResponse; +import org.apache.cloudstack.api.response.ListResponse; +import org.apache.cloudstack.quota.QuotaAccountStateFilter; +import org.apache.cloudstack.quota.QuotaService; +import org.apache.commons.lang3.ObjectUtils; +import org.apache.commons.lang3.StringUtils; + import java.util.List; import javax.inject.Inject; -@APICommand(name = "quotaSummary", responseObject = QuotaSummaryResponse.class, description = "Lists balance and quota usage for all accounts", since = "4.7.0", requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) +@APICommand(name = "quotaSummary", responseObject = QuotaSummaryResponse.class, description = "Lists accounts' balance summary.", since = "4.7.0", + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class QuotaSummaryCmd extends BaseListCmd { + @Inject + QuotaResponseBuilder quotaResponseBuilder; + + @Inject + QuotaService quotaService; + + @ACL + @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "ID of the account for which balance will be listed. Can not be specified with projectId.", since = "4.21.0") + private Long accountId; + @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = false, description = "Optional, Account Id for which statement needs to be generated") private String accountName; @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = false, entityType = DomainResponse.class, description = "Optional, If domain Id is given and the caller is domain admin then the statement is generated for domain.") private Long domainId; - @Parameter(name = ApiConstants.LIST_ALL, type = CommandType.BOOLEAN, required = false, description = "Optional, to list all accounts irrespective of the quota activity") + @Deprecated + @Parameter(name = ApiConstants.LIST_ALL, type = CommandType.BOOLEAN, description = "False (default) lists balance summary for account. True lists balance summary for " + + "accounts which the caller has access.") private Boolean listAll; - @Inject - QuotaResponseBuilder _responseBuilder; + @Parameter(name = ApiConstants.ACCOUNT_STATE_TO_SHOW, type = CommandType.STRING, description = "Possible values are [ALL, ACTIVE, REMOVED]. ALL will list summaries for " + + "active and removed accounts; ACTIVE will list summaries only for active accounts; REMOVED will list summaries only for removed accounts. The default value is ACTIVE.", + since = "4.21.0") + private String accountStateToShow; - public QuotaSummaryCmd() { - super(); - } + @ACL + @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "Project Id for which balance will be listed. Can not be specified with accountId.", since = "4.21.0") + private Long projectId; @Override public void execute() { Review Comment: Should this method include some pre-checks for accountId vs projectId parameters? ########## engine/schema/src/main/java/com/cloud/domain/dao/DomainDaoImpl.java: ########## @@ -262,7 +262,7 @@ public boolean isChildDomain(Long parentId, Long childId) { SearchCriteria<DomainVO> sc = DomainPairSearch.create(); sc.setParameters("id", parentId, childId); - List<DomainVO> domainPair = listBy(sc); + List<DomainVO> domainPair = listIncludingRemovedBy(sc); Review Comment: Sorry I'm not clear why should removed domains should be considered? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
