From ac7873a51e10aa7ecda3267631108dd1134beacc Mon Sep 17 00:00:00 2001 From: Nitesh Yadav Date: Tue, 6 Oct 2026 22:40:12 +0530 Subject: [PATCH] feat(shares): add preview-validate endpoint, fix document attachment 500 ClerkShareWithdrawalRequestApiResource: extracted validate(Long)'s checklist logic into buildShareWithdrawalChecks(Map) and added a new POST /preview-validate endpoint that runs it against a draft submission (same fields submit() accepts, same Sundry-downgrade rule applied first) instead of an already-persisted request. Lets the clerk see the exact checklist a manager will see on approval before the request is even submitted, with zero risk of the two checklists drifting apart, since both now call the same method. The one check validate() runs that a draft cannot -- "Maker and Checker are different users" -- is reported as a fixed reminder instead of silently dropped, since the clerk previewing this is necessarily the maker and no checker has been chosen yet. DocumentReadPlatformServiceImpl.retrieveDocumentContent(): a document/signature whose m_document row survives but whose underlying file is missing from the content store (the usual cause is a database copied between environments without its file store) was throwing an uncaught ContentStoreException, surfacing as a raw 500 on every fetch. Now caught and converted to the same DocumentNotFoundException (404) ImageReadPlatformServiceImpl already uses for the identical situation on client photos, so the frontend's existing graceful-fallback handling for a missing attachment applies here too. Co-Authored-By: Claude Opus 5 (1M context) --- .../DocumentReadPlatformServiceImpl.java | 43 ++++++- ...lerkShareWithdrawalRequestApiResource.java | 106 ++++++++++++++++-- 2 files changed, 137 insertions(+), 12 deletions(-) diff --git a/fineract-document/src/main/java/org/apache/fineract/infrastructure/documentmanagement/service/DocumentReadPlatformServiceImpl.java b/fineract-document/src/main/java/org/apache/fineract/infrastructure/documentmanagement/service/DocumentReadPlatformServiceImpl.java index 2437d810f..d83b0a795 100644 --- a/fineract-document/src/main/java/org/apache/fineract/infrastructure/documentmanagement/service/DocumentReadPlatformServiceImpl.java +++ b/fineract-document/src/main/java/org/apache/fineract/infrastructure/documentmanagement/service/DocumentReadPlatformServiceImpl.java @@ -18,20 +18,29 @@ */ package org.apache.fineract.infrastructure.documentmanagement.service; +import java.io.FileNotFoundException; +import java.nio.file.NoSuchFileException; import java.util.List; import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; import org.apache.fineract.infrastructure.contentstore.service.ContentStoreService; +import org.apache.fineract.infrastructure.core.exception.AbstractPlatformDomainRuleException; import org.apache.fineract.infrastructure.documentmanagement.data.DocumentContent; import org.apache.fineract.infrastructure.documentmanagement.data.DocumentData; import org.apache.fineract.infrastructure.documentmanagement.domain.DocumentRepository; +import org.apache.fineract.infrastructure.documentmanagement.exception.DocumentInvalidRequestException; import org.apache.fineract.infrastructure.documentmanagement.exception.DocumentNotFoundException; import org.apache.fineract.infrastructure.documentmanagement.mapping.DocumentMapper; import org.springframework.stereotype.Service; +@Slf4j @Service @RequiredArgsConstructor public class DocumentReadPlatformServiceImpl implements DocumentReadPlatformService { + /** Bound on the cause walk below, so a self-referencing cause chain cannot spin. */ + private static final int MAX_CAUSE_DEPTH = 10; + private final ContentStoreService storeService; private final DocumentRepository documentRepository; private final DocumentMapper documentMapper; @@ -52,9 +61,39 @@ public class DocumentReadPlatformServiceImpl implements DocumentReadPlatformServ public DocumentContent retrieveDocumentContent(final String entityType, final Long entityId, final Long documentId) { final var doc = documentRepository.findByIdAndParentEntityTypeAndParentEntityId(documentId, entityType, entityId) .orElseThrow(() -> new DocumentNotFoundException(entityType, entityId, documentId)); - final var is = storeService.download(doc.getLocation()); + try { + final var is = storeService.download(doc.getLocation()); + return documentMapper.map(doc, is); + } catch (final AbstractPlatformDomainRuleException e) { + throw e; + } catch (final Exception e) { + if (isMissingFromContentStore(e)) { + // The m_document row survives but the file it points at does not — the usual cause is + // a database copied between environments without its file store (see the identical + // guard in ImageReadPlatformServiceImpl). For the caller that is simply "this document + // has no content", not a server fault, so report it the same way as a missing document + // instead of letting a raw ContentStoreException surface as a 500. + log.warn("Document {} for {} {} is recorded but missing from the content store; treating it as not found", documentId, + entityType, entityId); + throw new DocumentNotFoundException(entityType, entityId, documentId); + } + throw new DocumentInvalidRequestException(e); + } + } - return documentMapper.map(doc, is); + /** + * Whether a failure bottoms out in "the stored file is not there", however it was wrapped on the + * way up (the file store rethrows as ContentStoreException). + */ + private boolean isMissingFromContentStore(final Throwable failure) { + Throwable cause = failure; + for (int depth = 0; cause != null && depth < MAX_CAUSE_DEPTH; depth++) { + if (cause instanceof NoSuchFileException || cause instanceof FileNotFoundException) { + return true; + } + cause = cause.getCause(); + } + return false; } @Override diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkwithdrawal/api/ClerkShareWithdrawalRequestApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkwithdrawal/api/ClerkShareWithdrawalRequestApiResource.java index e7dedf034..2b4bb563e 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkwithdrawal/api/ClerkShareWithdrawalRequestApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkwithdrawal/api/ClerkShareWithdrawalRequestApiResource.java @@ -390,6 +390,101 @@ public class ClerkShareWithdrawalRequestApiResource { public String validate(@PathParam("id") final Long id) { final AppUser currentUser = context.authenticatedUser(); final Map record = loadRecord(id); + final List> checks = buildShareWithdrawalChecks(record); + + final Long makerId = ((Number) record.get("clerk_id")).longValue(); + final boolean makerCheckerOk = !makerId.equals(currentUser.getId()); + checks.add(checkResult("Maker and Checker are different users", makerCheckerOk, + makerCheckerOk ? "Confirmed different users" : "You submitted this request — another manager must approve it")); + + final boolean allPassed = checks.stream().allMatch(c -> Boolean.TRUE.equals(c.get("pass"))); + final JsonObject res = new JsonObject(); + res.addProperty("allPassed", allPassed); + res.add("checks", gson.toJsonTree(checks)); + return gson.toJson(res); + } + + /** + * Lets the clerk see the same checklist the approver will see, before the request is even + * submitted — so a request that is going to fail a check (a missing GL mapping, an unverified + * KYC, an open AML alert) is caught while someone can still fix it, not after a manager has + * already spent time reviewing it. Takes the same fields {@link #submit(String)} does and + * applies the identical Sundry-downgrade rule, so what this previews is exactly what would be + * stored and exactly what {@link #validate(Long)} would later report on the saved request — + * never a second, independently-drifting copy of the rules. + * + *

+ * The one check {@link #validate(Long)} runs that this cannot — "Maker and Checker are + * different users" — has no real answer yet: the clerk previewing this *is* the maker, and no + * checker has been chosen. It is reported here as a fixed reminder rather than silently + * dropped, so the clerk sees why the two lists differ by exactly that one row, and knows the + * rule is still enforced later, at {@code approve()}. + */ + @POST + @Path("/preview-validate") + @Consumes(MediaType.APPLICATION_JSON) + @Produces(MediaType.APPLICATION_JSON) + public String previewValidate(final String body) { + context.authenticatedUser(); + final JsonElement root = fromJsonHelper.parse(body); + + final Long shareAccountId = fromJsonHelper.extractLongNamed("shareAccountId", root); + if (shareAccountId == null) { + throw apiError(400, "Share account id is required"); + } + final Long requestedShares = fromJsonHelper.extractLongNamed("requestedShares", root); + final String resolutionNumber = fromJsonHelper.extractStringNamed("resolutionNumber", root); + final String meetingDate = fromJsonHelper.extractStringNamed("meetingDate", root); + final Long settlementPaymentTypeId = fromJsonHelper.extractLongNamed("settlementPaymentTypeId", root); + final String settlementModeRaw = fromJsonHelper.extractStringNamed("settlementMode", root); + String settlementMode = settlementModeRaw == null || settlementModeRaw.isBlank() ? "SAVINGS" + : settlementModeRaw.trim().toUpperCase(); + Long settlementGlAccountId = fromJsonHelper.extractLongNamed("settlementGlAccountId", root); + + // Mirrors submit()'s own downgrade: naming the product's own Share Sundry GL as the + // destination means "stop here", not a real GL settlement — see submit() for the rationale. + if ("GL".equals(settlementMode) && settlementGlAccountId != null) { + final Long productId = jdbcTemplate.queryForList("SELECT product_id FROM m_share_account WHERE id = ?", shareAccountId) + .stream().findFirst().map(r -> ((Number) r.get("product_id")).longValue()).orElse(null); + if (productId != null) { + final Long glSundry = resolveGl(productId, FA_SHARES_SUSPENSE); + if (glSundry != null && glSundry.equals(settlementGlAccountId)) { + settlementMode = "NONE"; + settlementGlAccountId = null; + } + } + } + + final Map draftRecord = new java.util.HashMap<>(); + draftRecord.put("resolution_number", resolutionNumber); + draftRecord.put("meeting_date", meetingDate); + draftRecord.put("share_account_id", shareAccountId); + draftRecord.put("requested_shares", requestedShares != null ? requestedShares : 0L); + draftRecord.put("settlement_mode", settlementMode); + draftRecord.put("settlement_gl_account_id", settlementGlAccountId); + draftRecord.put("settlement_payment_type_id", settlementPaymentTypeId); + + final List> checks = buildShareWithdrawalChecks(draftRecord); + checks.add(checkResult("Maker and Checker are different users", true, + "Verified automatically when a different manager approves")); + + final boolean allPassed = checks.stream().allMatch(c -> Boolean.TRUE.equals(c.get("pass"))); + final JsonObject res = new JsonObject(); + res.addProperty("allPassed", allPassed); + res.add("checks", gson.toJsonTree(checks)); + return gson.toJson(res); + } + + /** + * The full checklist a checker sees before approving a share withdrawal — share balance and + * status, loan/tenure/guarantor exposure, settlement destination and payment type, and client + * KYC/risk/AML standing. Shared verbatim between {@link #validate(Long)} (an already-submitted + * request) and {@link #previewValidate(String)} (a draft that does not exist yet), so the two + * can never silently diverge. Reads {@code record} by the same string keys both callers + * populate: resolution_number, meeting_date, share_account_id, requested_shares, + * settlement_mode, settlement_gl_account_id, settlement_payment_type_id. + */ + private List> buildShareWithdrawalChecks(final Map record) { final List> checks = new ArrayList<>(); final String resolutionNumber = (String) record.get("resolution_number"); @@ -592,16 +687,7 @@ public class ClerkShareWithdrawalRequestApiResource { openAlerts != null && openAlerts > 0 ? openAlerts + " open alert(s)" : "None")); } - final Long makerId = ((Number) record.get("clerk_id")).longValue(); - final boolean makerCheckerOk = !makerId.equals(currentUser.getId()); - checks.add(checkResult("Maker and Checker are different users", makerCheckerOk, - makerCheckerOk ? "Confirmed different users" : "You submitted this request — another manager must approve it")); - - final boolean allPassed = checks.stream().allMatch(c -> Boolean.TRUE.equals(c.get("pass"))); - final JsonObject res = new JsonObject(); - res.addProperty("allPassed", allPassed); - res.add("checks", gson.toJsonTree(checks)); - return gson.toJson(res); + return checks; } private Map checkResult(final String label, final boolean pass, final String detail) {