From 305909f3a51c4034c87ba68fb1eea3334ffd473a Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 12 Sep 2026 01:51:35 -0700 Subject: [PATCH 1/8] Delete the child rows of usage blocks, invoices and payment methods, since no trigger does Production has no triggers, so the "there is a trigger" comments in these delete paths were false and the child rows were left as orphans. This replaces the missing triggers with explicit deletes, each child before its parent so a failure leaves a loadable parent rather than a dangling child. * InstrumentUsageDAO.delete now deletes each block's instrumentUsagePayment and invoiceInstrumentUsage rows on the caller's connection before the block. Removed the two false trigger comments * InvoiceDAO.delete deletes the invoice's invoiceInstrumentUsage links first. This also covers the billing-export failure handler, which deletes the invoice through this method * InvoiceInstrumentUsageDAO.getInvoiceBlock inner-joins invoice, so a link left by a deleted invoice no longer reports its block as billed. Added deleteBlocksForInvoice and deleteBlocksForUsage * InstrumentUsagePaymentDAO.hasInstrumentUsageForPayment inner-joins instrumentUsage, so a split whose block is gone no longer makes a payment method look in use. Added deletePaymentsForPaymentMethod, and removed the broken deletePaymentsForUsage(int), which passed a null connection and had no callers * ProjectPaymentMethodDAO.deletePaymentMethod uncomments the projectPaymentMethod unlink (the direct cause of the 6 orphaned links on prod), clears the method's instrumentUsagePayment splits, and deletes both before the payment method Not tested. See TESTS in ai-uwpr-webapp. Co-Authored-By: Claude --- src/org/uwpr/costcenter/InvoiceDAO.java | 11 +++++- .../costcenter/InvoiceInstrumentUsageDAO.java | 38 +++++++++++++++++-- .../instrumentlog/InstrumentUsageDAO.java | 15 +++++--- .../InstrumentUsagePaymentDAO.java | 22 +++++++---- .../payment/ProjectPaymentMethodDAO.java | 30 +++++++++++---- 5 files changed, 89 insertions(+), 27 deletions(-) diff --git a/src/org/uwpr/costcenter/InvoiceDAO.java b/src/org/uwpr/costcenter/InvoiceDAO.java index f7f0451b..4a59a90d 100644 --- a/src/org/uwpr/costcenter/InvoiceDAO.java +++ b/src/org/uwpr/costcenter/InvoiceDAO.java @@ -93,13 +93,20 @@ 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(); + + // Delete the invoice's usage links first, on the same connection. There is no trigger + // to do it, so leaving them orphans invoiceInstrumentUsage rows, and an orphaned link + // then reports its block as billed. Children before parent, so a failure leaves the + // invoice loadable and re-deletable rather than stranding the links. + InvoiceInstrumentUsageDAO.getInstance().deleteBlocksForInvoice(conn, invoice.getId()); + stmt = conn.createStatement(); stmt.execute(sql); } diff --git a/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java b/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java index 0048442b..7b12def2 100644 --- a/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java +++ b/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java @@ -46,8 +46,12 @@ public void saveBlocks(Connection conn, List invoiceBloc } public InvoiceInstrumentUsage getInvoiceBlock (int instrumentUsageId) throws SQLException { - - String sql = "SELECT * FROM invoiceInstrumentUsage WHERE instrumentUsageID="+instrumentUsageId; + + // Inner-join invoice so a link left behind by a deleted invoice does not report the block + // as billed. Callers treat a non-null result as "already invoiced" and refuse to edit it. + String sql = "SELECT iiu.* FROM invoiceInstrumentUsage iiu" + + " INNER JOIN invoice i ON i.id = iiu.invoiceID" + + " WHERE iiu.instrumentUsageID="+instrumentUsageId; Connection conn = null; Statement stmt = null; ResultSet rs = null; @@ -71,7 +75,35 @@ 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; } + + public void deleteBlocksForInvoice (Connection conn, int invoiceId) throws SQLException { + + String sql = "DELETE FROM invoiceInstrumentUsage WHERE invoiceID="+invoiceId; + PreparedStatement stmt = null; + + try { + stmt = conn.prepareStatement(sql); + 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="+instrumentUsageId; + PreparedStatement stmt = null; + + try { + stmt = conn.prepareStatement(sql); + 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..9662520c 100644 --- a/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java +++ b/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java @@ -397,9 +397,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 +419,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,6 +435,10 @@ 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(); diff --git a/src/org/uwpr/instrumentlog/InstrumentUsagePaymentDAO.java b/src/org/uwpr/instrumentlog/InstrumentUsagePaymentDAO.java index 11d3441f..8fac257a 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 a split 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,21 +155,23 @@ 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="+instrumentUsageId; + PreparedStatement stmt = null; try { - deletePaymentsForUsage(conn, instrumentUsageId); + stmt = conn.prepareStatement(sql); + 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 { + public void deletePaymentsForPaymentMethod (Connection conn, int paymentMethodId) throws SQLException { - String sql = "DELETE FROM instrumentUsagePayment where instrumentUsageID="+instrumentUsageId; + String sql = "DELETE FROM instrumentUsagePayment WHERE paymentMethodID="+paymentMethodId; PreparedStatement stmt = null; try { diff --git a/src/org/yeastrc/project/payment/ProjectPaymentMethodDAO.java b/src/org/yeastrc/project/payment/ProjectPaymentMethodDAO.java index 7abb7297..5319a2fd 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,15 +193,28 @@ public void savePaymentMethod(int projectId, PaymentMethod paymentMethod) throws } public void deletePaymentMethod(int paymentMethodId) throws SQLException { - - // first delete the payment method + + // No trigger cleans up the child rows, and PaymentMethodDAO.deletePaymentMethod opens its own + // connection, so this is not one transaction. Delete the children before the payment method, + // so a failure part way leaves a harmless extra child row rather than an orphaned link. + + // The bridge rows linking the method to its projects. These are the 6 orphaned + // projectPaymentMethod rows on prod, from this line having been commented out. + unlinkProjectPaymentMethod(paymentMethodId, 0); + + // Any instrumentUsagePayment splits. DeletePaymentMethodAction refuses a method still in + // use, but a split whose usage block is gone now passes that check, so clear it here. + Connection conn = null; + try { + conn = getConnection(); + InstrumentUsagePaymentDAO.getInstance().deletePaymentsForPaymentMethod(conn, paymentMethodId); + } + finally { + if(conn != null) try {conn.close();} catch(SQLException e){} + } + + // Finally the payment method itself. 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); - } public void unlinkProjectPaymentMethod(int paymentMethodId, int projectId) throws SQLException { From 326e97835782055793a08959837b72732d52bbfb Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 12 Sep 2026 02:00:05 -0700 Subject: [PATCH 2/8] Made project delete safe against cancelled time and missing subtype rows * DeleteProjectAction and the canDelete link in ViewProjectAction counted only scheduled blocks, so a project whose blocks were all cancelled could still be deleted. Those deleted=1 rows stay in instrumentUsage and getCostOld still bills them, and any leftover row breaks the monthly export. Both now count every block through getUsageBlockCountForProject, and error.project.hasinstrumenttime says cancelled time counts too * BilledProject.delete and Collaboration.delete deleted the child rows before checking the subtype row existed. A project missing its tblBilledProject or tblCollaboration row (the unloadable husks) would be stripped of its researchers, payment links and external data and then throw. Each now validates the subtype row first, in requireBilledProjectRow / requireCollaborationRow Not tested. See TESTS in ai-uwpr-webapp. Co-Authored-By: Claude --- src/PRMessageResources.properties | 2 +- src/org/yeastrc/project/BilledProject.java | 33 +++++++++++++++++-- src/org/yeastrc/project/Collaboration.java | 28 ++++++++++++++++ .../www/project/DeleteProjectAction.java | 7 ++-- .../www/project/ViewProjectAction.java | 8 ++--- 5 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/PRMessageResources.properties b/src/PRMessageResources.properties index 0efe01de..fbcd49c4 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. diff --git a/src/org/yeastrc/project/BilledProject.java b/src/org/yeastrc/project/BilledProject.java index 18bb6e59..71a7aa85 100644 --- a/src/org/yeastrc/project/BilledProject.java +++ b/src/org/yeastrc/project/BilledProject.java @@ -180,6 +180,11 @@ public void save() throws InvalidIDException, SQLException { public void delete() throws InvalidIDException, SQLException { + // Validate the subtype row exists before deleting any child rows. If tblBilledProject is + // missing (the unloadable billed-project husks), deleting the children first would strip a + // live project of its researchers, payment links and external data and then throw. + 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 +229,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/www/project/DeleteProjectAction.java b/src/org/yeastrc/www/project/DeleteProjectAction.java index 05ab0cea..1b5cf914 100644 --- a/src/org/yeastrc/www/project/DeleteProjectAction.java +++ b/src/org/yeastrc/www/project/DeleteProjectAction.java @@ -83,10 +83,11 @@ 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 ); 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); } From ca8534b7fc3661d9fea8bbb5339a293c4ea96aa0 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 12 Sep 2026 02:00:12 -0700 Subject: [PATCH 3/8] Deleted a data file's link rows before the blob DataFileDeleter.deleteDataFile deleted the files row first and its location-table links afterward. A failure between them left a link pointing at a deleted file, the same orphan the cascade-deletes work is closing. Delete the links first, then the blob. Not tested. See TESTS in ai-uwpr-webapp. Co-Authored-By: Claude --- src/org/yeastrc/files/DataFileDeleter.java | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) 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) { From c47714afbc2feb1edf0d6050b5609897615a03a6 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 12 Sep 2026 02:44:01 -0700 Subject: [PATCH 4/8] Cleaned up after the count-method change and fixed two stale comments Found by /code-review max on this branch. No logic changed. * InstrumentUsageDAO -- removed getScheduledUsageBlockCountForProject, which had no callers left once DeleteProjectAction and ViewProjectAction switched to getUsageBlockCountForProject * DeleteProjectAction -- the catch-block comment and log line still said "scheduled" instrument time, though the guard now counts cancelled blocks too * InvoiceDAO -- reworded an ungrammatical comment on the invoiceInstrumentUsage cleanup Not tested. See TESTS in ai-uwpr-webapp. Co-Authored-By: Claude --- src/org/uwpr/costcenter/InvoiceDAO.java | 6 +++--- src/org/uwpr/instrumentlog/InstrumentUsageDAO.java | 9 --------- src/org/yeastrc/www/project/DeleteProjectAction.java | 4 ++-- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/src/org/uwpr/costcenter/InvoiceDAO.java b/src/org/uwpr/costcenter/InvoiceDAO.java index 4a59a90d..989f26a0 100644 --- a/src/org/uwpr/costcenter/InvoiceDAO.java +++ b/src/org/uwpr/costcenter/InvoiceDAO.java @@ -102,9 +102,9 @@ public void delete(Invoice invoice) throws SQLException { conn = DBConnectionManager.getMainDbConnection(); // Delete the invoice's usage links first, on the same connection. There is no trigger - // to do it, so leaving them orphans invoiceInstrumentUsage rows, and an orphaned link - // then reports its block as billed. Children before parent, so a failure leaves the - // invoice loadable and re-deletable rather than stranding the links. + // to do it, so not deleting them leaves orphaned invoiceInstrumentUsage rows, and an + // orphaned link then reports its block as billed. Children before parent, so a failure + // leaves the invoice loadable and re-deletable rather than stranding the links. InvoiceInstrumentUsageDAO.getInstance().deleteBlocksForInvoice(conn, invoice.getId()); stmt = conn.createStatement(); diff --git a/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java b/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java index 9662520c..f41fd351 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. diff --git a/src/org/yeastrc/www/project/DeleteProjectAction.java b/src/org/yeastrc/www/project/DeleteProjectAction.java index 1b5cf914..6f8e811b 100644 --- a/src/org/yeastrc/www/project/DeleteProjectAction.java +++ b/src/org/yeastrc/www/project/DeleteProjectAction.java @@ -94,8 +94,8 @@ public ActionForward execute( ActionMapping mapping, 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 ); From fdfc68e0fd3a86f11fa9cb670c5a4b788105384d Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Tue, 15 Sep 2026 12:51:02 -0700 Subject: [PATCH 5/8] Hardened the cascade-delete paths after code review * InvoiceDAO.delete and ProjectPaymentMethodDAO.deletePaymentMethod now delete their child and parent rows in one transaction, so a mid-way failure rolls back instead of stranding half a delete. invoice and paymentMethod are MyISAM, so rollback covers only the InnoDB child rows. * PaymentMethodDAO.deletePaymentMethod and unlinkProjectPaymentMethod gained Connection-taking overloads so the delete runs on one connection. The old signatures stay as wrappers. * deletePaymentMethod and InvoiceBlockCreator.blockExported now fail loudly on an orphaned row (its parent already deleted) instead of cleaning it silently. The error names every offending row id. * DeletePaymentMethodAction, which a regular researcher can reach, shows a generic message with a reference to quote rather than the raw row ids, and logs the detail under that reference for an admin, the same pattern as error.jsp. Its logger, wrongly named for SaveNewPaymentMethodAction, is also fixed. * InstrumentUsageDAO.delete builds the audit-log prefix in a local variable, fixing the ": " that accumulated across a multi-block delete. * The delete helpers in InstrumentUsagePaymentDAO and InvoiceInstrumentUsageDAO now bind the id as a PreparedStatement parameter. * BilledProject.delete -- fixed a comment that described a collaboration's child rows, not a billed project's. Co-Authored-By: Claude --- src/PRMessageResources.properties | 1 + .../uwpr/costcenter/InvoiceBlockCreator.java | 14 ++++ src/org/uwpr/costcenter/InvoiceDAO.java | 17 +++-- .../costcenter/InvoiceInstrumentUsageDAO.java | 41 ++++++++++- .../instrumentlog/InstrumentUsageDAO.java | 4 +- .../InstrumentUsagePaymentDAO.java | 27 +++++-- src/org/yeastrc/project/BilledProject.java | 5 +- .../project/payment/PaymentMethodDAO.java | 27 ++++--- .../payment/ProjectPaymentMethodDAO.java | 71 +++++++++++++------ .../payment/DeletePaymentMethodAction.java | 16 +++-- 10 files changed, 170 insertions(+), 53 deletions(-) diff --git a/src/PRMessageResources.properties b/src/PRMessageResources.properties index fbcd49c4..fa3be43d 100644 --- a/src/PRMessageResources.properties +++ b/src/PRMessageResources.properties @@ -179,6 +179,7 @@ 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..9f71bce1 100644 --- a/src/org/uwpr/costcenter/InvoiceBlockCreator.java +++ b/src/org/uwpr/costcenter/InvoiceBlockCreator.java @@ -37,8 +37,10 @@ public InvoiceBlockCreator (Invoice invoice) { public void blockExported(UsageBlockBase block) throws BillingInformationExporterException { InvoiceInstrumentUsage oldSavedBlock = null; + List allRows = null; try { oldSavedBlock = invoiceBlockDao.getInvoiceBlock(block.getID()); + allRows = invoiceBlockDao.getAllInvoiceRowsForUsage(block.getID()); } catch(SQLException e) { throw new BillingInformationExporterException("Error getting results from invoiceInstrumentUsage table.", e); @@ -53,6 +55,18 @@ public void blockExported(UsageBlockBase block) throws BillingInformationExporte throw new BillingInformationExporterException("Usage block with ID "+block.getID()+" is already part of another invoice"); } } + else if(!allRows.isEmpty()) { + // getInvoiceBlock inner-joins invoice, so a null result can still hide rows left by a deleted + // invoice (orphans). The orphan cleanup should have removed these before deploy, so if any are + // here, refuse to invoice over them and name them rather than adding a second row. + List orphanRows = new ArrayList<>(); + for(InvoiceInstrumentUsage orphan: allRows) { + orphanRows.add("id " + orphan.getId() + " (deleted invoice " + orphan.getInvoiceId() + ")"); + } + throw new BillingInformationExporterException("Usage block " + block.getID() + " has " + + allRows.size() + " orphaned invoiceInstrumentUsage row(s), " + orphanRows + + ". Clean up these rows before invoicing this block."); + } // Add to blocks that will be invoiced InvoiceInstrumentUsage invoiceBlock = new InvoiceInstrumentUsage(); diff --git a/src/org/uwpr/costcenter/InvoiceDAO.java b/src/org/uwpr/costcenter/InvoiceDAO.java index 989f26a0..268ad4f5 100644 --- a/src/org/uwpr/costcenter/InvoiceDAO.java +++ b/src/org/uwpr/costcenter/InvoiceDAO.java @@ -100,17 +100,26 @@ public void delete(Invoice invoice) throws SQLException { try { conn = DBConnectionManager.getMainDbConnection(); + conn.setAutoCommit(false); - // Delete the invoice's usage links first, on the same connection. There is no trigger - // to do it, so not deleting them leaves orphaned invoiceInstrumentUsage rows, and an - // orphaned link then reports its block as billed. Children before parent, so a failure - // leaves the invoice loadable and re-deletable rather than stranding the links. + // 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 7b12def2..259b119f 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; /** @@ -79,13 +80,48 @@ public InvoiceInstrumentUsage getInvoiceBlock (int instrumentUsageId) throws SQL return null; } + /** + * Returns every invoiceInstrumentUsage row for the block, without the invoice join getInvoiceBlock + * uses. getInvoiceBlock hides a row left by a deleted invoice (an orphan), so this exposes those + * orphaned rows, letting the invoicing path refuse a block that still carries 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 { + 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){} + } + return rows; + } + public void deleteBlocksForInvoice (Connection conn, int invoiceId) throws SQLException { - String sql = "DELETE FROM invoiceInstrumentUsage WHERE invoiceID="+invoiceId; + String sql = "DELETE FROM invoiceInstrumentUsage WHERE invoiceID = ?"; PreparedStatement stmt = null; try { stmt = conn.prepareStatement(sql); + stmt.setInt(1, invoiceId); stmt.executeUpdate(); } finally { @@ -95,11 +131,12 @@ public void deleteBlocksForInvoice (Connection conn, int invoiceId) throws SQLEx public void deleteBlocksForUsage (Connection conn, int instrumentUsageId) throws SQLException { - String sql = "DELETE FROM invoiceInstrumentUsage WHERE instrumentUsageID="+instrumentUsageId; + String sql = "DELETE FROM invoiceInstrumentUsage WHERE instrumentUsageID = ?"; PreparedStatement stmt = null; try { stmt = conn.prepareStatement(sql); + stmt.setInt(1, instrumentUsageId); stmt.executeUpdate(); } finally { diff --git a/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java b/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java index f41fd351..aa58a0bc 100644 --- a/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java +++ b/src/org/uwpr/instrumentlog/InstrumentUsageDAO.java @@ -433,8 +433,8 @@ private void delete(Connection conn, List blocks, Researcher res 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 8fac257a..d3684801 100644 --- a/src/org/uwpr/instrumentlog/InstrumentUsagePaymentDAO.java +++ b/src/org/uwpr/instrumentlog/InstrumentUsagePaymentDAO.java @@ -127,8 +127,8 @@ private Connection getConnection() throws SQLException { public boolean hasInstrumentUsageForPayment(int paymentMethodId) throws SQLException { - // Inner-join instrumentUsage so a split whose usage block no longer exists (an orphan) does - // not make the payment method look in use and stop it being deleted or edited. + // 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; @@ -157,11 +157,12 @@ public boolean hasInstrumentUsageForPayment(int paymentMethodId) throws SQLExcep public void deletePaymentsForUsage (Connection conn, int instrumentUsageId) throws SQLException { - String sql = "DELETE FROM instrumentUsagePayment where instrumentUsageID="+instrumentUsageId; + String sql = "DELETE FROM instrumentUsagePayment WHERE instrumentUsageID = ?"; PreparedStatement stmt = null; try { stmt = conn.prepareStatement(sql); + stmt.setInt(1, instrumentUsageId); stmt.executeUpdate(); } finally { @@ -169,17 +170,31 @@ public void deletePaymentsForUsage (Connection conn, int instrumentUsageId) thro } } - public void deletePaymentsForPaymentMethod (Connection conn, int paymentMethodId) 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 paymentMethodID="+paymentMethodId; + 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/yeastrc/project/BilledProject.java b/src/org/yeastrc/project/BilledProject.java index 71a7aa85..ed5208d0 100644 --- a/src/org/yeastrc/project/BilledProject.java +++ b/src/org/yeastrc/project/BilledProject.java @@ -180,9 +180,8 @@ public void save() throws InvalidIDException, SQLException { public void delete() throws InvalidIDException, SQLException { - // Validate the subtype row exists before deleting any child rows. If tblBilledProject is - // missing (the unloadable billed-project husks), deleting the children first would strip a - // live project of its researchers, payment links and external data and then throw. + // 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: 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 5319a2fd..58f0ef04 100644 --- a/src/org/yeastrc/project/payment/ProjectPaymentMethodDAO.java +++ b/src/org/yeastrc/project/payment/ProjectPaymentMethodDAO.java @@ -194,36 +194,66 @@ public void savePaymentMethod(int projectId, PaymentMethod paymentMethod) throws public void deletePaymentMethod(int paymentMethodId) throws SQLException { - // No trigger cleans up the child rows, and PaymentMethodDAO.deletePaymentMethod opens its own - // connection, so this is not one transaction. Delete the children before the payment method, - // so a failure part way leaves a harmless extra child row rather than an orphaned link. + // 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){} + } + } - // The bridge rows linking the method to its projects. These are the 6 orphaned - // projectPaymentMethod rows on prod, from this line having been commented out. - unlinkProjectPaymentMethod(paymentMethodId, 0); + public void unlinkProjectPaymentMethod(int paymentMethodId, int projectId) throws SQLException { - // Any instrumentUsagePayment splits. DeletePaymentMethodAction refuses a method still in - // use, but a split whose usage block is gone now passes that check, so clear it here. Connection conn = null; try { conn = getConnection(); - InstrumentUsagePaymentDAO.getInstance().deletePaymentsForPaymentMethod(conn, paymentMethodId); + unlinkProjectPaymentMethod(conn, paymentMethodId, projectId); } finally { if(conn != null) try {conn.close();} catch(SQLException e){} } - - // Finally the payment method itself. - PaymentMethodDAO.getInstance().deletePaymentMethod(paymentMethodId); } - public void unlinkProjectPaymentMethod(int paymentMethodId, int projectId) throws SQLException { - + 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; @@ -233,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/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; From 060f0540c669b03fc35b19ff8be2a24a72903ae0 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Wed, 16 Sep 2026 12:26:01 -0700 Subject: [PATCH 6/8] Addressed Copilot review on PR #12 and simplified the block-billed check * ExportBillingInformationAction deletes the invoice on a failed export only when this request created it. A re-export reuses the period's invoice and delete() cascades to its links, so deleting it on failure erased a committed invoice. (Copilot finding 2) * Replaced getInvoiceBlock with isBlockInvoiced -- any invoiceInstrumentUsage row means billed. Every caller only needed a yes-or-no check, and this is fail-safe, so an orphaned link now protects a block from edit and delete instead of leaving it open. * blockExported checks every row, skips one already on the invoice being built (no duplicate on re-export), and refuses one tied to another invoice. (Copilot finding 1) Co-Authored-By: Claude --- .../uwpr/costcenter/InvoiceBlockCreator.java | 54 +++++++++---------- .../costcenter/InvoiceInstrumentUsageDAO.java | 40 ++++++-------- .../scheduler/UsageBlockDeletableDecider.java | 6 +-- .../ExportBillingInformationAction.java | 7 ++- .../JSONInstrumentUsageGetter.java | 4 +- .../www/scheduler/EditBlockDetailsAction.java | 3 +- .../scheduler/EditBlockDetailsFormAction.java | 3 +- 7 files changed, 53 insertions(+), 64 deletions(-) diff --git a/src/org/uwpr/costcenter/InvoiceBlockCreator.java b/src/org/uwpr/costcenter/InvoiceBlockCreator.java index 9f71bce1..f2c152f0 100644 --- a/src/org/uwpr/costcenter/InvoiceBlockCreator.java +++ b/src/org/uwpr/costcenter/InvoiceBlockCreator.java @@ -36,43 +36,42 @@ public InvoiceBlockCreator (Invoice invoice) { @Override public void blockExported(UsageBlockBase block) throws BillingInformationExporterException { - InvoiceInstrumentUsage oldSavedBlock = null; - List allRows = 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 if(!allRows.isEmpty()) { - // getInvoiceBlock inner-joins invoice, so a null result can still hide rows left by a deleted - // invoice (orphans). The orphan cleanup should have removed these before deploy, so if any are - // here, refuse to invoice over them and name them rather than adding a second row. - List orphanRows = new ArrayList<>(); - for(InvoiceInstrumentUsage orphan: allRows) { - orphanRows.add("id " + orphan.getId() + " (deleted invoice " + orphan.getInvoiceId() + ")"); + else { + otherRows.add("id " + row.getId() + " (invoice " + row.getInvoiceId() + ")"); } - throw new BillingInformationExporterException("Usage block " + block.getID() + " has " - + allRows.size() + " orphaned invoiceInstrumentUsage row(s), " + orphanRows - + ". Clean up these rows before invoicing this block."); + } + 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 @@ -95,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/InvoiceInstrumentUsageDAO.java b/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java index 259b119f..17b28a97 100644 --- a/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java +++ b/src/org/uwpr/costcenter/InvoiceInstrumentUsageDAO.java @@ -46,44 +46,36 @@ public void saveBlocks(Connection conn, List invoiceBloc } } - public InvoiceInstrumentUsage getInvoiceBlock (int instrumentUsageId) throws SQLException { + /** + * 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 { - // Inner-join invoice so a link left behind by a deleted invoice does not report the block - // as billed. Callers treat a non-null result as "already invoiced" and refuse to edit it. - String sql = "SELECT iiu.* FROM invoiceInstrumentUsage iiu" - + " INNER JOIN invoice i ON i.id = iiu.invoiceID" - + " WHERE iiu.instrumentUsageID="+instrumentUsageId; + 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){} } - - return null; } /** - * Returns every invoiceInstrumentUsage row for the block, without the invoice join getInvoiceBlock - * uses. getInvoiceBlock hides a row left by a deleted invoice (an orphan), so this exposes those - * orphaned rows, letting the invoicing path refuse a block that still carries one. + * 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 { 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/EditBlockDetailsAction.java b/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java index 6a9cd2dc..48450005 100644 --- a/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java +++ b/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java @@ -197,8 +197,7 @@ 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", "Usage block : "+block.getID() +" has already been billed."), diff --git a/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java b/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java index 7a3c9ce2..2cc6c700 100644 --- a/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java +++ b/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java @@ -246,8 +246,7 @@ 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", "The first block ( "+firstBlock.getStartDateFormated()+" - "+firstBlock.getEndDateFormated()+ From 70364d7e5f60e6f10c1069d03006aff5e095584b Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Wed, 16 Sep 2026 12:26:34 -0700 Subject: [PATCH 7/8] Removed the misleading "Invalid access:" prefix from billed-block errors The already-billed messages on the edit and delete paths rendered under error.costcenter.invalidaccess / error.scheduler.invalidaccess, whose template is "Invalid access: {0}" -- wrong for a billing condition. Added error.costcenter.notallowed ({0}, no prefix) and pointed the billed messages in EditBlockDetailsAction, EditBlockDetailsFormAction, and DeleteProjectInstrumentTimeAction at it. Genuine access checks keep the old key. Co-Authored-By: Claude --- src/PRMessageResources.properties | 1 + .../uwpr/www/scheduler/DeleteProjectInstrumentTimeAction.java | 2 +- src/org/uwpr/www/scheduler/EditBlockDetailsAction.java | 2 +- src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/PRMessageResources.properties b/src/PRMessageResources.properties index fa3be43d..2ee4f499 100644 --- a/src/PRMessageResources.properties +++ b/src/PRMessageResources.properties @@ -173,6 +173,7 @@ 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} 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 48450005..9261943a 100644 --- a/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java +++ b/src/org/uwpr/www/scheduler/EditBlockDetailsAction.java @@ -199,7 +199,7 @@ public ActionForward execute(ActionMapping mapping, ActionForm form, for(UsageBlockBase block: blocksToUpdate) { 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 2cc6c700..1ecfc829 100644 --- a/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java +++ b/src/org/uwpr/www/scheduler/EditBlockDetailsFormAction.java @@ -248,7 +248,7 @@ public int compare(UsageBlockBase blk1, UsageBlockBase blk2) { // If the first block in the range has already been billed, throw an error message. 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 ); From 338703798525cffb2dc93cba1e82c02015edface Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Wed, 16 Sep 2026 12:26:41 -0700 Subject: [PATCH 8/8] Hid Edit Project & Payment Method when no scheduler blocks are editable [Delete] and [Edit Dates & Operator] were already hidden when a tooltip's block group had no editable (non-billed) blocks, but [Edit Project & Payment Method] rendered unconditionally, so clicking it hit an empty-selection alert. Moved it inside the same hasEditableBlocks check. Co-Authored-By: Claude --- WebRoot/js/uwpr.scheduler.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 += ""