AMP-31199 : Create API to fetch cumulative commitments by donor and p… - #4511
Open
brianbrix wants to merge 11 commits into
Open
AMP-31199 : Create API to fetch cumulative commitments by donor and p…#4511brianbrix wants to merge 11 commits into
brianbrix wants to merge 11 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a public API for cumulative donor commitment totals by year and portfolio filters.
Changes:
- Adds the endpoint and CORS handler.
- Calculates per-donor and aggregate totals.
- Adds response DTOs for yearly donor commitments.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
PublicPortalService.java |
Implements filtering and commitment aggregation. |
PublicEndpoint.java |
Exposes the new public endpoint. |
PublicDonorCommitmentsByYear.java |
Defines the aggregate response. |
PublicDonorCommitment.java |
Defines per-donor totals. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- The endpoint is documented as returning commitments, but this only supplies a default. A caller can send
funding-type: "Actual Disbursements"(or a list of measures);applyExtendedSettingsthen adds those measures, and the genericBigDecimalloop reports their sum as a donor commitment. Pin this report to the commitment measure, or reject settings that select anything else.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:404
- Checking
containsKeydoes not apply the advertised USD default when JSON contains"currency": null. In that case the report engine uses its own default currency, while the response'scurrencyis null, making the total's unit ambiguous and potentially incorrect. Treat a null value as absent.
if (!settings.containsKey(SettingsConstants.CURRENCY_ID)) {
settings.put(SettingsConstants.CURRENCY_ID, "USD");
}
result.setCurrency((String) settings.get(SettingsConstants.CURRENCY_ID));
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- This endpoint promises commitment totals, but it only defaults
funding-typewhen absent. A caller can supply Actual Disbursements (or a list of measures), and the loop below sums those values while still returning them as commitments. ForceACTUAL_COMMITMENTSfor this dedicated API instead of honoring an arbitrary measure.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
Comment on lines
+423
to
+424
| spec.addColumn(new ReportColumn(ColumnConstants.DONOR_ID)); | ||
| spec.setHierarchies(spec.getColumns()); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- This endpoint is specifically documented to return commitments, but a caller-provided
funding-typeis retained here.applyExtendedSettingsthen adds that value as the report measure, so valid requests can make this endpoint return disbursements (or sum several funding measures) while labeling them as commitments. Override this setting unconditionally with Actual Commitments.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:403
- Hard-coding USD bypasses AMP's configured public/user default currency.
SettingsUtils.applySettings(..., true)normally derives that value viaEndpointUtils.getDefaultCurrencyCode(), so an omitted currency here can produce totals in a different currency from the rest of the public portal (and a present null value leaves the response currency null). Use the effective configured default instead.
if (!settings.containsKey(SettingsConstants.CURRENCY_ID)) {
settings.put(SettingsConstants.CURRENCY_ID, "USD");
}
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:392
- The new report-building and aggregation path has no automated coverage, although this component already has
PublicEndpointTest. Please cover the cumulative end-date, donor alias/ID handling, zero totals, and commitment-only measure selection so regressions do not silently return incorrectly grouped financial totals.
public static PublicDonorCommitmentsByYear getDonorCommitmentsByYear(SettingsAndFiltersParameters config,
Integer year) {
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- The endpoint promises commitment totals, but a caller-supplied
funding-typeis preserved here.applyExtendedSettingsthen adds that measure (or every measure in a supplied list), and the row loop sums everyBigDecimal, so requesting disbursements or multiple funding types returns non-commitment/mixed totals labeled as commitments. Force the report measure to Actual Commitments while retaining the other settings.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
| spec.setSummaryReport(false); | ||
| //TODO broken by year configurable | ||
| spec.setGroupingCriteria(GroupingCriteria.GROUPING_TOTALS_ONLY); | ||
| spec.setSummaryReport(true); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- This endpoint promises commitment totals, but it only defaults
funding-typewhen the caller omits it. A caller can supply disbursements, planned funding, or a list of measures;applyExtendedSettingswill then add those measures and the loop below sums everyBigDecimal, returning a non-commitment (or mixed) value as a commitment. Force this report's measure to actual commitments while retaining the other settings.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
amp/src/main/java/org/digijava/module/message/jobs/AmpDonorFundingJob.java:397
ReportsDashboardnow declares these accessors in this same change, so reflective lookup is unnecessary and silently converts implementation/linkage mistakes into missing years. Direct calls retain compile-time checking and avoid reflection for every emitted row.
private String extractYear(ReportsDashboard report) {
try {
Object year = ReportsDashboard.class.getMethod("getYear").invoke(report);
return year != null ? year.toString() : null;
} catch (ReflectiveOperationException ignored) {
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- This endpoint promises commitment totals, but a caller-supplied
funding-typeis retained.applyExtendedSettingsthen adds that measure, so requesting Actual Disbursements (or a list of measures) returns non-commitment values undertotal. Force the report measure to Actual Commitments regardless of dashboard settings (or reject incompatible values).
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
amp/src/main/java/org/digijava/module/message/jobs/AmpDonorFundingJob.java:363
- Summary mode drops non-hierarchy simple-text columns such as
AMP_ID(NiReportsEngine.java:692-693andSimpleTextColumn.java:111-112). Since AMP ID is deliberately added after the hierarchies, it will never appear inleafHeaders;findOptionalColumnByNamethen returns null and every emitted dashboard row loses itsactivityIds. Keep the report non-summary while retaining yearly grouping so the child rows remain available toextractAmpIds.
spec.setSummaryReport(true);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- This endpoint is specifically for commitments, but a caller-supplied
funding-typeis preserved.applyExtendedSettingsthen adds that value as the report measure, so requestingActual Disbursements(or another funding type) returns that amount labeled as a commitment. Force the commitment measure here and keep only currency/calendar settings configurable.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:483
- The normalized reporting-system/portfolio filter is currently ignored.
ReportsUtil.configureFiltersdelegates simple filters throughFilterUtils.idToSimpleColumn, butFilterUtilshas no mapping forFiltersConstants.REPORTING_SYSTEM; therefore requests with either alias return totals across every reporting system. RegisterREPORTING_SYSTEM -> ColumnConstants.REPORTING_SYSTEMinFilterUtils.
if (filters.containsKey(REPORTING_SYSTEM_CAMEL_FILTER) && !filters.containsKey(REPORTING_SYSTEM_FILTER)) {
filters.put(REPORTING_SYSTEM_FILTER, filters.get(REPORTING_SYSTEM_CAMEL_FILTER));
}
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/common/FiltersEndpoint.java:400
- This exposes
reporting-systemas a selectable filter, but that filter ID is not registered inFilterUtils.idToSimpleColumn. Consumers can retrieve these values, yet passing the selected IDs to report APIs is silently ignored. Add the same mapping used by other simple filters inFilterUtils.
@ApiMethod(id = FiltersConstants.REPORTING_SYSTEM, name = ColumnConstants.REPORTING_SYSTEM)
@ApiOperation(value = "Retrieve the data needed for building the 'Reporting System' filter.",
notes = "The response contains 2 objects - the filter definition and the values. \n"
+ "The filter widget should create a tree for 'Reporting System' values.")
@FilterDefinition(tab = EPConstants.TAB_OTHER, columns = ColumnConstants.REPORTING_SYSTEM)
public Response getReportingSystem() {
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:395
- Validate
yearagainst the supported reporting range before using it. Any integer is currently accepted; invalid date text is caught and discarded byFilterUtils.addDateFilterRule, which can make the response echo the requested year while returning totals without the intended cutoff.
int targetYear = year == null ? Calendar.getInstance().get(Calendar.YEAR) : year;
result.setYear(targetYear);
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- Pin this endpoint to commitments instead of honoring a caller-provided funding type. Currently a request can set
funding-type-idto disbursements (or a list of measures), and the loop below will return those values—or their sum—under a commitments API.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
amp/src/main/java/org/digijava/module/message/jobs/AmpDonorFundingJob.java:418
- Use the newly added
ReportsDashboardaccessors directly. Reflection removes compile-time checking and silently collapses all records into a null-year key if lookup/invocation ever fails, even though these methods are part of the same compiled class.
private String extractYear(ReportsDashboard report) {
try {
Object year = ReportsDashboard.class.getMethod("getYear").invoke(report);
return year != null ? year.toString() : null;
} catch (ReflectiveOperationException ignored) {
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:400
- This endpoint promises commitment totals, but a caller-supplied
funding-typeis retained.applyExtendedSettingsthen reports that measure, andgetTotalForFilterssums it, so requesting disbursements (or a list of measures) returns non-commitment/combined values. Force the commitment measure for this endpoint rather than only defaulting it when absent.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
amp/src/main/java/org/digijava/module/message/jobs/AmpDonorFundingJob.java:363
- Summary reports discard non-hierarchy
SimpleTextColumns, while AMP ID is added as a non-hierarchy column below. Consequently AMP ID is absent fromleafHeaders,findOptionalColumnByNamereturns null, and every emitted record loses itsactivityIds. Keep the report non-summary (yearly grouping still applies independently) or make AMP ID available through the hierarchy.
spec.setSummaryReport(true);
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:428
- This executes the full report engine once per requested donor. A portfolio request containing many donors therefore incurs N report/database runs and scales linearly. Build one report with Donor Agency as a hierarchy and extract each donor's aggregate from its child area instead.
for (Long donorId : requestedDonorIds) {
Map<String, Object> donorFilters = copyMap(baseFilters);
donorFilters.put(DONOR_AGENCY_FILTER, Collections.singletonList(donorId));
BigDecimal donorTotal = getTotalForFilters(settings, donorFilters);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicPortalService.java:401
- This endpoint is documented as returning commitments, but it preserves any caller-supplied
funding-type.applyExtendedSettingsturns that value (or list of values) into report measures, andgetTotalForFiltersthen sums everyBigDecimalcell. A request selecting disbursements therefore returns disbursements, while a multi-measure request returns the sum of commitments and disbursements under a commitments-only response. Force the measure to Actual Commitments while retaining the other settings.
if (!settings.containsKey(SettingsConstants.FUNDING_TYPE_ID)) {
settings.put(SettingsConstants.FUNDING_TYPE_ID, SettingsConstants.DEFAULT_FUNDING_TYPE_ID);
}
Comment on lines
+426
to
+429
| for (Long donorId : requestedDonorIds) { | ||
| Map<String, Object> donorFilters = copyMap(baseFilters); | ||
| donorFilters.put(DONOR_AGENCY_FILTER, Collections.singletonList(donorId)); | ||
| BigDecimal donorTotal = getTotalForFilters(settings, donorFilters); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ortfolio