Prevent deposit PDF 500s, preserve AdvancedDeposit filters, and improve Financial Reports UX by DawoudIO · Pull Request #7643 · ChurchCRM/CRM

DawoudIO · 2025-11-26T22:55:45Z

What Changed

Critical Bug (PledgeQuery): filterForAdvancedDeposit() was using LEFT JOIN for deposit date filtering, which doesn't enforce the deposit must exist. Fixed by using innerJoinWithDeposit() when datetype='Deposit' fix #7638

AdvancedDeposit CSV Export: No-data redirect wasn't preserving datetype + date parameters. Fixed.
TaxReport CSV Export: No-data redirect wasn't preserving date parameters. Fixed.
ZeroGivers CSV Export: No-data redirect wasn't preserving date parameters. Fixed.
Restores Advanced Deposit Report datetype filtering (Deposit vs Payment date) and preserves date filters across redirects.
Fixes deposit PDF/CSV server errors and noisy logs when deposits have no payments.
Adds client-side pre-checks and user notifications to prevent invalid export attempts.
Improves Financial Reports UI: moves inline styles to SCSS, de-cramps filters, and fixes datepicker z-index.

Type

Testing

Screenshots

Security Check

Introduces new input validation
Modifies authentication/authorization
Affects data privacy/GDPR

Code Quality

Database: Propel ORM only, no raw SQL
No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor)
Bootstrap CSS classes used
All CSS bundled via webpack

Pre-Merge

Tested locally
No new warnings
Build passes
Backward compatible (or migration documented)

… show user-friendly notification

…tify user

…ayments (avoid inline alert page)

…s; HEAD returns 404 status

…ort; restore datetype logic The filterForAdvancedDeposit method was added without proper $datetype support. When I added $datetype handling, it used useDepositQuery() on a LEFT JOIN, which doesn't filter correctly. The original 5.22.0 SQL used INNER JOIN deposit_dep. Now: - When datetype='Deposit': use innerJoinWithDeposit() + useDepositQuery() for deposit date filtering - When datetype='Payment': use leftJoinWithDeposit() for optional deposits, filter by plg_date - This matches the original SQL behavior and fixes reports showing no results when filtering by deposit date

… no-data redirect; use FinancialService for ORM data

… apply date filtering accordingly

…or all financial reports AdvancedDeposit, TaxReport, and ZeroGivers now consistently preserve date filters and report-specific parameters (datetype for AdvancedDeposit) when redirecting after no data is returned for CSV export. This matches the PDF no-data redirect behavior.

…RM/CRM into fix/financial-reports-ux

Copilot

Pull request overview

This PR addresses critical bugs in deposit PDF generation and Advanced Deposit Report filtering. The main changes restore datetype filtering (Deposit vs Payment date) for the Advanced Deposit Report, add client-side and server-side guards to prevent 500 errors when generating PDFs for deposits without payments, preserve filter parameters across CSV export redirects, and improve Financial Reports UI by extracting inline styles to SCSS.

Key Changes:

Adds datetype parameter to PledgeQuery::filterForAdvancedDeposit() to support filtering by either payment date or deposit date
Implements client-side and API-level validation to prevent PDF generation for empty deposits
Preserves date range and datetype parameters when redirecting back from "No Data" CSV exports

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/ChurchCRM/model/ChurchCRM/PledgeQuery.php	Adds datetype parameter and conditional INNER/LEFT JOIN logic for deposit date filtering; modifies payment method filtering approach
src/ChurchCRM/Service/FinancialService.php	Passes datetype parameter through to PledgeQuery
src/api/routes/finance/finance-deposits.php	Adds payment count validation before PDF generation with 404 response for empty deposits
src/Reports/PrintDeposit.php	Adds server-side guard that redirects with warning message when deposit has no payments
src/skin/js/FindDepositSlip.js	Implements client-side AJAX check for payments before allowing PDF export
src/skin/js/DepositSlipEditor.js	Implements client-side AJAX check for payments before PDF generation
src/Reports/AdvancedDeposit.php	Preserves DateStart, DateEnd, and datetype parameters in redirect URL
src/FinancialReports.php	Extracts and preserves date/datetype parameters from GET/POST for form repopulation
src/skin/scss/_financial-reports.scss	New SCSS file containing styles extracted from inline PHP
src/skin/churchcrm.scss	Imports the new _financial-reports.scss module

