Fixes for software module assignment in Distributions Management view of Mgmt UI (#759)

* Fix software module assignment
* Fix hawkbit-bot findings
* Fix review findings

Signed-off-by: Stefan Behl <stefan.behl@bosch-si.com>

hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/common/ConfirmationDialog.java

@@ -42,6 +42,8 @@ public class ConfirmationDialog implements Button.ClickListener {
 
     private final Window window;
 
+    private boolean isImplicitClose;
+
     /**
      * Constructor for configuring confirmation dialog.
      * 
@@ -107,6 +109,31 @@ public class ConfirmationDialog implements Button.ClickListener {
         this(caption, question, okLabel, cancelLabel, callback, null, id, null);
     }
 
+    /**
+     * Constructor for configuring confirmation dialog.
+     * 
+     * @param caption
+     *            the dialog caption.
+     * @param question
+     *            the question.
+     * @param okLabel
+     *            the Ok button label.
+     * @param cancelLabel
+     *            the cancel button label.
+     * @param callback
+     *            the callback.
+     * @param id
+     *            the id of the confirmation dialog
+     * @param mapCloseToCancel
+     *            Flag indicating whether or not the close event on the
+     *            enclosing window should be mapped to a cancel event.
+     */
+    public ConfirmationDialog(final String caption, final String question, final String okLabel,
+            final String cancelLabel, final ConfirmationDialogCallback callback, final String id,
+            final boolean mapCloseToCancel) {
+        this(caption, question, okLabel, cancelLabel, callback, null, id, null, mapCloseToCancel);
+    }
+
     /**
      * Constructor for configuring confirmation dialog.
      * 
@@ -152,6 +179,37 @@ public class ConfirmationDialog implements Button.ClickListener {
     public ConfirmationDialog(final String caption, final String question, final String okLabel,
             final String cancelLabel, final ConfirmationDialogCallback callback, final Resource icon, final String id,
             final ConfirmationTab tab) {
+        this(caption, question, okLabel, cancelLabel, callback, icon, id, tab, false);
+
+    }
+
+    /**
+     * Constructor for configuring confirmation dialog.
+     * 
+     * @param caption
+     *            the dialog caption.
+     * @param question
+     *            the question.
+     * @param okLabel
+     *            the Ok button label.
+     * @param cancelLabel
+     *            the cancel button label.
+     * @param callback
+     *            the callback.
+     * @param icon
+     *            the icon of the dialog
+     * @param id
+     *            the id of the confirmation dialog
+     * @param tab
+     *            ConfirmationTab which contains more information about the
+     *            action which has to be confirmed, e.g. maintenance window
+     * @param mapCloseToCancel
+     *            Flag indicating whether or not the close event on the
+     *            enclosing window should be mapped to a cancel event.
+     */
+    public ConfirmationDialog(final String caption, final String question, final String okLabel,
+            final String cancelLabel, final ConfirmationDialogCallback callback, final Resource icon, final String id,
+            final ConfirmationTab tab, final boolean mapCloseToCancel) {
         window = new Window(caption);
         if (!StringUtils.isEmpty(id)) {
             window.setId(id);
@@ -165,6 +223,13 @@ public class ConfirmationDialog implements Button.ClickListener {
         okButton = createOkButton(okLabel);
 
         final Button cancelButton = createCancelButton(cancelLabel);
+        if (mapCloseToCancel) {
+            window.addCloseListener(e -> {
+                if (!isImplicitClose) {
+                    cancelButton.click();
+                }
+            });
+        }
         window.setModal(true);
         window.addStyleName(SPUIStyleDefinitions.CONFIRMBOX_WINDOW_STYLE);
         if (this.callback == null) {
@@ -230,6 +295,7 @@ public class ConfirmationDialog implements Button.ClickListener {
     @Override
     public void buttonClick(final ClickEvent event) {
         if (window.getParent() != null) {
+            isImplicitClose = true;
             UI.getCurrent().removeWindow(window);
         }
         callback.response(event.getSource().equals(okButton));

hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/distributions/dstable/DistributionSetDetails.java

@@ -10,12 +10,9 @@ package org.eclipse.hawkbit.ui.distributions.dstable;
 
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Set;
 
 import org.eclipse.hawkbit.repository.DistributionSetManagement;
 import org.eclipse.hawkbit.repository.DistributionSetTagManagement;
-import org.eclipse.hawkbit.repository.SoftwareModuleManagement;
-import org.eclipse.hawkbit.repository.TargetManagement;
 import org.eclipse.hawkbit.repository.model.SoftwareModule;
 import org.eclipse.hawkbit.ui.SpPermissionChecker;
 import org.eclipse.hawkbit.ui.artifacts.event.SoftwareModuleEvent;
@@ -23,9 +20,6 @@ import org.eclipse.hawkbit.ui.artifacts.event.SoftwareModuleEvent.SoftwareModule
 import org.eclipse.hawkbit.ui.common.detailslayout.AbstractDistributionSetDetails;
 import org.eclipse.hawkbit.ui.common.detailslayout.SoftwareModuleDetailsTable;
 import org.eclipse.hawkbit.ui.common.detailslayout.TargetFilterQueryDetailsTable;
-import org.eclipse.hawkbit.ui.common.entity.SoftwareModuleIdName;
-import org.eclipse.hawkbit.ui.components.SPUIComponentProvider;
-import org.eclipse.hawkbit.ui.decorators.SPUIButtonStyleNoBorder;
 import org.eclipse.hawkbit.ui.distributions.event.SaveActionWindowEvent;
 import org.eclipse.hawkbit.ui.distributions.state.ManageDistUIState;
 import org.eclipse.hawkbit.ui.management.dstable.DistributionAddUpdateWindowLayout;
@@ -38,8 +32,6 @@ import org.vaadin.spring.events.EventScope;
 import org.vaadin.spring.events.annotation.EventBusListenerMethod;
 
 import com.vaadin.data.Item;
-import com.vaadin.server.FontAwesome;
-import com.vaadin.ui.Button;
 import com.vaadin.ui.HorizontalLayout;
 import com.vaadin.ui.Label;
 import com.vaadin.ui.UI;
@@ -56,10 +48,6 @@ public class DistributionSetDetails extends AbstractDistributionSetDetails {
 
     private final ManageDistUIState manageDistUIState;
 
-    private final transient SoftwareModuleManagement softwareModuleManagement;
-
-    private final transient TargetManagement targetManagement;
-
     private final TargetFilterQueryDetailsTable tfqDetailsTable;
 
     private Map<String, StringBuilder> assignedSWModule;
@@ -68,17 +56,14 @@ public class DistributionSetDetails extends AbstractDistributionSetDetails {
             final SpPermissionChecker permissionChecker, final ManageDistUIState manageDistUIState,
             final ManagementUIState managementUIState,
             final DistributionAddUpdateWindowLayout distributionAddUpdateWindowLayout,
-            final SoftwareModuleManagement softwareManagement,
+            final DistributionSetManagement distributionSetManagement, final UINotification uiNotification,
-            final DistributionSetManagement distributionSetManagement, final TargetManagement targetManagement,
+            final DistributionSetTagManagement distributionSetTagManagement,
-            final UINotification uiNotification, final DistributionSetTagManagement distributionSetTagManagement,
             final DsMetadataPopupLayout dsMetadataPopupLayout) {
         super(i18n, eventBus, permissionChecker, managementUIState, distributionAddUpdateWindowLayout,
                 distributionSetManagement, dsMetadataPopupLayout, uiNotification, distributionSetTagManagement,
                 createSoftwareModuleDetailsTable(i18n, permissionChecker, distributionSetManagement, eventBus,
                         manageDistUIState, uiNotification));
         this.manageDistUIState = manageDistUIState;
-        this.softwareModuleManagement = softwareManagement;
-        this.targetManagement = targetManagement;
 
         tfqDetailsTable = new TargetFilterQueryDetailsTable(i18n);
 
@@ -106,66 +91,7 @@ public class DistributionSetDetails extends AbstractDistributionSetDetails {
         populateTargetFilterQueries();
     }
 
-    @Override
+    private static String getUnsavedAssignedSwModule(final String name, final String version) {
-    protected void populateModule() {
-        super.populateModule();
-        showUnsavedAssignment();
-    }
-
-    @SuppressWarnings("unchecked")
-    private void showUnsavedAssignment() {
-        final Set<SoftwareModuleIdName> softwareModuleIdNameList = manageDistUIState.getLastSelectedDistribution()
-                .map(selectedDistId -> manageDistUIState.getAssignedList().entrySet().stream()
-                        .filter(entry -> entry.getKey().getId().equals(selectedDistId)).findAny()
-                        .map(Map.Entry::getValue).orElse(null))
-                .orElse(null);
-
-        if (softwareModuleIdNameList != null) {
-            if (assignedSWModule == null) {
-                assignedSWModule = new HashMap<>();
-            }
-
-            softwareModuleIdNameList.stream().map(SoftwareModuleIdName::getId).map(softwareModuleManagement::get)
-                    .forEach(found -> found.ifPresent(softwareModule -> {
-
-                        if (assignedSWModule.containsKey(softwareModule.getType().getName())) {
-                            assignedSWModule.get(softwareModule.getType().getName()).append("</br>").append("<I>")
-                                    .append(HawkbitCommonUtil.getFormattedNameVersion(softwareModule.getName(),
-                                            softwareModule.getVersion()))
-                                    .append("<I>");
-
-                        } else {
-                            assignedSWModule
-                                    .put(softwareModule.getType().getName(),
-                                            new StringBuilder().append("<I>")
-                                                    .append(HawkbitCommonUtil.getFormattedNameVersion(
-                                                            softwareModule.getName(), softwareModule.getVersion()))
-                                                    .append("<I>"));
-                        }
-
-                    }));
-            for (final Map.Entry<String, StringBuilder> entry : assignedSWModule.entrySet()) {
-                final Item item = getSoftwareModuleTable().getContainerDataSource().getItem(entry.getKey());
-                if (item != null) {
-                    item.getItemProperty(SOFT_MODULE).setValue(createSoftModuleLayout(entry.getValue().toString()));
-                }
-            }
-        }
-    }
-
-    private Button assignSoftModuleButton(final String softwareModuleName) {
-        if (getPermissionChecker().hasUpdateRepositoryPermission() && manageDistUIState.getLastSelectedDistribution()
-                .map(selected -> targetManagement.countByAssignedDistributionSet(selected) <= 0).orElse(false)) {
-
-            final Button reassignSoftModule = SPUIComponentProvider.getButton(softwareModuleName, "", "", "", true,
-                    FontAwesome.TIMES, SPUIButtonStyleNoBorder.class);
-            reassignSoftModule.setEnabled(false);
-            return reassignSoftModule;
-        }
-        return null;
-    }
-
-    private static String getUnsavedAssigedSwModule(final String name, final String version) {
         return HawkbitCommonUtil.getFormattedNameVersion(name, version);
     }
 
@@ -177,52 +103,48 @@ public class DistributionSetDetails extends AbstractDistributionSetDetails {
 
         getSoftwareModuleTable().getContainerDataSource().getItemIds();
         if (assignedSWModule.containsKey(module.getType().getName())) {
-            /*
-             * If software module type is software, means multiple softwares can
-             * assigned to that type. Hence if multipe softwares belongs to same
-             * type is drroped, then add to the list.
-             */
 
+            /*
+             * If the module type allows multiple assignments, just append the
+             * module entry to the list.
+             */
             if (module.getType().getMaxAssignments() > 1) {
                 assignedSWModule.get(module.getType().getName()).append("</br>").append("<I>")
-                        .append(getUnsavedAssigedSwModule(module.getName(), module.getVersion())).append("</I>");
+                        .append(getUnsavedAssignedSwModule(module.getName(), module.getVersion())).append("</I>");
             }
 
             /*
-             * If software module type is firmware, means single software can be
+             * If the module type does not allow multiple assignments, override
-             * assigned to that type. Hence if multiple softwares belongs to
+             * the previous module entry.
-             * same type is dropped, then override with previous one.
              */
             if (module.getType().getMaxAssignments() == 1) {
                 assignedSWModule.put(module.getType().getName(), new StringBuilder().append("<I>")
-                        .append(getUnsavedAssigedSwModule(module.getName(), module.getVersion())).append("</I>"));
+                        .append(getUnsavedAssignedSwModule(module.getName(), module.getVersion())).append("</I>"));
             }
 
         } else {
             assignedSWModule.put(module.getType().getName(), new StringBuilder().append("<I>")
-                    .append(getUnsavedAssigedSwModule(module.getName(), module.getVersion())).append("</I>"));
+                    .append(getUnsavedAssignedSwModule(module.getName(), module.getVersion())).append("</I>"));
         }
 
         for (final Map.Entry<String, StringBuilder> entry : assignedSWModule.entrySet()) {
             final Item item = getSoftwareModuleTable().getContainerDataSource().getItem(entry.getKey());
             if (item != null) {
-                item.getItemProperty(SOFT_MODULE).setValue(createSoftModuleLayout(entry.getValue().toString()));
+                item.getItemProperty(SOFT_MODULE).setValue(createSoftwareModuleLayout(entry.getValue().toString()));
             }
         }
     }
 
-    private VerticalLayout createSoftModuleLayout(final String softwareModuleName) {
+    private static VerticalLayout createSoftwareModuleLayout(final String softwareModuleName) {
         final VerticalLayout verticalLayout = new VerticalLayout();
         final HorizontalLayout horizontalLayout = new HorizontalLayout();
         horizontalLayout.setSizeFull();
         final Label softwareModule = HawkbitCommonUtil.getFormatedLabel("");
-        final Button reassignSoftModule = assignSoftModuleButton(softwareModuleName);
         softwareModule.setValue(softwareModuleName);
         softwareModule.setDescription(softwareModuleName);
         softwareModule.setId(softwareModuleName + "-label");
         horizontalLayout.addComponent(softwareModule);
         horizontalLayout.setExpandRatio(softwareModule, 1F);
-        horizontalLayout.addComponent(reassignSoftModule);
         verticalLayout.addComponent(horizontalLayout);
         return verticalLayout;
     }
@@ -248,10 +170,7 @@ public class DistributionSetDetails extends AbstractDistributionSetDetails {
         if ((saveActionWindowEvent == SaveActionWindowEvent.SAVED_ASSIGNMENTS
                 || saveActionWindowEvent == SaveActionWindowEvent.DISCARD_ALL_ASSIGNMENTS)
                 && getSelectedBaseEntity() != null) {
-            if (assignedSWModule != null) {
+            clearAssignments();
-                assignedSWModule.clear();
-            }
-
             getDistributionSetManagement().getWithDetails(getSelectedBaseEntityId()).ifPresent(set -> {
                 setSelectedBaseEntity(set);
                 UI.getCurrent().access(this::populateModule);
@@ -264,10 +183,13 @@ public class DistributionSetDetails extends AbstractDistributionSetDetails {
         if (saveActionWindowEvent == SaveActionWindowEvent.DISCARD_ASSIGNMENT
                 || saveActionWindowEvent == SaveActionWindowEvent.DISCARD_ALL_ASSIGNMENTS
                 || saveActionWindowEvent == SaveActionWindowEvent.DELETE_ALL_SOFWARE) {
-            if (assignedSWModule != null) {
+            clearAssignments();
-                assignedSWModule.clear();
+        }
-            }
+    }
-            showUnsavedAssignment();
+
+    private void clearAssignments() {
+        if (assignedSWModule != null) {
+            assignedSWModule.clear();
         }
     }
 

hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/distributions/dstable/DistributionSetTable.java

@@ -323,10 +323,9 @@ public class DistributionSetTable extends AbstractNamedVersionTable<Distribution
                     if (ok) {
                         saveAllAssignments();
                     } else {
-                        manageDistUIState.getAssignedList().clear();
+                        cancelAllAssignments();
-                        manageDistUIState.getConsolidatedDistSoftwareList().clear();
                     }
-                }, UIComponentIdProvider.SOFT_MODULE_TO_DIST_ASSIGNMENT_CONFIRM_ID);
+                }, UIComponentIdProvider.SOFT_MODULE_TO_DIST_ASSIGNMENT_CONFIRM_ID, true);
     }
 
     private String createConfirmationMessageForAssignment(final String distributionNameToAssign,
@@ -342,7 +341,7 @@ public class DistributionSetTable extends AbstractNamedVersionTable<Distribution
 
     private void saveAllAssignments() {
         manageDistUIState.getAssignedList().forEach((distIdName, softIdNameSet) -> {
-            final List<Long> softIds = softIdNameSet.stream().map(softIdName -> softIdName.getId())
+            final List<Long> softIds = softIdNameSet.stream().map(SoftwareModuleIdName::getId)
                     .collect(Collectors.toList());
             distributionSetManagement.assignSoftwareModules(distIdName.getId(), softIds);
         });
@@ -354,9 +353,18 @@ public class DistributionSetTable extends AbstractNamedVersionTable<Distribution
         }
 
         getNotification().displaySuccess(getI18n().getMessage("message.software.assignment", count));
+        resetState();
+        getEventBus().publish(this, SaveActionWindowEvent.SAVED_ASSIGNMENTS);
+    }
+
+    private void cancelAllAssignments() {
+        resetState();
+        getEventBus().publish(this, SaveActionWindowEvent.DISCARD_ALL_ASSIGNMENTS);
+    }
+
+    private void resetState() {
         manageDistUIState.getAssignedList().clear();
         manageDistUIState.getConsolidatedDistSoftwareList().clear();
-        getEventBus().publish(this, SaveActionWindowEvent.SAVED_ASSIGNMENTS);
     }
 
     private boolean validSoftwareModule(final Long distId, final SoftwareModule sm) {

hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/distributions/dstable/DistributionSetTableLayout.java

@@ -59,8 +59,8 @@ public class DistributionSetTableLayout extends AbstractTableLayout<Distribution
                         distributionAddUpdateWindowLayout),
                 distributionSetTable,
                 new DistributionSetDetails(i18n, eventBus, permissionChecker, manageDistUIState, null,
-                        distributionAddUpdateWindowLayout, softwareManagement, distributionSetManagement,
+                        distributionAddUpdateWindowLayout, distributionSetManagement, uiNotification,
-                        targetManagement, uiNotification, distributionSetTagManagement, popupLayout));
+                        distributionSetTagManagement, popupLayout));
     }
 
     public DistributionSetTable getDistributionSetTable() {