Repository: adessose/budgeteer
Issue: Remove additional information not possible

A contract may have one or more fields of “additional information” which may be used, e.g., for project specific contract ids.

Once I have added and saved some information to a specific contract and later decide to remove this piece of information from the field, the removal is not saved. Neither is a workaround, e.g., by replacing the information by whitespace only, supported. The information update is just ignored.

This should be fixed. Any edit result, including the empty field content, should be accepted and saved.

Changed files:
budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/contract/ContractServiceTest.java
budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/persistence/contract/ContractFieldEntity.java
budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/contract/ContractService.java

diff --git a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/persistence/contract/ContractFieldEntity.java b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/persistence/contract/ContractFieldEntity.java
index 3ea03cf5..e09882f0 100644
--- a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/persistence/contract/ContractFieldEntity.java
+++ b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/persistence/contract/ContractFieldEntity.java
@@ -1,6 +1,8 @@
 package org.wickedsource.budgeteer.persistence.contract;
 
+import lombok.AllArgsConstructor;
 import lombok.Data;
+import lombok.NoArgsConstructor;
 import org.wickedsource.budgeteer.persistence.project.ProjectContractField;
 
 import javax.persistence.*;
@@ -8,6 +10,7 @@ import java.io.Serializable;
 
 @Data
 @Entity