Copilot · 2025-11-26T22:59:50Z

src/ChurchCRM/model/ChurchCRM/PledgeQuery.php

     * @param array $familyIds Optional family IDs to filter
     * @param array $methods Optional payment methods to filter
     * @param array $classificationIds Optional classification IDs to filter
+     * @param string $datetype 'Payment' for pledge date, 'Deposit' for deposit date


The PHPDoc comment states "'Payment' for pledge date" but the parameter name is $datetype and the valid values are 'Payment' or 'Deposit'. This is confusing.

The comment should clarify what 'Payment' means in this context - it filters by the payment date (plg_date), not the deposit date.

Recommendation: Update the comment to be clearer:

* @param string $datetype 'Payment' for payment date (plg_date), 'Deposit' for deposit date (dep_date)

Suggested change

* @param string $datetype 'Payment' for pledge date, 'Deposit' for deposit date

* @param string $datetype 'Payment' for payment date (plg_date), 'Deposit' for deposit date (dep_date)

Copilot · 2025-11-26T22:59:51Z

src/ChurchCRM/model/ChurchCRM/PledgeQuery.php

+            foreach ($methods as $method) {
+                $this->addOr(PledgeQuery::create()->filterByMethod($method));
+            }


The payment method filtering logic is incorrect. Using addOr() with a new query instance doesn't combine filters properly in Propel. This will likely produce incorrect SQL.

The original code $this->filterByMethod($methods, \Propel\Runtime\ActiveQuery\Criteria::IN) was correct for filtering by multiple payment methods using an IN clause. The new loop-based approach with addOr() won't work as intended.

Recommendation: Revert this change to use the original IN clause approach:

if (!empty($methods)) { $this->filterByMethod($methods, \Propel\Runtime\ActiveQuery\Criteria::IN); }

Copilot · 2025-11-26T22:59:51Z

src/ChurchCRM/model/ChurchCRM/PledgeQuery.php

+            // For payment date, left join is fine since deposit is optional
+            $this->leftJoinWithDeposit();
        }
+


Trailing whitespace on line 113. This line contains only spaces after the closing brace, which should be removed for code cleanliness.

Suggested change

Copilot · 2025-11-26T22:59:51Z

src/skin/js/FindDepositSlip.js

        $.each(selectedRows, function (index, value) {
-            window.CRM.VerifyThenLoadAPIContent(
-                window.CRM.root + "/api/deposits/" + value.Id + "/" + type,
-            );
+            var url = window.CRM.root + "/api/deposits/" + value.Id + "/" + type;
+            if (type === 'pdf') {
+                // Check payments first; if none, notify and skip
+                $.ajax({
+                    method: 'GET',
+                    url: window.CRM.root + '/api/deposits/' + value.Id + '/payments',
+                    dataType: 'json'
+                }).done(function(data) {
+                    var count = Array.isArray(data) ? data.length : 0;
+                    if (count === 0) {
+                        window.CRM.notify(i18next.t('No payments on this deposit'), { type: 'warning', delay: 5000 });
+                        return;
+                    }
+                    window.CRM.VerifyThenLoadAPIContent(url);
+                }).fail(function(jqXHR) {
+                    var errorMsg = i18next.t('There was a problem retrieving the requested object');
+                    if (jqXHR.responseJSON && jqXHR.responseJSON.message) { errorMsg = jqXHR.responseJSON.message; }
+                    window.CRM.notify(errorMsg, { type: 'danger', delay: 7000 });
+                });
+            } else {
+                // csv or other types - proceed as before
+                window.CRM.VerifyThenLoadAPIContent(url);
+            }
        });


