diff --git a/WebRoot/js/uwpr.scheduler.js b/WebRoot/js/uwpr.scheduler.js index ad8feea9..30f7f164 100644 --- a/WebRoot/js/uwpr.scheduler.js +++ b/WebRoot/js/uwpr.scheduler.js @@ -265,11 +265,14 @@ linksdiv += "[Delete]"; linksdiv += "   "; linksdiv += "[Edit Dates & Operator]"; + linksdiv += ""; + + // Edit Project & Payment Method also acts on the selected blocks, so hide it too + // when none can be selected (every block is billed). + linksdiv += '
'; + linksdiv += "[Edit Project & Payment Method]"; linksdiv += "
"; } - linksdiv += '
'; - linksdiv += "[Edit Project & Payment Method]"; - linksdiv += "
"; linksdiv += "" diff --git a/src/PRMessageResources.properties b/src/PRMessageResources.properties index 0efe01de..2ee4f499 100644 --- a/src/PRMessageResources.properties +++ b/src/PRMessageResources.properties @@ -119,7 +119,7 @@ error.project.invalidgroup=Somehow, you have sent a group that is not a valid gr error.project.noabstract=You must supply an abstract for this project. error.project.longAbstract=Abstract cannot exceed 500 words. error.project.invalid.progress=Progress report should contain at least 20 words. -error.project.hasinstrumenttime=This project cannot be deleted because instrument time has been scheduled for it. Only projects with no scheduled instrument time can be deleted. +error.project.hasinstrumenttime=This project cannot be deleted because instrument time is recorded for it. Only projects with no instrument time, scheduled or cancelled, can be deleted. error.project.archivedpayment=This project is archived. Unarchive it before adding or deleting payment methods. error.project.archivedinstrumenttime=This project is archived. Unarchive it before changing instrument time. error.project.archivefailed.one=The project could not be updated and was left unchanged. @@ -173,12 +173,14 @@ error.costcenter.delete=Error deleting: {0} error.costcenter.edit=Error editing: {0} error.costcenter.export=Error exporting: {0} error.costcenter.invalidaccess=Invalid access: {0} +error.costcenter.notallowed={0} error.payment.infoincomplete=Incomplete information: {0} error.payment.invalidid=Invalid ID: {0} error.payment.invalidaccess=Invalid access: {0} error.payment.load=Error loading data: {0} error.payment.save=Error saving. {0} +error.payment.delete=This payment method could not be deleted. Please contact UWPR staff and quote reference {0} so they can resolve it. error.scheduler.invalidid=Invalid ID: {0} error.scheduler.save=Error saving. {0} diff --git a/src/org/uwpr/costcenter/InvoiceBlockCreator.java b/src/org/uwpr/costcenter/InvoiceBlockCreator.java index 05fb86df..f2c152f0 100644 --- a/src/org/uwpr/costcenter/InvoiceBlockCreator.java +++ b/src/org/uwpr/costcenter/InvoiceBlockCreator.java @@ -36,29 +36,42 @@ public InvoiceBlockCreator (Invoice invoice) { @Override public void blockExported(UsageBlockBase block) throws BillingInformationExporterException { - InvoiceInstrumentUsage oldSavedBlock = null; + List allRows; try { - oldSavedBlock = invoiceBlockDao.getInvoiceBlock(block.getID()); + allRows = invoiceBlockDao.getAllInvoiceRowsForUsage(block.getID()); } catch(SQLException e) { throw new BillingInformationExporterException("Error getting results from invoiceInstrumentUsage table.", e); } - // If there is already an entry in the table for this block it means this block - // has already been included in an invoice. If the invoice ID we have been given - // is different from the one associated with this block it means that this block - // is being included in multiple invoices. This should never happen - if(oldSavedBlock != null) { - if(oldSavedBlock.getInvoiceId() != invoice.getId()) { - throw new BillingInformationExporterException("Usage block with ID "+block.getID()+" is already part of another invoice"); + // A block with any invoiceInstrumentUsage row is already invoiced. A row for this invoice means the + // block is already on it (a re-export) -- skip it so saveBlocks does not add a duplicate. A row for + // any other invoice means the block is already invoiced elsewhere, or the row is an orphan left by a + // deleted invoice -- refuse rather than invoice over it. + boolean alreadyOnThisInvoice = false; + List otherRows = new ArrayList<>(); + for(InvoiceInstrumentUsage row: allRows) { + if(row.getInvoiceId() == invoice.getId()) { + alreadyOnThisInvoice = true; } + else { + otherRows.add("id " + row.getId() + " (invoice " + row.getInvoiceId() + ")"); + } + } + if(!otherRows.isEmpty()) { + throw new BillingInformationExporterException("Usage block " + block.getID() + + " already has an invoiceInstrumentUsage row for another invoice, " + otherRows + + ". Resolve it before invoicing this block."); } - // Add to blocks that will be invoiced - InvoiceInstrumentUsage invoiceBlock = new InvoiceInstrumentUsage(); - invoiceBlock.setInvoiceId(invoice.getId()); - invoiceBlock.setInstrumentUsageId(block.getID()); - invoicedBlocks.add(invoiceBlock); + // Add to blocks that will be invoiced, unless the block already has a row for this invoice, which + // would make saveBlocks insert a duplicate. + if(!alreadyOnThisInvoice) { + InvoiceInstrumentUsage invoiceBlock = new InvoiceInstrumentUsage(); + invoiceBlock.setInvoiceId(invoice.getId()); + invoiceBlock.setInstrumentUsageId(block.getID()); + invoicedBlocks.add(invoiceBlock); + } } public void updateBlock(UsageBlockBase block) throws BillingInformationExporterException @@ -81,8 +94,7 @@ else if(block.getStartDate().before(invoice.getBillStartDate())) // This SHOULD NOT happen, unless blocks in the previous billing cycle were not invoiced. try { - InvoiceInstrumentUsage invoicedBlock = InvoiceInstrumentUsageDAO.getInstance().getInvoiceBlock(block.getID()); - if(invoicedBlock != null) + if(InvoiceInstrumentUsageDAO.getInstance().isBlockInvoiced(block.getID())) { throw new BillingInformationExporterException("Cannot split block. It has already been invoiced. " + block.toString()); } diff --git a/src/org/uwpr/costcenter/InvoiceDAO.java b/src/org/uwpr/costcenter/InvoiceDAO.java index f7f0451b..268ad4f5 100644 --- a/src/org/uwpr/costcenter/InvoiceDAO.java +++ b/src/org/uwpr/costcenter/InvoiceDAO.java @@ -93,17 +93,33 @@ public Invoice getInvoice (Date startDate, Date endDate) throws SQLException { } public void delete(Invoice invoice) throws SQLException { - + String sql = "DELETE FROM invoice WHERE id="+invoice.getId(); Connection conn = null; Statement stmt = null; - + try { conn = DBConnectionManager.getMainDbConnection(); + conn.setAutoCommit(false); + + // Delete the invoice's usage links, then the invoice, on one connection. No trigger + // removes the links, and a link left behind reports its block as billed. + // invoiceInstrumentUsage is InnoDB, so a failure on the invoice delete rolls back the + // link delete and leaves the invoice re-deletable. invoice is MyISAM and commits on its + // own, so a commit failure after it still strands the links. + InvoiceInstrumentUsageDAO.getInstance().deleteBlocksForInvoice(conn, invoice.getId()); + stmt = conn.createStatement(); stmt.execute(sql); + + conn.commit(); + } + catch(SQLException e) { + if(conn != null) try {conn.rollback();} catch(SQLException ignored){} + throw e; } finally { + if(conn != null) try {conn.setAutoCommit(true);} catch(SQLException ignored){} if(conn != null) try {conn.close();} catch(SQLException e){} if(stmt != null) try {stmt.close();} catch(SQLException e){} } diff --git a/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java b/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java index 0048442b..17b28a97 100644 --- a/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java +++ b/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java @@ -10,6 +10,7 @@ import org.yeastrc.db.DBConnectionManager; import java.sql.*; +import java.util.ArrayList; import java.util.List; /** @@ -45,25 +46,56 @@ public void saveBlocks(Connection conn, List invoiceBloc } } - public InvoiceInstrumentUsage getInvoiceBlock (int instrumentUsageId) throws SQLException { - - String sql = "SELECT * FROM invoiceInstrumentUsage WHERE instrumentUsageID="+instrumentUsageId; + /** + * True if the block has any invoiceInstrumentUsage row. Any row means the block is invoiced -- a live + * invoice, or an orphan the pre-deploy cleanup was meant to remove -- so callers refuse to edit or + * delete it either way. + */ + public boolean isBlockInvoiced (int instrumentUsageId) throws SQLException { + + String sql = "SELECT 1 FROM invoiceInstrumentUsage WHERE instrumentUsageID = ? LIMIT 1"; Connection conn = null; - Statement stmt = null; + PreparedStatement stmt = null; ResultSet rs = null; - + try { conn = DBConnectionManager.getMainDbConnection(); - stmt = conn.createStatement(); - rs = stmt.executeQuery(sql); - - if(rs.next()) - { - InvoiceInstrumentUsage invoiceBlock = new InvoiceInstrumentUsage(); - invoiceBlock.setId(rs.getInt("id")); - invoiceBlock.setInvoiceId(rs.getInt("invoiceID")); - invoiceBlock.setInstrumentUsageId(rs.getInt("instrumentUsageID")); - return invoiceBlock; + stmt = conn.prepareStatement(sql); + stmt.setInt(1, instrumentUsageId); + rs = stmt.executeQuery(); + return rs.next(); + } + finally { + if(conn != null) try {conn.close();} catch(SQLException e){} + if(stmt != null) try {stmt.close();} catch(SQLException e){} + if(rs != null) try {rs.close();} catch(SQLException e){} + } + } + + /** + * Returns every invoiceInstrumentUsage row for the block. blockExported uses this to skip a block + * already on the invoice being built (a re-export) and to refuse one linked to any other invoice, + * whether that invoice still exists or is an orphan left by a deleted one. + */ + public List getAllInvoiceRowsForUsage (int instrumentUsageId) throws SQLException { + + String sql = "SELECT id, invoiceID, instrumentUsageID FROM invoiceInstrumentUsage WHERE instrumentUsageID = ?"; + Connection conn = null; + PreparedStatement stmt = null; + ResultSet rs = null; + List rows = new ArrayList<>(); + + try { + conn = DBConnectionManager.getMainDbConnection(); + stmt = conn.prepareStatement(sql); + stmt.setInt(1, instrumentUsageId); + rs = stmt.executeQuery(); + while(rs.next()) { + InvoiceInstrumentUsage row = new InvoiceInstrumentUsage(); + row.setId(rs.getInt("id")); + row.setInvoiceId(rs.getInt("invoiceID")); + row.setInstrumentUsageId(rs.getInt("instrumentUsageID")); + rows.add(row); } } finally { @@ -71,7 +103,36 @@ public InvoiceInstrumentUsage getInvoiceBlock (int instrumentUsageId) throws SQL if(stmt != null) try {stmt.close();} catch(SQLException e){} if(rs != null) try {rs.close();} catch(SQLException e){} } - - return null; + return rows; + } + + public void deleteBlocksForInvoice (Connection conn, int invoiceId) throws SQLException { + + String sql = "DELETE FROM invoiceInstrumentUsage WHERE invoiceID = ?"; + PreparedStatement stmt = null; + + try { + stmt = conn.prepareStatement(sql); + stmt.setInt(1, invoiceId); + stmt.executeUpdate(); + } + finally { + if(stmt != null) try {stmt.close();} catch(SQLException e){} + } + } + + public void deleteBlocksForUsage (Connection conn, int instrumentUsageId) throws SQLException { + + String sql = "DELETE FROM invoiceInstrumentUsage WHERE instrumentUsageID = ?"; + PreparedStatement stmt = null; + + try { + stmt = conn.prepareStatement(sql); + stmt.setInt(1, instrumentUsageId); + stmt.executeUpdate(); + } + finally { + if(stmt != null) try {stmt.close();} catch(SQLException e){} + } } } diff --git a/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java b/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java index b3fdded6..aa58a0bc 100644 --- a/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java +++ b/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java @@ -347,15 +347,6 @@ public int getUsageBlockCountForProject(int projectId) throws SQLException { return getUsageBlockCountForProject(projectId, false, false); } - /** - * Counts the blocks currently scheduled for the project. Blocks cancelled before - * 10.28.2022 carry deleted=1 and are excluded. - */ - public int getScheduledUsageBlockCountForProject(int projectId) throws SQLException { - - return getUsageBlockCountForProject(projectId, true, false); - } - /** * Counts the blocks scheduled for the project that have not ended yet. Cancelled blocks * are excluded. @@ -397,9 +388,6 @@ private int getUsageBlockCountForProject(int projectId, boolean scheduledOnly, b public void purge(UsageBlockBase block, Researcher researcher) throws SQLException { - // NOTE: There is a trigger on instrumentUsage table that will - // delete all entries in the instrumentUsagePayment where instrumentUsageID is equal to - // the given usageId Connection conn = null; try { @@ -422,9 +410,11 @@ private void delete(Connection conn, List blocks, Researcher res { return; } - // NOTE: There is a trigger on instrumentUsage table that will - // delete all entries in the instrumentUsagePayment where instrumentUsageID is equal to - // the given usageId + + // No trigger deletes a usage block's child rows, so delete them here on the caller's + // connection, each block's children before the block itself. + InstrumentUsagePaymentDAO paymentDao = InstrumentUsagePaymentDAO.getInstance(); + InvoiceInstrumentUsageDAO invoiceUsageDao = InvoiceInstrumentUsageDAO.getInstance(); PreparedStatement stmt = null; String sql = "DELETE FROM instrumentUsage WHERE id=?"; @@ -436,11 +426,15 @@ private void delete(Connection conn, List blocks, Researcher res for(UsageBlockBase block: blocks) { log.info("Deleting usage block ID "+block.getID()); + + paymentDao.deletePaymentsForUsage(conn, block.getID()); + invoiceUsageDao.deleteBlocksForUsage(conn, block.getID()); + stmt.setInt(1, block.getID()); stmt.executeUpdate(); - message = message == null ? "" : message + ": "; - logDao.logSignupPurged(conn, block, researcher.getID(), message + block.toString()); + String logMessage = message == null ? "" : message + ": "; + logDao.logSignupPurged(conn, block, researcher.getID(), logMessage + block.toString()); } } finally { diff --git a/src/org/uwpr/instrumentlog/InstrumentUsagePaymentDAO.java b/src/org/uwpr/instrumentlog/InstrumentUsagePaymentDAO.java index 11d3441f..d3684801 100644 --- a/src/org/uwpr/instrumentlog/InstrumentUsagePaymentDAO.java +++ b/src/org/uwpr/instrumentlog/InstrumentUsagePaymentDAO.java @@ -126,8 +126,12 @@ private Connection getConnection() throws SQLException { } public boolean hasInstrumentUsageForPayment(int paymentMethodId) throws SQLException { - - String sql = "SELECT count(*) FROM instrumentUsagePayment WHERE paymentMethodID = "+paymentMethodId; + + // Inner-join instrumentUsage so an instrumentUsagePayment row whose usage block no longer exists + // (an orphan) does not make the payment method look in use and stop it being deleted or edited. + String sql = "SELECT count(*) FROM instrumentUsagePayment iup" + + " INNER JOIN instrumentUsage iu ON iu.id = iup.instrumentUsageID" + + " WHERE iup.paymentMethodID = "+paymentMethodId; Connection conn = null; Statement stmt = null; ResultSet rs = null; @@ -151,29 +155,46 @@ public boolean hasInstrumentUsageForPayment(int paymentMethodId) throws SQLExcep return false; } - public void deletePaymentsForUsage (int instrumentUsageId) throws SQLException { + public void deletePaymentsForUsage (Connection conn, int instrumentUsageId) throws SQLException { - Connection conn = null; + String sql = "DELETE FROM instrumentUsagePayment WHERE instrumentUsageID = ?"; + PreparedStatement stmt = null; try { - deletePaymentsForUsage(conn, instrumentUsageId); + stmt = conn.prepareStatement(sql); + stmt.setInt(1, instrumentUsageId); + stmt.executeUpdate(); } finally { - if(conn != null) try {conn.close();} catch(SQLException e){} + if(stmt != null) try {stmt.close();} catch(SQLException e){} } } - public void deletePaymentsForUsage (Connection conn, int instrumentUsageId) throws SQLException { + /** + * Returns the instrumentUsageID of every instrumentUsagePayment row for the payment method. + * DeletePaymentMethodAction refuses to delete a method with live usage, so a non-empty result means + * orphaned rows whose usage block was purged. deletePaymentMethod uses this to refuse the delete + * and name the rows rather than delete them silently. + */ + public List getUsageIdsForPaymentMethod (Connection conn, int paymentMethodId) throws SQLException { - String sql = "DELETE FROM instrumentUsagePayment where instrumentUsageID="+instrumentUsageId; + String sql = "SELECT instrumentUsageID FROM instrumentUsagePayment WHERE paymentMethodID = ?"; PreparedStatement stmt = null; + ResultSet rs = null; + List usageIds = new ArrayList<>(); try { stmt = conn.prepareStatement(sql); - stmt.executeUpdate(); + stmt.setInt(1, paymentMethodId); + rs = stmt.executeQuery(); + while(rs.next()) { + usageIds.add(rs.getInt("instrumentUsageID")); + } } finally { + if(rs != null) try {rs.close();} catch(SQLException e){} if(stmt != null) try {stmt.close();} catch(SQLException e){} } + return usageIds; } } diff --git a/src/org/uwpr/scheduler/UsageBlockDeletableDecider.java b/src/org/uwpr/scheduler/UsageBlockDeletableDecider.java index 6a4a9ed8..de7e0612 100644 --- a/src/org/uwpr/scheduler/UsageBlockDeletableDecider.java +++ b/src/org/uwpr/scheduler/UsageBlockDeletableDecider.java @@ -37,8 +37,7 @@ public boolean isBlockEditable(UsageBlockBase block, User user, StringBuilder er Groups groupsMan = Groups.getInstance(); // If this block has already been billed it cannot be edited even by admins - InvoiceInstrumentUsage billedBlock = InvoiceInstrumentUsageDAO.getInstance().getInvoiceBlock(block.getID()); - if(billedBlock != null) { + if(InvoiceInstrumentUsageDAO.getInstance().isBlockInvoiced(block.getID())) { errorMessage.append("Block cannot be edited. It has already been billed."); return false; } @@ -80,8 +79,7 @@ public boolean isBlockEditable(UsageBlockBase block, User user, StringBuilder er public boolean isBlockDeletable(UsageBlockBase block, User user, StringBuilder errorMessage) throws SQLException { // If this block has already been billed it cannot be deleted even by admins - InvoiceInstrumentUsage billedBlock = InvoiceInstrumentUsageDAO.getInstance().getInvoiceBlock(block.getID()); - if(billedBlock != null) { + if(InvoiceInstrumentUsageDAO.getInstance().isBlockInvoiced(block.getID())) { errorMessage.append("Block cannot be deleted. It has already been billed."); return false; } diff --git a/src/org/uwpr/www/costcenter/ExportBillingInformationAction.java b/src/org/uwpr/www/costcenter/ExportBillingInformationAction.java index 97cb5892..3086f26d 100644 --- a/src/org/uwpr/www/costcenter/ExportBillingInformationAction.java +++ b/src/org/uwpr/www/costcenter/ExportBillingInformationAction.java @@ -73,6 +73,7 @@ public ActionForward execute(ActionMapping mapping, ActionForm form, Exception exception = null; Invoice invoice = null; + boolean invoiceCreated = false; // true only when this request created the invoice, not reused an existing one try { BillingInformationExcelExporter exporter = new BillingInformationExcelExporter(); @@ -95,6 +96,7 @@ public ActionForward execute(ActionMapping mapping, ActionForm form, invoice.setBillEndDate(endBillDate); invoice.setCreatedBy(user.getResearcher().getID()); InvoiceDAO.getInstance().save(invoice); + invoiceCreated = true; } InvoiceBlockCreator invoiceBlockCreator = new InvoiceBlockCreator(invoice); @@ -132,7 +134,10 @@ public ActionForward execute(ActionMapping mapping, ActionForm form, if(exception != null) { - if(invoice != null) { + // Only delete an invoice this request created. A reused invoice's links are committed billing + // records and delete() now cascades to them, so deleting it after a failed re-export would + // erase that invoice and its links. + if(invoice != null && invoiceCreated) { log.info("Deleting invoice ID: "+invoice.getId()); try { InvoiceDAO.getInstance().delete(invoice); diff --git a/src/org/uwpr/www/instrumentlog/JSONInstrumentUsageGetter.java b/src/org/uwpr/www/instrumentlog/JSONInstrumentUsageGetter.java index 97d64d53..826a3083 100644 --- a/src/org/uwpr/www/instrumentlog/JSONInstrumentUsageGetter.java +++ b/src/org/uwpr/www/instrumentlog/JSONInstrumentUsageGetter.java @@ -310,10 +310,8 @@ private JSONObject getForContiguousBlocks(List blocks, e.printStackTrace(); } // If this block has already been billed it cannot be deleted or edited even by admins - InvoiceInstrumentUsage billedBlock = null; try { - billedBlock = invoiceInstrumentUsageDao.getInvoiceBlock(block.getID()); - if(billedBlock == null) { + if(!invoiceInstrumentUsageDao.isBlockInvoiced(block.getID())) { blockObject.put("editable", true); } } catch (SQLException e) { diff --git a/src/org/uwpr/www/scheduler/DeleteProjectInstrumentTimeAction.java b/src/org/uwpr/www/scheduler/DeleteProjectInstrumentTimeAction.java index 4774cbc3..5bfa10a7 100644 --- a/src/org/uwpr/www/scheduler/DeleteProjectInstrumentTimeAction.java +++ b/src/org/uwpr/www/scheduler/DeleteProjectInstrumentTimeAction.java @@ -140,7 +140,7 @@ public ActionForward execute(ActionMapping mapping, ActionForm form, if(!UsageBlockDeletableDecider.getInstance().isBlockDeletable(usageBlock, user, errorMessage)) { ActionErrors errors = new ActionErrors(); - errors.add("scheduler", new ActionMessage("error.scheduler.invalidaccess", + errors.add("scheduler", new ActionMessage("error.costcenter.notallowed", errorMessage.toString())); saveErrors( request, errors ); ActionForward fwd = mapping.findForward("Failure"); diff --git a/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java b/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java index 6a9cd2dc..9261943a 100644 --- a/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java +++ b/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java @@ -197,10 +197,9 @@ public ActionForward execute(ActionMapping mapping, ActionForm form, // A caller without access to the block's project must not learn its billing state, so the // already-billed check runs after the source-project access check above. for(UsageBlockBase block: blocksToUpdate) { - InvoiceInstrumentUsage billedBlock = InvoiceInstrumentUsageDAO.getInstance().getInvoiceBlock(block.getID()); - if(billedBlock != null) { + if(InvoiceInstrumentUsageDAO.getInstance().isBlockInvoiced(block.getID())) { return returnError(mapping, request, "scheduler", - new ActionMessage("error.costcenter.invalidaccess", + new ActionMessage("error.costcenter.notallowed", "Usage block : "+block.getID() +" has already been billed."), "viewScheduler", "?projectId="+projectId+"&instrumentId="+instrumentId); } diff --git a/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java b/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java index 7a3c9ce2..1ecfc829 100644 --- a/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java +++ b/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java @@ -246,10 +246,9 @@ public int compare(UsageBlockBase blk1, UsageBlockBase blk2) { UsageBlockBase firstBlock = blocksToUpdate.get(0); // If the first block in the range has already been billed, throw an error message. - InvoiceInstrumentUsage billedBlock = InvoiceInstrumentUsageDAO.getInstance().getInvoiceBlock(firstBlock.getID()); - if(billedBlock != null) { + if(InvoiceInstrumentUsageDAO.getInstance().isBlockInvoiced(firstBlock.getID())) { ActionErrors errors = new ActionErrors(); - errors.add("scheduler", new ActionMessage("error.costcenter.invalidaccess", + errors.add("scheduler", new ActionMessage("error.costcenter.notallowed", "The first block ( "+firstBlock.getStartDateFormated()+" - "+firstBlock.getEndDateFormated()+ ") in the selected range has already been billed. Please select blocks that have not been billed.")); saveErrors( request, errors ); diff --git a/src/org/yeastrc/files/DataFileDeleter.java b/src/org/yeastrc/files/DataFileDeleter.java index 68da7f09..93c412d2 100644 --- a/src/org/yeastrc/files/DataFileDeleter.java +++ b/src/org/yeastrc/files/DataFileDeleter.java @@ -30,22 +30,26 @@ public void deleteDataFile( DataFile datafile ) throws Exception { PreparedStatement stmt = null; try { - - String sql = "DELETE FROM files WHERE id = ?"; + conn = DBConnectionManager.getPrConnection(); - stmt = conn.prepareStatement( sql ); - stmt.setInt( 1, datafile.getId() ); - stmt.executeUpdate(); - stmt.close(); stmt = null; - + + // Delete the link rows before the blob. A failure between them would otherwise leave a + // location row pointing at a deleted file, the same kind of orphan the cascade-deletes + // work is closing. for( String t : DataFileDataUtils.getTypeLocationMap().values() ) { - sql = "DELETE FROM " + t + " WHERE file_id = ?"; + String sql = "DELETE FROM " + t + " WHERE file_id = ?"; stmt = conn.prepareStatement( sql ); stmt.setInt( 1, datafile.getId() ); stmt.executeUpdate(); stmt.close(); stmt = null; } - + + String sql = "DELETE FROM files WHERE id = ?"; + stmt = conn.prepareStatement( sql ); + stmt.setInt( 1, datafile.getId() ); + stmt.executeUpdate(); + stmt.close(); stmt = null; + } finally { if (stmt != null) { diff --git a/src/org/yeastrc/project/BilledProject.java b/src/org/yeastrc/project/BilledProject.java index 18bb6e59..ed5208d0 100644 --- a/src/org/yeastrc/project/BilledProject.java +++ b/src/org/yeastrc/project/BilledProject.java @@ -180,6 +180,10 @@ public void save() throws InvalidIDException, SQLException { public void delete() throws InvalidIDException, SQLException { + // Validate the subtype row exists before deleting any child rows so a missing tblBilledProject + // does not leave a project stripped of its researchers, payment links and external data. + requireBilledProjectRow(); + // Nothing here is atomic, so the order is what limits the damage when a step fails: // - the payment method links and the rows shared by all project types // - tblBilledProject, which is what makes this a billed project @@ -224,8 +228,32 @@ public void delete() throws InvalidIDException, SQLException { // re-initialize the id super.id = 0; } - - + + /** + * Throws InvalidIDException if this project has no tblBilledProject row. Called before any + * delete step so a missing subtype row does not leave a project stripped of its child rows. + */ + private void requireBilledProjectRow() throws InvalidIDException, SQLException { + + Connection conn = DBConnectionManager.getPrConnection(); + Statement stmt = null; + ResultSet rs = null; + + try { + stmt = conn.createStatement(); + rs = stmt.executeQuery("SELECT projectID FROM tblBilledProject WHERE projectID = " + super.id); + if( !rs.next() ) { + throw new InvalidIDException("Attempted to delete a Billed Project not found in the database."); + } + } + finally { + if(rs != null) try {rs.close();} catch(SQLException e){} + if(stmt != null) try {stmt.close();} catch(SQLException e){} + if(conn != null) try {conn.close();} catch(SQLException e){} + } + } + + /** * Removes a group from the set of groups to which this project belongs. If the group * isn't in the set, nothing happens. diff --git a/src/org/yeastrc/project/Collaboration.java b/src/org/yeastrc/project/Collaboration.java index b267cd4d..c5ca265a 100644 --- a/src/org/yeastrc/project/Collaboration.java +++ b/src/org/yeastrc/project/Collaboration.java @@ -238,6 +238,10 @@ public void load(int id) throws InvalidIDException, SQLException { */ public void delete() throws InvalidIDException, SQLException { + // Validate the subtype row exists before deleting any child rows, so a missing tblCollaboration + // does not leave a project stripped of its reviewers, rejection causes and shared rows. + requireCollaborationRow(); + // Nothing here is atomic, so the order is what limits the damage when a step fails: // - the rejection causes, the reviewers, and the rows shared by all project types // - tblCollaboration, which is what makes this a collaboration project @@ -304,6 +308,30 @@ public void delete() throws InvalidIDException, SQLException { super.id = 0; } + /** + * Throws InvalidIDException if this project has no tblCollaboration row. Called before any + * delete step so a missing subtype row does not leave a project stripped of its child rows. + */ + private void requireCollaborationRow() throws InvalidIDException, SQLException { + + Connection conn = getConnection(); + Statement stmt = null; + ResultSet rs = null; + + try { + stmt = conn.createStatement(); + rs = stmt.executeQuery("SELECT projectID FROM tblCollaboration WHERE projectID = " + super.id); + if( !rs.next() ) { + throw new InvalidIDException("Attempted to delete a Collaboration Project not found in the database."); + } + } + finally { + if(rs != null) try {rs.close();} catch(SQLException e){} + if(stmt != null) try {stmt.close();} catch(SQLException e){} + if(conn != null) try {conn.close();} catch(SQLException e){} + } + } + // SET METHODS diff --git a/src/org/yeastrc/project/payment/PaymentMethodDAO.java b/src/org/yeastrc/project/payment/PaymentMethodDAO.java index 63dc7cf2..7e766f99 100644 --- a/src/org/yeastrc/project/payment/PaymentMethodDAO.java +++ b/src/org/yeastrc/project/payment/PaymentMethodDAO.java @@ -295,23 +295,32 @@ private Connection getConnection() throws SQLException } public void deletePaymentMethod(int paymentMethodId) throws SQLException { - - String sql = "DELETE FROM paymentMethod WHERE id="+paymentMethodId; + Connection conn = null; - Statement stmt = null; - try { conn = getConnection(); - stmt = conn.createStatement(); - int numRowsDeleted = stmt.executeUpdate(sql); - + deletePaymentMethod(conn, paymentMethodId); + } + finally { + if(conn != null) try {conn.close();} catch(SQLException ignored){} + } + } + + public void deletePaymentMethod(Connection conn, int paymentMethodId) throws SQLException { + + String sql = "DELETE FROM paymentMethod WHERE id = ?"; + PreparedStatement stmt = null; + + try { + stmt = conn.prepareStatement(sql); + stmt.setInt(1, paymentMethodId); + int numRowsDeleted = stmt.executeUpdate(); + if(numRowsDeleted == 0) { throw new SQLException("Deleting payment method failed, no rows affected."); } - } finally { - if(conn != null) try {conn.close();} catch(SQLException ignored){} if(stmt != null) try {stmt.close();} catch(SQLException ignored){} } } diff --git a/src/org/yeastrc/project/payment/ProjectPaymentMethodDAO.java b/src/org/yeastrc/project/payment/ProjectPaymentMethodDAO.java index 7abb7297..58f0ef04 100644 --- a/src/org/yeastrc/project/payment/ProjectPaymentMethodDAO.java +++ b/src/org/yeastrc/project/payment/ProjectPaymentMethodDAO.java @@ -7,6 +7,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.uwpr.instrumentlog.InstrumentUsagePaymentDAO; import org.yeastrc.db.DBConnectionManager; import org.yeastrc.project.ProjectMismatchException; @@ -192,24 +193,67 @@ public void savePaymentMethod(int projectId, PaymentMethod paymentMethod) throws } public void deletePaymentMethod(int paymentMethodId) throws SQLException { - - // first delete the payment method - PaymentMethodDAO.getInstance().deletePaymentMethod(paymentMethodId); - - // now delete the entry in the bridge table - // NOTE: there is a trigger on paymentMethod that will - // delete all entries in projectPaymentMethod that have this paymentMethodId - // unlinkProjectPaymentMethod(paymentMethodId, 0); - + + // No trigger cleans up the child rows, so delete the projectPaymentMethod bridge rows before the + // payment method. projectPaymentMethod is InnoDB and shares this transaction, so a failure rolls + // back and deletes nothing. paymentMethod is MyISAM, so it commits immediately. The one case + // left uncovered is a commit failure after the paymentMethod delete, which leaves orphaned + // projectPaymentMethod rows. getPaymentMethod logs those on load, and only paymentMethod on + // InnoDB would close the window. + Connection conn = null; + try { + conn = getConnection(); + conn.setAutoCommit(false); + + // The orphan cleanup should have removed every orphaned instrumentUsagePayment row before this + // code was deployed, and DeletePaymentMethodAction blocks deleting a method with live usage, so a + // method reaching here should have no instrumentUsagePayment rows. If it has any, an orphan has + // recurred -- refuse the delete and name the rows so an admin can find and clean them, rather than + // deleting them silently. + List orphanedUsageIds = InstrumentUsagePaymentDAO.getInstance().getUsageIdsForPaymentMethod(conn, paymentMethodId); + if(!orphanedUsageIds.isEmpty()) { + throw new SQLException("Payment method " + paymentMethodId + " has " + orphanedUsageIds.size() + + " orphaned instrumentUsagePayment row(s), for purged usage blocks " + orphanedUsageIds + + ". Clean up these rows before deleting the payment method."); + } + + // The bridge rows linking the method to its projects. + unlinkProjectPaymentMethod(conn, paymentMethodId, 0); + + // The payment method itself, last. + PaymentMethodDAO.getInstance().deletePaymentMethod(conn, paymentMethodId); + + conn.commit(); + } + catch(SQLException e) { + if(conn != null) try {conn.rollback();} catch(SQLException ignored){} + throw e; + } + finally { + if(conn != null) try {conn.setAutoCommit(true);} catch(SQLException ignored){} + if(conn != null) try {conn.close();} catch(SQLException e){} + } } public void unlinkProjectPaymentMethod(int paymentMethodId, int projectId) throws SQLException { - + + Connection conn = null; + try { + conn = getConnection(); + unlinkProjectPaymentMethod(conn, paymentMethodId, projectId); + } + finally { + if(conn != null) try {conn.close();} catch(SQLException e){} + } + } + + public void unlinkProjectPaymentMethod(Connection conn, int paymentMethodId, int projectId) throws SQLException { + if(paymentMethodId == 0 && projectId == 0) { log.error("paymentMethodId and projectId are both 0 in unlinkProjectPaymentMethod. Skipping..."); return; } - + String sql = "DELETE FROM projectPaymentMethod WHERE "; if(paymentMethodId != 0) { sql += "paymentMethodID="+paymentMethodId; @@ -219,20 +263,15 @@ public void unlinkProjectPaymentMethod(int paymentMethodId, int projectId) throw if(projectId != 0) { sql += " projectID="+projectId; } - - Connection conn = null; + Statement stmt = null; - ResultSet rs = null; - + try { - conn = getConnection(); stmt = conn.createStatement(); - stmt.executeUpdate(sql); + stmt.executeUpdate(sql); } finally { - if(conn != null) try {conn.close();} catch(SQLException e){} if(stmt != null) try {stmt.close();} catch(SQLException e){} - if(rs != null) try {rs.close();} catch(SQLException e){} } } diff --git a/src/org/yeastrc/www/project/DeleteProjectAction.java b/src/org/yeastrc/www/project/DeleteProjectAction.java index 05ab0cea..6f8e811b 100644 --- a/src/org/yeastrc/www/project/DeleteProjectAction.java +++ b/src/org/yeastrc/www/project/DeleteProjectAction.java @@ -83,18 +83,19 @@ public ActionForward execute( ActionMapping mapping, return mapping.findForward("standardHome"); } - // Refuse to delete a project with instrument time scheduled. The leftover instrumentUsage - // rows would break the monthly billing export and the instrument calendar. + // Refuse to delete a project with any instrument time against it, cancelled blocks included. + // Blocks cancelled before 10.28.2022 kept deleted=1 rows that getCostOld still bills, and any + // leftover instrumentUsage row breaks the monthly billing export and the instrument calendar. try { - if (InstrumentUsageDAO.getInstance().getScheduledUsageBlockCountForProject(projectID) > 0) { + if (InstrumentUsageDAO.getInstance().getUsageBlockCountForProject(projectID) > 0) { ActionErrors errors = new ActionErrors(); errors.add("project", new ActionMessage("error.project.hasinstrumenttime")); saveErrors( request, errors ); return mapping.findForward("standardHome"); } } catch (SQLException e) { - // The project loaded above, so it exists. Only the scheduled-time check failed. - log.error("Error checking scheduled instrument time for project " + projectID, e); + // The project loaded above, so it exists. Only the instrument-time check failed. + log.error("Error checking instrument time for project " + projectID, e); ActionErrors errors = new ActionErrors(); errors.add("project", new ActionMessage("error.project.instrumenttimecheckfailed")); saveErrors( request, errors ); diff --git a/src/org/yeastrc/www/project/ViewProjectAction.java b/src/org/yeastrc/www/project/ViewProjectAction.java index 6283325c..ed6f438d 100644 --- a/src/org/yeastrc/www/project/ViewProjectAction.java +++ b/src/org/yeastrc/www/project/ViewProjectAction.java @@ -110,13 +110,13 @@ public ActionForward execute( ActionMapping mapping, // Hides the link only -- ArchiveProjectsAction enforces this. request.setAttribute("canArchive", project.checkAccess(user.getResearcher())); - // Deletable only while no instrument time is scheduled. Hides the link only -- - // DeleteProjectAction enforces this. + // Deletable only while no instrument time is recorded, cancelled blocks included, matching + // the DeleteProjectAction guard. Hides the link only -- DeleteProjectAction enforces this. try { request.setAttribute("canDelete", - InstrumentUsageDAO.getInstance().getScheduledUsageBlockCountForProject(project.getID()) == 0); + InstrumentUsageDAO.getInstance().getUsageBlockCountForProject(project.getID()) == 0); } catch (SQLException e) { - log.error("Error checking scheduled instrument time for project " + project.getID(), e); + log.error("Error checking instrument time for project " + project.getID(), e); request.setAttribute("canDelete", false); } diff --git a/src/org/yeastrc/www/project/payment/DeletePaymentMethodAction.java b/src/org/yeastrc/www/project/payment/DeletePaymentMethodAction.java index 1a39b66e..c8bc90d5 100644 --- a/src/org/yeastrc/www/project/payment/DeletePaymentMethodAction.java +++ b/src/org/yeastrc/www/project/payment/DeletePaymentMethodAction.java @@ -5,6 +5,8 @@ */ package org.yeastrc.www.project.payment; +import java.util.UUID; + import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -31,7 +33,7 @@ */ public class DeletePaymentMethodAction extends Action { - private static final Logger log = LogManager.getLogger(SaveNewPaymentMethodAction.class); + private static final Logger log = LogManager.getLogger(DeletePaymentMethodAction.class); public ActionForward execute(ActionMapping mapping, ActionForm form, HttpServletRequest request, HttpServletResponse response) throws Exception { @@ -170,11 +172,17 @@ public ActionForward execute(ActionMapping mapping, ActionForm form, ppmDao.deletePaymentMethod(paymentMethodId); } catch(Exception e) { + // The failure detail -- including the ids of any orphaned rows that blocked the delete -- is + // logged, not shown. The user gets a generic message and a reference to quote, and an admin + // finds the matching log entry by that reference and cleans up. Same pattern as error.jsp. + String reference = UUID.randomUUID().toString(); + log.error("Error deleting payment method [" + reference + "], project " + projectId + + ", paymentMethod " + paymentMethodId, e); + ActionErrors errors = new ActionErrors(); - errors.add("costcenter", new ActionMessage("error.costcenter.delete", "Error deleting payment method."+e.getMessage())); + errors.add("costcenter", new ActionMessage("error.payment.delete", reference)); saveErrors( request, errors ); - log.error("Error deleting payment method", e); - + ActionForward fwd = mapping.findForward("Failure"); ActionForward newFwd = new ActionForward(fwd.getPath()+"?ID="+projectId, fwd.getRedirect()); return newFwd;