+@NoArgsConstructor
 @Table(name="CONTRACT_FIELD")
 public class ContractFieldEntity implements Serializable{
 
@@ -27,4 +30,9 @@ public class ContractFieldEntity implements Serializable{
     @JoinColumn(name = "CONTRACT_ID")
     private ContractEntity contract;
 
+    public ContractFieldEntity(ProjectContractField field, String value) {
+        this.field = field;
+        this.value = value;
+    }
+
 }
diff --git a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/contract/ContractService.java b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/contract/ContractService.java
index 50c047e7..5aa1511e 100644
--- a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/contract/ContractService.java
+++ b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/contract/ContractService.java
@@ -1,5 +1,6 @@
 package org.wickedsource.budgeteer.service.contract;
 
+import org.apache.commons.lang3.StringUtils;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.security.access.prepost.PreAuthorize;
 import org.springframework.stereotype.Service;
@@ -17,6 +18,8 @@ import org.wickedsource.budgeteer.web.pages.contract.overview.table.ContractOver
 import javax.transaction.Transactional;
 import java.math.BigDecimal;
 import java.util.*;
+import java.util.function.Function;
+import java.util.stream.Collectors;
 
 @Service
 @Transactional
@@ -38,7 +41,7 @@ public class ContractService {
     private ContractDataMapper mapper;
 
     @PreAuthorize("canReadProject(#projectId)")
-    public ContractOverviewTableModel getContractOverviewByProject(long projectId){
+    public ContractOverviewTableModel getContractOverviewByProject(long projectId) {
         ContractOverviewTableModel result = new ContractOverviewTableModel();
         result.setContracts(mapper.map(contractRepository.findByProjectId(projectId)));
         return result;
@@ -57,11 +60,11 @@ public class ContractService {
     }
 
     @PreAuthorize("canReadProject(#projectId)")
-    public ContractBaseData getEmptyContractModel(long projectId){
+    public ContractBaseData getEmptyContractModel(long projectId) {
         ProjectEntity project = projectRepository.findOne(projectId);
         ContractBaseData model = new ContractBaseData(projectId);
         Set<ProjectContractField> fields = project.getContractFields();
-        for(ProjectContractField field : fields){
+        for (ProjectContractField field : fields) {
             model.getContractAttributes().add(new DynamicAttributeField(field.getFieldName(), ""));
         }
         return model;
@@ -73,7 +76,7 @@ public class ContractService {
         contractEntity.setId(0);
         contractEntity.setProject(project);
 
-        if(contractBaseData.getContractId() != 0){
+        if (contractBaseData.getContractId() != 0) {
             contractEntity = contractRepository.findOne(contractBaseData.getContractId());
         }
         //Update basic information
@@ -85,37 +88,25 @@ public class ContractService {
         contractEntity.setLink(contractBaseData.getFileModel().getLink());
         contractEntity.setFileName(contractBaseData.getFileModel().getFileName());
         contractEntity.setFile(contractBaseData.getFileModel().getFile());
-        if(contractBaseData.getTaxRate() < 0) {
+        if (contractBaseData.getTaxRate() < 0) {
             throw new IllegalArgumentException("Taxrate must be positive.");
         } else {
             contractEntity.setTaxRate(new BigDecimal(contractBaseData.getTaxRate()));
         }
 
-        //update additional information of the current contract
-        for(DynamicAttributeField fields : contractBaseData.getContractAttributes()){
-            if(fields.getValue() != null && !fields.getValue().isEmpty()) {
-                boolean attributeFound = false;
-                //see, if the attribute already exists -> Update the value
-                for (ContractFieldEntity contractFieldEntity : contractEntity.getContractFields()) {
-                    if (contractFieldEntity.getField().getFieldName().equals(fields.getName().trim())) {
-                        contractFieldEntity.setValue(fields.getValue());
-                        attributeFound = true;
-                        break;
-                    }
-                }
-                // Create a new Attribute
-                if (!attributeFound) {
-                    // see if the Project already contains a field with this name. If not, create a new one
-                    ProjectContractField projectContractField = projectRepository.findContractFieldByName(contractBaseData.getProjectId(), fields.getName().trim());
-                    if (projectContractField == null) {
-                        projectContractField = new ProjectContractField(0, fields.getName().trim(), project);
-                    }
-                    ContractFieldEntity field = new ContractFieldEntity();
-                    field.setId(0);
-                    field.setField(projectContractField);
-                    field.setValue(fields.getValue() == null ? "" : fields.getValue().trim());
-                    contractEntity.getContractFields().add(field);
-                }
+        // Use LinkedHashMap as backing map implementation to ensure insertion order
+        Map<String, ContractFieldEntity> contractFields = contractEntity.getContractFields().stream()
+                .collect(Collectors.toMap(field -> field.getField().getFieldName(), Function.identity(), (a, b) -> a,
+                        LinkedHashMap::new));
+
+        for (DynamicAttributeField dynamicAttribute : contractBaseData.getContractAttributes()) {
+            ContractFieldEntity fieldEntity = contractFields.get(dynamicAttribute.getName().trim());
+            if (fieldEntity != null) {
+                fieldEntity.setValue(StringUtils.trimToEmpty(dynamicAttribute.getValue()));
+            } else {
+                ContractFieldEntity newFieldEntity = createNewContractField(contractBaseData, dynamicAttribute, project);
+                contractEntity.getContractFields().add(newFieldEntity);
+                contractFields.put(newFieldEntity.getField().getFieldName(), newFieldEntity);
             }
         }
         contractRepository.save(contractEntity);
@@ -145,7 +136,7 @@ public class ContractService {
         cal.setTime(contract.getStartDate());
         Calendar currentDate = Calendar.getInstance();
         currentDate.setTime(new Date());
-        while(cal.before(currentDate)) {
+        while (cal.before(currentDate)) {
             months.add(cal.getTime());
             cal.add(Calendar.MONTH, 1);
         }
@@ -156,8 +147,8 @@ public class ContractService {
     public List<Date> getMonthListForProjectId(long projectId) {
         List<ContractEntity> contracts = contractRepository.findByProjectId(projectId);
         Date startDate = new Date();
-        for(ContractEntity contract : contracts) {
-            if(contract.getStartDate().before(startDate)) {
+        for (ContractEntity contract : contracts) {
+            if (contract.getStartDate().before(startDate)) {
                 startDate = contract.getStartDate();
             }
         }
@@ -167,7 +158,7 @@ public class ContractService {
         cal.setTime(startDate);
         Calendar currentDate = Calendar.getInstance();
         currentDate.setTime(new Date());
-        while(cal.before(currentDate)) {
+        while (cal.before(currentDate)) {
             months.add(cal.getTime());
             cal.add(Calendar.MONTH, 1);
         }
@@ -179,4 +170,13 @@ public class ContractService {
         List<ContractEntity> contracts = contractRepository.findByProjectId(projectId);
         return (null != contracts && !contracts.isEmpty());
     }
+
+    private ContractFieldEntity createNewContractField(ContractBaseData contractBaseData,
+                                                       DynamicAttributeField dynamicAttribute,
+                                                       ProjectEntity project) {
+        ProjectContractField projectContractField = Optional
+                .ofNullable(projectRepository.findContractFieldByName(contractBaseData.getProjectId(), dynamicAttribute.getName().trim()))
+                .orElse(new ProjectContractField(0, dynamicAttribute.getName().trim(), project));
+        return new ContractFieldEntity(projectContractField, StringUtils.trimToEmpty(dynamicAttribute.getValue()));
+    }
 }
diff --git a/budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/contract/ContractServiceTest.java b/budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/contract/ContractServiceTest.java
index c0bfc214..35a0ab8e 100644
--- a/budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/contract/ContractServiceTest.java
+++ b/budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/contract/ContractServiceTest.java
@@ -5,6 +5,7 @@ import com.github.springtestdbunit.annotation.DatabaseOperation;
 import com.github.springtestdbunit.annotation.DatabaseSetup;
 import com.github.springtestdbunit.annotation.DatabaseTearDown;
 import org.apache.commons.lang3.time.DateUtils;
+import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -51,7 +52,7 @@ class ContractServiceTest extends ServiceIntegrationTestTemplate {
     @Test
     @DatabaseSetup("contractTest.xml")
     @DatabaseTearDown(value = "contractTest.xml", type = DatabaseOperation.DELETE_ALL)
-    void testSaveNewContract(){
+    void testSaveNewContract() {
         ContractBaseData testObject = new ContractBaseData();
         testObject.setBudget(MoneyUtil.createMoney(12));
         testObject.setContractId(0);
@@ -78,7 +79,7 @@ class ContractServiceTest extends ServiceIntegrationTestTemplate {
     @Test
     @DatabaseSetup("contractTest.xml")
     @DatabaseTearDown(value = "contractTest.xml", type = DatabaseOperation.DELETE_ALL)
-    void testSaveNewContract2(){
+    void testSaveNewContract2() {
         ContractBaseData testObject = new ContractBaseData();
         testObject.setContractId(0);
         testObject.setProjectId(2);
@@ -103,7 +104,7 @@ class ContractServiceTest extends ServiceIntegrationTestTemplate {
     @Test
     @DatabaseSetup("contractTest.xml")
     @DatabaseTearDown(value = "contractTest.xml", type = DatabaseOperation.DELETE_ALL)
-    void testUpdateContract(){
+    void testUpdateContract() {
         ContractBaseData testObject = service.getContractById(4);
 
         testObject.setBudget(MoneyUtil.createMoney(12));
@@ -128,8 +129,8 @@ class ContractServiceTest extends ServiceIntegrationTestTemplate {
         assertEquals(6, savedContract.getContractAttributes().size());
         for (int i = 0; i < 6; i++) {
             boolean found = false;
-            for(DynamicAttributeField field : savedContract.getContractAttributes()){
-                if(field.getName().equals(field.getValue()) && field.getValue().equals("test" + i)){
+            for (DynamicAttributeField field : savedContract.getContractAttributes()) {
+                if (field.getName().equals(field.getValue()) && field.getValue().equals("test" + i)) {
                     found = true;
                     break;
                 }
@@ -146,7 +147,7 @@ class ContractServiceTest extends ServiceIntegrationTestTemplate {
     @Test
     @DatabaseSetup("contractTest.xml")
     @DatabaseTearDown(value = "contractTest.xml", type = DatabaseOperation.DELETE_ALL)
-    void testUpdateContract1(){
+    void testUpdateContract1() {
         ContractBaseData testObject = service.getContractById(5);
 
         testObject.setBudget(MoneyUtil.createMoney(12));
@@ -172,8 +173,8 @@ class ContractServiceTest extends ServiceIntegrationTestTemplate {
         assertEquals(7, savedContract.getContractAttributes().size());
         for (int i = 0; i < 7; i++) {
             boolean found = false;
-            for(DynamicAttributeField field : savedContract.getContractAttributes()){
-                if(field.getName().equals(field.getValue()) && field.getValue().equals("test" + i)){
+            for (DynamicAttributeField field : savedContract.getContractAttributes()) {
+                if (field.getName().equals(field.getValue()) && field.getValue().equals("test" + i)) {
                     found = true;
                     break;
                 }
@@ -235,7 +236,7 @@ class ContractServiceTest extends ServiceIntegrationTestTemplate {
     private List<DynamicAttributeField> getListOfContractFields() {
         List<DynamicAttributeField> result = new LinkedList<DynamicAttributeField>();
         DynamicAttributeField data = new DynamicAttributeField();
-        for(int i = 0; i < 5; i++) {
+        for (int i = 0; i < 5; i++) {
             data = new DynamicAttributeField();
             data.setName("test" + i);
             data.setValue("test" + i);
@@ -257,7 +258,37 @@ class ContractServiceTest extends ServiceIntegrationTestTemplate {
         assertNull(service.getContractById(3));
     }
 
+    @Test
+    @DatabaseSetup("contractTest.xml")
+    @DatabaseTearDown(value = "contractTest.xml", type = DatabaseOperation.DELETE_ALL)
+    void shouldUpdateAttributeWithEmptyValue() {
+        ContractBaseData testObject = service.getContractById(4);
+        testObject.setContractAttributes(getListOfContractFields());
+        DynamicAttributeField field = testObject.getContractAttributes().get(0);
+        field.setValue(null);
+
+        long newContractId = service.save(testObject);
+
+        ContractBaseData savedContract = service.getContractById(newContractId);
+
+        Assertions.assertThat(savedContract.getContractAttributes())
+                .contains(new DynamicAttributeField(field.getName(), ""));
+    }
+
+    @Test
+    @DatabaseSetup("contractTest.xml")
+    @DatabaseTearDown(value = "contractTest.xml", type = DatabaseOperation.DELETE_ALL)
+    void shouldSaveAttributeWithEmptyValue() {
+        ContractBaseData testObject = service.getContractById(4);
+        testObject.setContractAttributes(getListOfContractFields());
+        DynamicAttributeField field = new DynamicAttributeField("new-key", null);
+        testObject.getContractAttributes().add(field);
 
+        long newContractId = service.save(testObject);
 
+        ContractBaseData savedContract = service.getContractById(newContractId);
 
+        Assertions.assertThat(savedContract.getContractAttributes())
+                .contains(new DynamicAttributeField(field.getName(), ""));
+    }
 }