Skip to content

AMP-31226: Add third dimension for disaggregation - #4522

Open
brianbrix wants to merge 49 commits into
developfrom
feature/AMP-31226/New-Disaggregation-Dimension
Open

AMP-31226: Add third dimension for disaggregation#4522
brianbrix wants to merge 49 commits into
developfrom
feature/AMP-31226/New-Disaggregation-Dimension

Conversation

@brianbrix

@brianbrix brianbrix commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

No description provided.

brianbrix and others added 30 commits April 26, 2026 21:33
@brianbrix
Add funding columns to this old GGW codebase
30c6a72
@brianbrix
Add workflow to old codebase
5d998ec
@brianbrix
Add workflow to old codebase
6dbd505
@brianbrix
Add workflow to old codebase
01722e9
@brianbrix
Resolve failing deployment
d487737
@brianbrix
Add funding columns to the old GGW codebase DonorFundingJob
f6b5be9
@brianbrix
Add Missing global settings
d27116f
@brianbrix
Add Missing global settings
efc1653
@brianbrix
Failing xml patches
d3f6db2
@brianbrix
Failing xml patches
8ac723a
@brianbrix
Failing xml patches
6e8d5ec
@brianbrix
Failing xml patches
8489239
@brianbrix
Failing xml patches
4737503
@brianbrix
Revert donor job columns
2d506e6
@brianbrix
Revert donor job columns
eaf47c3
@brianbrix
Resolve map popup issue
7824ac6
@brianbrix
Merge remote-tracking branch 'origin/future/v4.0' into future/GGW
9517e7f 
# Conflicts:
#	amp/src/main/java/org/digijava/module/aim/helper/GlobalSettingsConstants.java
#	amp/src/main/resources/xmlpatches/4.0/AMP-31057-Add-Currecies-for-dashboard.xml
#	amp/src/main/resources/xmlpatches/4.0/AMP-31057-Donor-Funding-Admin-Levels-v1.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_0.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_1.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_2.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_3.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_4.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_5.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_6.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_7.xml
#	amp/src/main/resources/xmlpatches/general/views/v_nationalobjectives_level_8.xml
#	amp/src/main/resources/xmlpatches/general/views/v_pledges_tertiary_programs_level_2.xml
#	amp/src/main/resources/xmlpatches/general/views/v_pledges_tertiary_programs_level_3.xml
#	amp/src/main/webapp/WEB-INF/jsp/esrigis/view/mainmapPopup.jsp
@brianbrix
Resolve map popup issue
ccda24b
@brianbrix
Report engine failing
d4c708f
@brianbrix
Report engine failing
0ed93d1 
Missing gazeteer
@brianbrix
Internal error when saving activity
fd2781c
@brianbrix
Internal error when saving activity
d7e3293
@brianbrix
Internal error when saving activity
20a1394
@brianbrix
Project list folded on load
04fbc87
@brianbrix
AMP-31125: Issue viewing/editing some activities from tab
2011c00
@brianbrix
AMP-31125: Issue viewing/editing some activities from tab
aaf11ce
@brianbrix
AMP-31125: Issue some activities missing from GIS
3c6957d
@brianbrix
AMP-31125: Issue some activities missing from GIS
045d5f0
@jdeanquin-dg
Merge pull request #4475 from devgateway/fix/GGW/GGW-issues
37c81be 
fix/GGW/GGW-issues
@brianbrix
Merge branch 'develop' into future/v4.0
4ae5b6a 
# Conflicts:
#	.github/workflows/deploy.yml
#	amp/pom.xml
#	amp/src/main/java/org/dgfoundation/amp/onepager/util/ActivityUtil.java
#	amp/src/main/java/org/digijava/module/message/jobs/AmpDonorFundingJob.java
#	amp/src/main/resources/org/digijava/module/aim/dbentity/AmpStructure.hbm.xml
#	amp/src/main/webapp/WEB-INF/jsp/esrigis/view/mainmapPopup.jsp
@brianbrix
Refactor image handling in EditActivity.java
a11385f 
Replace image clearing and adding with a direct HashSet assignment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings October 29, 2026 07:41
brianbrix and others added 3 commits October 29, 2026 10:42
@brianbrix
Change INSERT statement to use nextval for sequence
ef7c661 
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@brianbrix
AMP-31226: Add third disaggregation dimension
584b466
@brianbrix
Merge remote-tracking branch 'origin/feature/AMP-31226/New-Disaggrega…
aa0439f 
…tion-Dimension' into feature/AMP-31226/New-Disaggregation-Dimension
Copilot AI reviewed Oct 29, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Four critical active findings remain, including incorrect activity-ID fallback behavior, a missing Level 2 database view, shallow-clone image corruption, and a sequence collision risk.

