Extract new methods

add two new methods: updateDiskOfferingTagsIfIsNotNull that will check  and set tags to the diskOffering; isUpdateDiskOfferingNeeded that verify if any update is needed.

Create unit tests do cover all those new methods

Author: Rodrigo Lopez

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

@@ -39,6 +39,7 @@
 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.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;
@@ -3184,7 +3184,7 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             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 || tags != 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);
@@ -3208,13 +3208,7 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             diskOffering.setDisplayOffering(displayDiskOffering);
         }
 
-        if (tags != null){
-            if(tags.isBlank()){
-                diskOffering.setTags(null);
-            } else {
-                diskOffering.setTags(tags);
-            }
-        }
+        updateDiskOfferingTagsIfIsNotNull(tags, diskOffering);
 
         if (updateNeeded && !_diskOfferingDao.update(diskOfferingId, diskOffering)) {
             return null;
@@ -3251,6 +3245,31 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
         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.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.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.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 void validateMaximumIopsAndBytesLengthTestAllNull() {
     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);
+    }
 }