From 3073f68e1cc9140c24ffe038c88003a609e0cac5 Mon Sep 17 00:00:00 2001 From: Paul Luchyn Date: Fri, 11 Nov 2016 11:59:47 +0200 Subject: [PATCH] Fixed bug with foreign key constraint violation while order deleting Code refactor, API changes, changed logic of file and order deleting, minor fixes Code refactoring Test refactor, file deleting logic changes --- .../org/libreplan/web/orders/OrderModel.java | 9 +++ .../web/orders/files/IOrderFileModel.java | 9 ++- .../web/orders/files/OrderFileModel.java | 51 ++++++++++++++- .../orders/files/OrderFilesController.java | 14 ++-- .../libreplan/web/orders/OrderFilesTest.java | 65 +++++++++++++------ 5 files changed, 118 insertions(+), 30 deletions(-) diff --git a/libreplan-webapp/src/main/java/org/libreplan/web/orders/OrderModel.java b/libreplan-webapp/src/main/java/org/libreplan/web/orders/OrderModel.java index 0dab683e7..0a715b20b 100644 --- a/libreplan-webapp/src/main/java/org/libreplan/web/orders/OrderModel.java +++ b/libreplan-webapp/src/main/java/org/libreplan/web/orders/OrderModel.java @@ -59,6 +59,7 @@ import org.libreplan.business.orders.entities.Order; import org.libreplan.business.orders.entities.OrderElement; import org.libreplan.business.orders.entities.OrderLineGroup; import org.libreplan.business.orders.entities.OrderStatusEnum; +import org.libreplan.business.orders.entities.OrderFile; import org.libreplan.business.planner.entities.PositionConstraintType; import org.libreplan.business.qualityforms.daos.IQualityFormDAO; import org.libreplan.business.qualityforms.entities.QualityForm; @@ -84,6 +85,7 @@ import org.libreplan.business.users.entities.UserRole; import org.libreplan.web.calendars.BaseCalendarModel; import org.libreplan.web.common.IntegrationEntityModel; import org.libreplan.web.common.concurrentdetection.OnConcurrentModification; +import org.libreplan.web.orders.files.IOrderFileModel; import org.libreplan.web.orders.labels.LabelsOnConversation; import org.libreplan.web.planner.order.ISaveCommand.IBeforeSaveActions; import org.libreplan.web.planner.order.PlanningStateCreator; @@ -169,6 +171,9 @@ public class OrderModel extends IntegrationEntityModel implements IOrderModel { @Autowired private IOrderVersionDAO orderVersionDAO; + @Autowired + private IOrderFileModel orderFileModel; + private List orderList = new ArrayList<>(); @Override @@ -508,6 +513,10 @@ public class OrderModel extends IntegrationEntityModel implements IOrderModel { @Transactional public void remove(Order detachedOrder) { Order order = orderDAO.findExistingEntity(detachedOrder.getId()); + + List orderFiles = orderFileModel.findByParent(order); + orderFiles.forEach((orderFile -> orderFileModel.delete(orderFile))); + removeVersions(order); if ( order.hasNoVersions() ) { diff --git a/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/IOrderFileModel.java b/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/IOrderFileModel.java index 135157c9d..bfee19feb 100644 --- a/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/IOrderFileModel.java +++ b/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/IOrderFileModel.java @@ -30,7 +30,14 @@ public interface IOrderFileModel { List getAll(); - void delete(OrderFile file); + /** + * This method is used to delete OrderFile and physical file asociated with it + * + * @param file {@link OrderFile} that need to be deleted + * @return true if file was deleted successfully. + * @return false if file was not deleted successfully. + */ + boolean delete(OrderFile file); List findByParent(OrderElement parent); diff --git a/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/OrderFileModel.java b/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/OrderFileModel.java index 7e074bcfa..4746ef67f 100644 --- a/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/OrderFileModel.java +++ b/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/OrderFileModel.java @@ -4,12 +4,15 @@ import org.libreplan.business.orders.daos.IOrderFileDAO; import org.libreplan.business.orders.entities.OrderElement; import org.libreplan.business.orders.entities.OrderFile; import org.libreplan.business.users.entities.User; +import org.libreplan.web.common.IConfigurationModel; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; +import org.zkoss.zkplus.spring.SpringUtil; +import java.io.File; import java.util.Date; import java.util.List; @@ -21,11 +24,15 @@ import java.util.List; @Scope(BeanDefinition.SCOPE_PROTOTYPE) public class OrderFileModel implements IOrderFileModel { + public static final String UNKNOWN_FILE_EXTENSION = "Unknown type"; + @Autowired private IOrderFileDAO fileDAO; private OrderFile orderFile; + private IConfigurationModel configurationModel; + @Override @Transactional public void confirmSave() { @@ -68,10 +75,23 @@ public class OrderFileModel implements IOrderFileModel { return fileDAO.getAll(); } + /** + * This method is used to delete OrderFile and physical file asociated with it. + * Also different cases can occur: + * - First case: there is no troubles and OrderFile is deleted from database + * and physical file is deleted from disc. + * - Second case: there is some troubles with deleting physical file from disc, but + * OrderFile is deleted from database. + * + * @param file {@link OrderFile} that needs to be deleted. + * @return true if file was deleted successfully. + * @return false if file was not successfully deleted . + */ @Override @Transactional - public void delete(OrderFile file){ + public boolean delete(OrderFile file) { fileDAO.delete(file); + return deletePhysicalFile(file); } @Override @@ -83,4 +103,33 @@ public class OrderFileModel implements IOrderFileModel { public OrderFile getOrderFile() { return orderFile; } + + private boolean deletePhysicalFile (OrderFile file) { + + try { + configurationModel = (IConfigurationModel) SpringUtil.getBean("configurationModel"); + configurationModel.init(); + + String projectCode = file.getParent().getCode(); + String directory = configurationModel.getRepositoryLocation() + "orders" + "/" + projectCode; + File fileToDelete; + + if (UNKNOWN_FILE_EXTENSION.equals(file.getType())) { + fileToDelete = new File(directory + "/" + file.getName()); + } + else { + fileToDelete = new File(directory + "/" + file.getName() + "." + file.getType()); + } + + return fileToDelete.delete(); + } catch (Exception ignored) { + /* + * org.zkoss.zk.ui.Execution("SpringUtil can be called only under ZK environment!") can occur if + * ZK environment is not raised, this can occur in JUnit tests + */ + return false; + } + } + + } diff --git a/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/OrderFilesController.java b/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/OrderFilesController.java index 794b2f5ab..4e0e9f632 100644 --- a/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/OrderFilesController.java +++ b/libreplan-webapp/src/main/java/org/libreplan/web/orders/files/OrderFilesController.java @@ -174,19 +174,13 @@ public class OrderFilesController extends GenericForwardComposer { return; } - if ( isRepositoryExists() ) { - String projectCode = orderElementModel.getOrderElement().getCode(); configurationModel.init(); - String directory = configurationModel.getRepositoryLocation() + "orders" + "/" + projectCode; - File fileToDelete = new File(directory + "/" + file.getName() + "." + file.getType()); - - boolean deleted = fileToDelete.delete(); + boolean deleted = orderFileModel.delete(file); if ( deleted ){ - orderFileModel.delete(file); messages.clearMessages(); messages.showMessage(Level.INFO, "File successfully deleted"); @@ -252,7 +246,11 @@ public class OrderFilesController extends GenericForwardComposer { orderFileModel.createNewFileObject(); orderFileModel.setFileName(FilenameUtils.getBaseName(media.getName())); - orderFileModel.setFileType(FilenameUtils.getExtension(media.getName())); + + orderFileModel.setFileType(FilenameUtils.getExtension(media.getName()).isEmpty() + ? OrderFileModel.UNKNOWN_FILE_EXTENSION + : FilenameUtils.getExtension(media.getName())); + orderFileModel.setUploadDate(new Date()); orderFileModel.setUploader(userDAO.findByLoginName(SecurityUtils.getSessionUserLoginName())); orderFileModel.setParent(orderElementModel.getOrderElement()); diff --git a/libreplan-webapp/src/test/java/org/libreplan/web/orders/OrderFilesTest.java b/libreplan-webapp/src/test/java/org/libreplan/web/orders/OrderFilesTest.java index e876799b7..3c1610837 100644 --- a/libreplan-webapp/src/test/java/org/libreplan/web/orders/OrderFilesTest.java +++ b/libreplan-webapp/src/test/java/org/libreplan/web/orders/OrderFilesTest.java @@ -65,6 +65,17 @@ import java.util.Date; WEBAPP_SPRING_SECURITY_CONFIG_FILE, WEBAPP_SPRING_SECURITY_CONFIG_TEST_FILE }) + +/** + * CRUD test + * 1. Add row to files table + * 2. Read it + * 3. Update it + * 4. Delete it + * + * Negative test + * 1. Create row with null field value and try to save it + */ public class OrderFilesTest { @Autowired @@ -82,30 +93,27 @@ public class OrderFilesTest { @Autowired private IHoursGroupDAO hoursGroupDAO; - /** - * CRUD test - * 1. Add row to files table - * 2. Read it - * 3. Update it - * 4. Delete it - * - * Negative test - * 1. Create row with null field value and try to save it - */ - @Test @Transactional - public void testCRUD() { + public void testCreate() { - // Create int sizeBefore = orderFileModel.getAll().size(); + createEntities(); + int sizeWithNewRow = orderFileModel.getAll().size(); assertEquals(sizeBefore + 1, sizeWithNewRow); - OrderFile orderFile = null; - // Read + removeEntities(); + } + + @Test + @Transactional + public void testRead() { + createEntities(); + + OrderFile orderFile = null; try { orderFile = orderFileModel.findByParent(orderElementDAO.findUniqueByCode("1a1k1k1k")).get(0); assertEquals(orderFile.getName(), "Index"); @@ -113,21 +121,38 @@ public class OrderFilesTest { e.printStackTrace(); } - // Update - orderFile.setName("yii2"); - orderFileModel.confirmSave(); + removeEntities(); + } + @Test + @Transactional + public void testUpdate() { + createEntities(); + + OrderFile orderFile = null; try { orderFile = orderFileModel.findByParent(orderElementDAO.findUniqueByCode("1a1k1k1k")).get(0); + orderFile.setName("yii2"); + orderFileModel.confirmSave(); assertTrue(orderFile.getName().equals("yii2")); } catch (InstanceNotFoundException e) { e.printStackTrace(); } - // Delete removeEntities(); + } + + @Test + @Transactional + public void testDelete() { + createEntities(); + + int sizeBefore = orderFileModel.getAll().size(); + + removeEntities(); + int sizeAfter = orderFileModel.getAll().size(); - assertEquals(sizeBefore, sizeAfter); + assertEquals(sizeBefore - 1, sizeAfter); } @Transactional