When multiple deposits are selected for PDF export, the AJAX requests are fired asynchronously in parallel (via $.each). If one fails, the others continue processing, which could lead to race conditions and inconsistent user feedback.

Additionally, if a user selects 10 deposits with no payments, they'll receive 10 separate warning notifications that may overlap or confuse the user.

Recommendation: Consider batching the validation check before the loop, or showing a consolidated message about which deposits failed validation.

Copilot · 2025-11-26T22:59:51Z

src/api/routes/finance/finance-deposits.php

+            return SlimUtils::renderJson($response->withStatus(404), ['message' => 'No Payments on this Deposit']);
+        }
+
        $deposit = DepositQuery::create()->findOneById($id);


Missing null check for the deposit. If a deposit with the given ID doesn't exist, findOneById() returns null, and calling getPDF() on null will cause a fatal error.

Recommendation: Add a null check before calling getPDF():

$deposit = DepositQuery::create()->findOneById($id); if ($deposit === null) { return SlimUtils::renderJson($response->withStatus(404), ['message' => 'Deposit not found']); } $deposit->getPDF();

Suggested change

$deposit = DepositQuery::create()->findOneById($id);

$deposit = DepositQuery::create()->findOneById($id);

if ($deposit === null) {

return SlimUtils::renderJson($response->withStatus(404), ['message' => 'Deposit not found']);

}

Copilot · 2025-11-26T22:59:52Z

src/Reports/PrintDeposit.php

+            RedirectUtils::absoluteRedirect($_SERVER['HTTP_REFERER']);
+        } else {
+            RedirectUtils::redirect('DepositSlipEditor.php?DepositSlipID=' . (int)$iDepositSlipID);
+        }


Potential open redirect vulnerability. Using $_SERVER['HTTP_REFERER'] directly in RedirectUtils::absoluteRedirect() without validation allows an attacker to control the redirect destination by manipulating the Referer header.

An attacker could craft a malicious link that redirects users to an external phishing site after they attempt to generate a PDF for an empty deposit.

Recommendation: Validate that the referer URL belongs to the same domain before redirecting:

if (array_key_exists('HTTP_REFERER', $_SERVER) && !empty($_SERVER['HTTP_REFERER'])) { $referer = $_SERVER['HTTP_REFERER']; $parsedReferer = parse_url($referer); $parsedRoot = parse_url(SystemURLs::getRootPath()); if (isset($parsedReferer['host']) && $parsedReferer['host'] === $parsedRoot['host']) { RedirectUtils::absoluteRedirect($referer); exit; } } RedirectUtils::redirect('DepositSlipEditor.php?DepositSlipID=' . (int)$iDepositSlipID);

Suggested change

RedirectUtils::absoluteRedirect($_SERVER['HTTP_REFERER']);

} else {

RedirectUtils::redirect('DepositSlipEditor.php?DepositSlipID=' . (int)$iDepositSlipID);

}

$referer = $_SERVER['HTTP_REFERER'];

$parsedReferer = parse_url($referer);

$parsedRoot = parse_url(SystemURLs::getRootPath());

if (isset($parsedReferer['host']) && $parsedReferer['host'] === $parsedRoot['host']) {

RedirectUtils::absoluteRedirect($referer);

exit;

}

}

RedirectUtils::redirect('DepositSlipEditor.php?DepositSlipID=' . (int)$iDepositSlipID);

- Clarify PledgeQuery datetype PHPDoc (payment vs deposit date filtering) - Remove trailing whitespace in PledgeQuery - Improve batch PDF export validation in FindDepositSlip (consolidate messages) * Validate all deposits before starting downloads * Show single summary message instead of 10 separate notifications * Prevents UI confusion and race conditions

DawoudIO added 8 commits November 26, 2025 14:40