1 open issue
5 resolved since last review

Review effort: Lite

Comment thread amp/TEMPLATE/ampTemplate/tabs/js/business/grid/gridManager.js Resolved
Copilot AI lite review requested due to automatic review settings October 29, 2026 07:48
Copilot AI reviewed Oct 29, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Critical link-generation and structure-cloning defects can produce incorrect entity links and delete source images, while the missing third-category dependency check can cause protected deletions to fail with server errors.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity Check third-category references before option deletion

amp/​src/​main/​resources/​xmlpatches/​4.0/​AMP-31226-add-third-disaggregation-category.xml:12

The new third-category foreign key is not included in DisaggregationService.isOptionLinkedToIndicator, which still checks only parentCategory and childCategory. Deleting an option referenced only by third_category_id will bypass the API's protection and then fail on the FK at flush (or surface as a 500); include thirdCategory.id in that query.

1 resolved since last review

Review effort: Lite

Copilot AI lite review requested due to automatic review settings October 29, 2026 08:03
Copilot AI reviewed Oct 29, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

A critical active finding shows that AmpStructure.clone() aliases the source image set, so merging can corrupt the original structure; this must be fixed before approval.

No open issues 🎉

1 resolved since last review

Review effort: Lite

Copilot AI lite review requested due to automatic review settings October 29, 2026 17:55
Copilot AI reviewed Oct 29, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical unresolved issues prevent the indicator modals from being built and make AmpStructure cloning corrupt the source activity during merges.

2 open issues

Review effort: Lite

Comment thread ...s/reampv2-app/src/modules/admin/indicator_manager/components/modals/AddNewIndicatorModal.tsx
import axios from 'axios';
import Accordion from 'react-bootstrap/Accordion';
import initialTranslations from '../../config/initialTranslations.json';
import { buildThreeDisaggregationValues, hasEnteredDisaggregationValue } from '../../utils/disaggregation';
Comment thread ...ges/reampv2-app/src/modules/admin/indicator_manager/components/modals/EditIndicatorModal.tsx
import lodash from 'lodash';
import axios from 'axios';
import initialTranslations from '../../config/initialTranslations.json';
import { buildThreeDisaggregationValues, hasEnteredDisaggregationValue } from '../../utils/disaggregation';
Copilot AI lite review requested due to automatic review settings October 29, 2026 18:46
Copilot AI reviewed Oct 29, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

A critical shallow-clone bug causes image collections to be shared and mutated during prepareMerge, corrupting the original activity.

2 open issues

Review effort: Lite

Copilot AI lite review requested due to automatic review settings October 29, 2026 19:58
Copilot AI reviewed Oct 29, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

A critical active finding remains because AmpStructure.clone() can clear the source activity version’s shared image set, potentially losing attachments during version merge.

2 open issues

Review effort: Lite

Copilot AI lite review requested due to automatic review settings October 30, 2026 04:20
Copilot AI reviewed Oct 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

A critical finding remains in AmpStructure.clone(): its shallow-copied image set is mutated during cloning, potentially corrupting the source activity during a merge.

2 open issues

Review effort: Lite

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@brianbrix @jdeanquin-dg