enable update tags on disk offerings (#4194)

api/src/main/java/org/apache/cloudstack/api/command/admin/offering/UpdateDiskOfferingCmd.java

@@ -19,6 +19,7 @@ package org.apache.cloudstack.api.command.admin.offering;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.apache.cloudstack.acl.RoleType;
 import org.apache.cloudstack.api.APICommand;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.api.ApiErrorCode;
@@ -77,6 +78,14 @@ public class UpdateDiskOfferingCmd extends BaseCmd {
             since = "4.13")
     private String zoneIds;
 
+    @Parameter(name = ApiConstants.TAGS,
+            type = CommandType.STRING,
+            description = "comma-separated list of tags for the disk offering, tags should match with existing storage pool tags",
+            authorized = {RoleType.Admin},
+            since = "4.15")
+    private String tags;
+
+
     /////////////////////////////////////////////////////
     /////////////////// Accessors ///////////////////////
     /////////////////////////////////////////////////////
@@ -161,6 +170,10 @@ public class UpdateDiskOfferingCmd extends BaseCmd {
         return validZoneIds;
     }
 
+    public String getTags() {
+        return tags;
+    }
+
 /////////////////////////////////////////////////////
     /////////////// API Implementation///////////////////
     /////////////////////////////////////////////////////

engine/schema/src/main/java/com/cloud/storage/DiskOfferingVO.java

@@ -364,7 +364,7 @@ public class DiskOfferingVO implements DiskOffering {
         return created;
     }
 
-    protected void setTags(String tags) {
+    public void setTags(String tags) {
         this.tags = tags;
     }
 

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -39,6 +39,7 @@ import java.util.UUID;
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
 
+import com.cloud.utils.StringUtils;
 import org.apache.cloudstack.acl.SecurityChecker;
 import org.apache.cloudstack.affinity.AffinityGroup;
 import org.apache.cloudstack.affinity.AffinityGroupService;
@@ -222,7 +223,6 @@ import com.cloud.user.dao.AccountDao;
 import com.cloud.user.dao.UserDao;
 import com.cloud.utils.NumbersUtil;
 import com.cloud.utils.Pair;
-import com.cloud.utils.StringUtils;
 import com.cloud.utils.UriUtils;
 import com.cloud.utils.component.ManagerBase;
 import com.cloud.utils.db.DB;
@@ -3109,6 +3109,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
         final Boolean displayDiskOffering = cmd.getDisplayOffering();
         final List<Long> domainIds = cmd.getDomainIds();
         final List<Long> zoneIds = cmd.getZoneIds();
+        final String tags = cmd.getTags();
 
         // Check if diskOffering exists
         final DiskOffering diskOfferingHandle = _entityMgr.findById(DiskOffering.class, diskOfferingId);
@@ -3190,7 +3191,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
             throw new InvalidParameterValueException(String.format("Unable to update disk offering: %s by id user: %s because it is not root-admin or domain-admin", diskOfferingHandle.getUuid(), user.getUuid()));
         }
 
-        final boolean updateNeeded = name != null || displayText != null || sortKey != null || displayDiskOffering != null;
+        final boolean updateNeeded = shouldUpdateDiskOffering(name, displayText, sortKey, displayDiskOffering, tags);
         final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
         if (!updateNeeded && !detailsUpdateNeeded) {
             return _diskOfferingDao.findById(diskOfferingId);
@@ -3214,30 +3215,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
             diskOffering.setDisplayOffering(displayDiskOffering);
         }
 
-        // Note: tag editing commented out for now;keeping the code intact,
+        updateDiskOfferingTagsIfIsNotNull(tags, diskOffering);
-        // might need to re-enable in next releases
-        // if (tags != null)
-        // {
-        // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
-        // {
-        // //no new tags; no existing tags
-        // diskOffering.setTagsArray(csvTagsToList(null));
-        // }
-        // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
-        // null)
-        // {
-        // //new tags + existing tags
-        // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
-        // List<String> newTags = csvTagsToList(tags);
-        // oldTags.addAll(newTags);
-        // diskOffering.setTagsArray(oldTags);
-        // }
-        // else if(!tags.trim().isEmpty())
-        // {
-        // //new tags; NO existing tags
-        // diskOffering.setTagsArray(csvTagsToList(tags));
-        // }
-        // }
 
         if (updateNeeded && !_diskOfferingDao.update(diskOfferingId, diskOffering)) {
             return null;
@@ -3274,6 +3252,31 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
         return _diskOfferingDao.findById(diskOfferingId);
     }
 
+    /**
+     * Check the tags parameters to the diskOffering
+     * <ul>
+     *     <li>If tags is null, do nothing and return.</li>
+     *     <li>If tags is not null, set tag to the diskOffering.</li>
+     *     <li>If tags is an blank string, set null on diskOffering tag.</li>
+     * </ul>
+     */
+    protected void updateDiskOfferingTagsIfIsNotNull(String tags, DiskOfferingVO diskOffering) {
+        if (tags == null) { return; }
+        if (StringUtils.isNotBlank(tags)) {
+            diskOffering.setTags(tags);
+        } else {
+            diskOffering.setTags(null);
+        }
+    }
+
+    /**
+     * Check if it needs to update any parameter when updateDiskoffering is called
+     * Verify if name or displayText are not blank, tags is not null, sortkey and displayDiskOffering is not null
+     */
+    protected boolean shouldUpdateDiskOffering(String name, String displayText, Integer sortKey, Boolean displayDiskOffering, String tags) {
+        return StringUtils.isNotBlank(name) || StringUtils.isNotBlank(displayText) || tags != null || sortKey != null || displayDiskOffering != null;
+    }
+
     @Override
     @ActionEvent(eventType = EventTypes.EVENT_DISK_OFFERING_DELETE, eventDescription = "deleting disk offering")
     public boolean deleteDiskOffering(final DeleteDiskOfferingCmd cmd) {

server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java

@@ -24,6 +24,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.nullable;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
@@ -56,6 +57,7 @@ import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
 
 import com.cloud.api.query.dao.NetworkOfferingJoinDao;
 import com.cloud.api.query.vo.NetworkOfferingJoinVO;
@@ -87,6 +89,7 @@ import com.cloud.network.dao.IPAddressVO;
 import com.cloud.network.dao.PhysicalNetworkDao;
 import com.cloud.network.dao.PhysicalNetworkVO;
 import com.cloud.projects.ProjectManager;
+import com.cloud.storage.DiskOfferingVO;
 import com.cloud.storage.VolumeVO;
 import com.cloud.storage.dao.VolumeDao;
 import com.cloud.user.Account;
@@ -108,6 +111,7 @@ public class ConfigurationManagerTest {
 
     private static final Logger s_logger = Logger.getLogger(ConfigurationManagerTest.class);
 
+    @Spy
     @InjectMocks
     ConfigurationManagerImpl configurationMgr = new ConfigurationManagerImpl();
 
@@ -163,11 +167,12 @@ public class ConfigurationManagerTest {
     ImageStoreDao _imageStoreDao;
     @Mock
     ConfigurationDao _configDao;
+    @Mock
+    DiskOfferingVO diskOfferingVOMock;
 
     VlanVO vlan = new VlanVO(Vlan.VlanType.VirtualNetwork, "vlantag", "vlangateway", "vlannetmask", 1L, "iprange", 1L, 1L, null, null, null);
 
     private static final String MAXIMUM_DURATION_ALLOWED = "3600";
-
     @Mock
     Network network;
     @Mock
@@ -938,4 +943,33 @@ public class ConfigurationManagerTest {
     public void validateMaximumIopsAndBytesLengthTestDefaultLengthConfigs() {
         configurationMgr.validateMaximumIopsAndBytesLength(36000l, 36000l, 36000l, 36000l);
     }
+
+    @Test
+    public void shouldUpdateDiskOfferingTests(){
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), Mockito.anyString(), Mockito.anyInt(), Mockito.anyBoolean(), Mockito.anyString()));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), Mockito.anyString(), nullable(Integer.class), nullable(Boolean.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), Mockito.anyInt(), nullable(Boolean.class), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), Mockito.anyBoolean(), nullable(String.class)));
+        Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), nullable(Boolean.class), Mockito.anyString()));
+    }
+
+    @Test
+    public void shouldUpdateDiskOfferingTestFalse(){
+        Assert.assertFalse(configurationMgr.shouldUpdateDiskOffering(null, null, null, null, null));
+    }
+
+    @Test
+    public void updateDiskOfferingTagsIfIsNotNullTestWhenTagsIsNull(){
+        Mockito.doNothing().when(configurationMgr).updateDiskOfferingTagsIfIsNotNull(null, diskOfferingVOMock);
+        this.configurationMgr.updateDiskOfferingTagsIfIsNotNull(null, diskOfferingVOMock);
+        Mockito.verify(configurationMgr, Mockito.times(1)).updateDiskOfferingTagsIfIsNotNull(null, diskOfferingVOMock);
+    }
+    @Test
+    public void updateDiskOfferingTagsIfIsNotNullTestWhenTagsIsNotNull(){
+        String tags = "tags";
+        Mockito.doNothing().when(configurationMgr).updateDiskOfferingTagsIfIsNotNull(tags, diskOfferingVOMock);
+        this.configurationMgr.updateDiskOfferingTagsIfIsNotNull(tags, diskOfferingVOMock);
+        Mockito.verify(configurationMgr, Mockito.times(1)).updateDiskOfferingTagsIfIsNotNull(tags, diskOfferingVOMock);
+    }
 }