DepositSlipEditor: prevent PDF download when deposit has no payments;…

46d1c98

… show user-friendly notification

FindDepositSlip: skip PDF export for deposits without payments and no…

2f6729e

…tify user

PrintDeposit: flash global warning and redirect when deposit has no p…

7510143

…ayments (avoid inline alert page)

API: deposits PDF route returns 404 JSON for deposits with no payment…

63cae9e

…s; HEAD returns 404 status

AdvancedDeposit: propagate datetype and preserve DateStart/DateEnd on…

69fa0e1

… no-data redirect; use FinancialService for ORM data

FinancialService: accept datetype in getAdvancedDepositReportData and…

7145fab

… apply date filtering accordingly

DawoudIO added this to the 6.2.0 milestone Nov 26, 2025

Copilot AI review requested due to automatic review settings November 26, 2025 22:55

DawoudIO requested a review from a team as a code owner November 26, 2025 22:55

DawoudIO added the bug label Nov 26, 2025

DawoudIO requested review from respencer and removed request for a team November 26, 2025 22:55

DawoudIO added the UI label Nov 26, 2025

DawoudIO requested review from DAcodedBEAT, MrClever, bigtigerku and grayeul November 26, 2025 22:55

Copilot started reviewing on behalf of DawoudIO November 26, 2025 22:56 View session

DawoudIO added 2 commits November 26, 2025 14:57

Merge branch 'master' into fix/financial-reports-ux

c4465de

Merge branch 'fix/financial-reports-ux' of https://github.com/ChurchC…

fa2f224

…RM/CRM into fix/financial-reports-ux

Copilot finished reviewing on behalf of DawoudIO November 26, 2025 22:58

Copilot AI reviewed Nov 26, 2025

View reviewed changes

DawoudIO added 6 commits November 26, 2025 15:20

Merge branch 'master' into fix/financial-reports-ux

0023549

format cleanup

6ee6b98

Update PledgeQuery.php

801169f

better error reporting

f2ec5e8

Update PrintDeposit.php

4bb5db8

DawoudIO added 2 commits November 26, 2025 15:38

Update PledgeQuery.php

aa34c08

better UX

6022891

DawoudIO merged commit 74f780f into master Nov 27, 2025
7 checks passed

DawoudIO deleted the fix/financial-reports-ux branch November 27, 2025 00:08

DawoudIO mentioned this pull request Nov 27, 2025

Advanced Deposit Report Returning “No Data Found” #7636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prevent deposit PDF 500s, preserve AdvancedDeposit filters, and improve Financial Reports UX#7643

Prevent deposit PDF 500s, preserve AdvancedDeposit filters, and improve Financial Reports UX#7643
DawoudIO merged 18 commits intomasterfrom
fix/financial-reports-ux

DawoudIO commented Nov 26, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	* @param string $datetype 'Payment' for pledge date, 'Deposit' for deposit date
	* @param string $datetype 'Payment' for payment date (plg_date), 'Deposit' for deposit date (dep_date)

-            RedirectUtils::absoluteRedirect($_SERVER['HTTP_REFERER']);
-        } else {
-            RedirectUtils::redirect('DepositSlipEditor.php?DepositSlipID=' . (int)$iDepositSlipID);
-        }
+            $referer = $_SERVER['HTTP_REFERER'];
+            $parsedReferer = parse_url($referer);
+            $parsedRoot = parse_url(SystemURLs::getRootPath());
+            if (isset($parsedReferer['host']) && $parsedReferer['host'] === $parsedRoot['host']) {
+                RedirectUtils::absoluteRedirect($referer);
+                exit;
+            }
+        }
+        RedirectUtils::redirect('DepositSlipEditor.php?DepositSlipID=' . (int)$iDepositSlipID);

Uh oh!

Conversation

DawoudIO commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Changed

Type

Testing

Screenshots

Security Check

Code Quality

Pre-Merge

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DawoudIO commented Nov 26, 2025 •

edited

Loading