From 826073525179e3a4882b217f866d0288035acb08 Mon Sep 17 00:00:00 2001 From: Landry JUGE Date: Wed, 1 Jul 2026 21:22:33 +0200 Subject: [PATCH] Address remaining review feedback on service offering categories Fixes the outstanding reviewer/Copilot comments that were not covered by the earlier "apply copilot suggestions" commit: - Remove trailing space in ListServiceOfferingsCmd (checkstyle build failure) - Add dedicated EVENT_SERVICE_OFFERING_CATEGORY_{CREATE,EDIT,DELETE} event types mapped to ServiceOfferingCategory, and use them for category CRUD instead of the ServiceOffering event types (audit-trail misclassification) - Eliminate the N+1 category lookup in ServiceOfferingJoinDaoImpl by joining service_offering_category into service_offering_view and reading the category uuid/name directly from the join VO - Add missing ServiceOfferingDao.listByCategoryId used by the delete usage-check (the branch did not compile without it) - Add UNIQUE constraint on service_offering_category.name - UI: drop dead params.categoryid=null branch in DeployVM; fix double space in ListView category condition; fix radio-option__icon CSS typo; remove unused categories/categoryLoading state and fetchCategories() from AddComputeOffering and CloneComputeOffering --- api/src/main/java/com/cloud/event/EventTypes.java | 9 +++++++++ .../user/offering/ListServiceOfferingsCmd.java | 2 +- .../com/cloud/service/dao/ServiceOfferingDao.java | 2 ++ .../cloud/service/dao/ServiceOfferingDaoImpl.java | 12 ++++++++++++ .../resources/META-INF/db/schema-42210to42300.sql | 3 ++- .../db/views/cloud.service_offering_view.sql | 4 ++++ .../api/query/dao/ServiceOfferingJoinDaoImpl.java | 15 ++++----------- .../cloud/api/query/vo/ServiceOfferingJoinVO.java | 10 ++++++++++ .../configuration/ConfigurationManagerImpl.java | 6 +++--- ui/src/components/view/ListView.vue | 2 +- ui/src/views/compute/DeployVM.vue | 2 -- .../compute/wizard/ComputeOfferingSelection.vue | 2 +- ui/src/views/offering/AddComputeOffering.vue | 14 +------------- ui/src/views/offering/CloneComputeOffering.vue | 14 +------------- 14 files changed, 51 insertions(+), 46 deletions(-) diff --git a/api/src/main/java/com/cloud/event/EventTypes.java b/api/src/main/java/com/cloud/event/EventTypes.java index 42395bf8999..52bcd4edeac 100644 --- a/api/src/main/java/com/cloud/event/EventTypes.java +++ b/api/src/main/java/com/cloud/event/EventTypes.java @@ -83,6 +83,7 @@ import com.cloud.network.vpc.VpcOffering; import com.cloud.offering.DiskOffering; import com.cloud.offering.NetworkOffering; import com.cloud.offering.ServiceOffering; +import com.cloud.offering.ServiceOfferingCategory; import com.cloud.projects.Project; import com.cloud.server.ResourceTag; import com.cloud.storage.GuestOS; @@ -379,6 +380,11 @@ public class EventTypes { public static final String EVENT_SERVICE_OFFERING_EDIT = "SERVICE.OFFERING.EDIT"; public static final String EVENT_SERVICE_OFFERING_DELETE = "SERVICE.OFFERING.DELETE"; + // Service Offering Categories + public static final String EVENT_SERVICE_OFFERING_CATEGORY_CREATE = "SERVICE.OFFERING.CATEGORY.CREATE"; + public static final String EVENT_SERVICE_OFFERING_CATEGORY_EDIT = "SERVICE.OFFERING.CATEGORY.EDIT"; + public static final String EVENT_SERVICE_OFFERING_CATEGORY_DELETE = "SERVICE.OFFERING.CATEGORY.DELETE"; + // Disk Offerings public static final String EVENT_DISK_OFFERING_CREATE = "DISK.OFFERING.CREATE"; public static final String EVENT_DISK_OFFERING_CLONE = "DISK.OFFERING.CLONE"; @@ -1054,6 +1060,9 @@ public class EventTypes { entityEventDetails.put(EVENT_SERVICE_OFFERING_CLONE, ServiceOffering.class); entityEventDetails.put(EVENT_SERVICE_OFFERING_EDIT, ServiceOffering.class); entityEventDetails.put(EVENT_SERVICE_OFFERING_DELETE, ServiceOffering.class); + entityEventDetails.put(EVENT_SERVICE_OFFERING_CATEGORY_CREATE, ServiceOfferingCategory.class); + entityEventDetails.put(EVENT_SERVICE_OFFERING_CATEGORY_EDIT, ServiceOfferingCategory.class); + entityEventDetails.put(EVENT_SERVICE_OFFERING_CATEGORY_DELETE, ServiceOfferingCategory.class); // Disk Offerings entityEventDetails.put(EVENT_DISK_OFFERING_CREATE, DiskOffering.class); diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java index f369a1e5d0d..2b181d42ee6 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java @@ -204,7 +204,7 @@ public class ListServiceOfferingsCmd extends BaseListProjectAndAccountResourcesC public Long getCategoryId() { return categoryId; } - + public void setZoneId(Long zoneId) { this.zoneId = zoneId; } diff --git a/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDao.java b/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDao.java index d3bab4fcbe2..1f496b462ed 100644 --- a/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDao.java +++ b/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDao.java @@ -59,5 +59,7 @@ public interface ServiceOfferingDao extends GenericDao List listIdsByHostTag(String tag); + List listByCategoryId(Long categoryId); + void addCheckForGpuEnabled(SearchBuilder serviceOfferingSearch, Boolean gpuEnabled); } diff --git a/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java b/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java index f360770ad68..d09e2569f33 100644 --- a/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDaoImpl.java @@ -56,6 +56,7 @@ public class ServiceOfferingDaoImpl extends GenericDaoBase ServiceOfferingsByKeywordSearch; protected final SearchBuilder PublicCpuRamSearch; protected final SearchBuilder SearchComputeOfferingByComputeOnlyDiskOffering; + protected final SearchBuilder CategoryIdSearch; public ServiceOfferingDaoImpl() { super(); @@ -79,6 +80,17 @@ public class ServiceOfferingDaoImpl extends GenericDaoBase listByCategoryId(Long categoryId) { + SearchCriteria sc = CategoryIdSearch.create(); + sc.setParameters("category_id", categoryId); + return listBy(sc); } @Override diff --git a/engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql b/engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql index 3e456815b9c..af30a6d3289 100644 --- a/engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql +++ b/engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql @@ -25,7 +25,8 @@ CREATE TABLE IF NOT EXISTS `cloud`.`service_offering_category` ( `uuid` varchar(40), `sort_key` int NOT NULL DEFAULT 0, PRIMARY KEY (`id`), - CONSTRAINT `uc_service_offering_category__uuid` UNIQUE (`uuid`) + CONSTRAINT `uc_service_offering_category__uuid` UNIQUE (`uuid`), + CONSTRAINT `uc_service_offering_category__name` UNIQUE (`name`) ) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8; INSERT INTO `cloud`.`service_offering_category` (id, name, uuid) VALUES (1, 'Default', UUID()); diff --git a/engine/schema/src/main/resources/META-INF/db/views/cloud.service_offering_view.sql b/engine/schema/src/main/resources/META-INF/db/views/cloud.service_offering_view.sql index fa01eaef01b..933f32c10d9 100644 --- a/engine/schema/src/main/resources/META-INF/db/views/cloud.service_offering_view.sql +++ b/engine/schema/src/main/resources/META-INF/db/views/cloud.service_offering_view.sql @@ -86,6 +86,8 @@ SELECT `service_offering`.`gpu_count` AS `gpu_count`, `service_offering`.`gpu_display` AS `gpu_display`, `service_offering`.`category_id` AS `category_id`, + `service_offering_category`.`uuid` AS `category_uuid`, + `service_offering_category`.`name` AS `category_name`, GROUP_CONCAT(DISTINCT(domain.id)) AS domain_id, GROUP_CONCAT(DISTINCT(domain.uuid)) AS domain_uuid, GROUP_CONCAT(DISTINCT(domain.name)) AS domain_name, @@ -106,6 +108,8 @@ FROM LEFT JOIN `cloud`.`gpu_card` ON vgpu_profile.card_id = gpu_card.id LEFT JOIN + `cloud`.`service_offering_category` ON service_offering.category_id = service_offering_category.id + LEFT JOIN `cloud`.`service_offering_details` AS `domain_details` ON `domain_details`.`service_offering_id` = `service_offering`.`id` AND `domain_details`.`name`='domainid' LEFT JOIN `cloud`.`domain` AS `domain` ON FIND_IN_SET(`domain`.`id`, `domain_details`.`value`) diff --git a/server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java index b351d8de15f..2e4ef94bc03 100644 --- a/server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java @@ -44,8 +44,6 @@ import com.cloud.dc.dao.VsphereStoragePolicyDao; import com.cloud.offering.ServiceOffering; import com.cloud.server.ResourceTag.ResourceObjectType; import com.cloud.storage.DiskOfferingVO; -import com.cloud.service.dao.ServiceOfferingCategoryDao; -import com.cloud.service.ServiceOfferingCategoryVO; import com.cloud.user.AccountManager; import com.cloud.utils.db.GenericDaoBase; import com.cloud.utils.db.Filter; @@ -66,8 +64,6 @@ public class ServiceOfferingJoinDaoImpl extends GenericDaoBase sofIdSearch; @@ -152,13 +148,10 @@ public class ServiceOfferingJoinDaoImpl extends GenericDaoBase - diff --git a/ui/src/views/compute/DeployVM.vue b/ui/src/views/compute/DeployVM.vue index 4853242d2b2..2789b6a8bb4 100644 --- a/ui/src/views/compute/DeployVM.vue +++ b/ui/src/views/compute/DeployVM.vue @@ -2315,8 +2315,6 @@ export default { } if (categoryId && categoryId !== '-1') { params.categoryid = categoryId - } else { - params.categoryid = null } this.handleSearchFilter('serviceOfferings', params) }, diff --git a/ui/src/views/compute/wizard/ComputeOfferingSelection.vue b/ui/src/views/compute/wizard/ComputeOfferingSelection.vue index 245f419ac0f..db0cbca8c23 100644 --- a/ui/src/views/compute/wizard/ComputeOfferingSelection.vue +++ b/ui/src/views/compute/wizard/ComputeOfferingSelection.vue @@ -446,7 +446,7 @@ export default { width: 100%; height: 100%; - .radio-opion__icon { + .radio-option__icon { font-size: 2rem; margin-bottom: 0.5rem; } diff --git a/ui/src/views/offering/AddComputeOffering.vue b/ui/src/views/offering/AddComputeOffering.vue index 0bfa18fefef..10109dfcc16 100644 --- a/ui/src/views/offering/AddComputeOffering.vue +++ b/ui/src/views/offering/AddComputeOffering.vue @@ -117,9 +117,7 @@ export default { leaseexpiryaction: undefined, vgpuProfiles: [], vgpuProfileLoading: false, - externalDetailsEnabled: false, - categories: [], - categoryLoading: false + externalDetailsEnabled: false } }, beforeCreate () { @@ -231,7 +229,6 @@ export default { this.fetchDomainData() this.fetchZoneData() this.fetchGPUCards() - this.fetchCategories() if (isAdmin()) { this.fetchStorageTagData() this.fetchDeploymentPlannerData() @@ -257,15 +254,6 @@ export default { this.gpuCardLoading = false }) }, - fetchCategories () { - this.categoryLoading = true - getAPI('listServiceOfferingCategories', { - }).then(json => { - this.categories = json.listserviceofferingcategoriesresponse.serviceofferingcategory || [] - }).finally(() => { - this.categoryLoading = false - }) - }, fetchDiskOfferings () { this.diskOfferingLoading = true getAPI('listDiskOfferings', { diff --git a/ui/src/views/offering/CloneComputeOffering.vue b/ui/src/views/offering/CloneComputeOffering.vue index 8ed257fecf4..c91fe1095e3 100644 --- a/ui/src/views/offering/CloneComputeOffering.vue +++ b/ui/src/views/offering/CloneComputeOffering.vue @@ -140,9 +140,7 @@ export default { leaseexpiryaction: undefined, vgpuProfiles: [], vgpuProfileLoading: false, - externalDetailsEnabled: false, - categories: [], - categoryLoading: false + externalDetailsEnabled: false } }, beforeCreate () { @@ -254,7 +252,6 @@ export default { this.fetchDomainData() this.fetchZoneData() this.fetchGPUCards() - this.fetchCategories() if (isAdmin()) { this.fetchStorageTagData() this.fetchDeploymentPlannerData() @@ -400,15 +397,6 @@ export default { this.gpuCardLoading = false }) }, - fetchCategories () { - this.categoryLoading = true - getAPI('listServiceOfferingCategories', { - }).then(json => { - this.categories = json.listserviceofferingcategoriesresponse.serviceofferingcategory || [] - }).finally(() => { - this.categoryLoading = false - }) - }, fetchDiskOfferings () { this.diskOfferingLoading = true getAPI('listDiskOfferings', {