ui/scripts/configuration.js

@@ -2613,7 +2613,8 @@
                                     var data = {
                                         id: args.context.diskOfferings[0].id,
                                         name: args.data.name,
-                                        displaytext: args.data.displaytext
+                                        displaytext: args.data.displaytext,
+                                        tags: args.data.tags
                                     };
                                     $.ajax({
                                         url: createURL('updateDiskOffering'),
@@ -2939,7 +2940,41 @@
                                         label: 'label.cache.mode'
                                     },
                                     tags: {
-                                        label: 'label.storage.tags'
+                                        label: 'label.storage.tags',
+                                        docID: 'helpPrimaryStorageTags',
+                                        isEditable: true,
+                                        isTokenInput: true,
+                                        dataProvider: function(args) {
+                                            $.ajax({
+                                                url: createURL("listStorageTags"),
+                                                dataType: "json",
+                                                success: function(json) {
+                                                    var item = json.liststoragetagsresponse.storagetag;
+                                                    var tags = [];
+
+                                                    if (item != null)
+                                                    {
+                                                        tags = $.map(item, function(tag) {
+                                                            return {
+                                                                id: tag.name,
+                                                                name: tag.name
+                                                            };
+                                                        });
+                                                    }
+
+                                                    args.response.success({
+                                                        data: tags,
+                                                        hintText: _l('hint.type.part.storage.tag'),
+                                                        noResultsText: _l('hint.no.storage.tags')
+                                                    });
+                                                },
+                                                error: function(XMLHttpResponse) {
+                                                    var errorMsg = parseXMLHttpResponse(XMLHttpResponse);
+
+                                                    args.response.error(errorMsg);
+                                                }
+                                            });
+                                        }
                                     },
                                     domain: {
                                         label: 'label.domain'