Conversation
brianbrix
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive enhancements to the AMP Indicators module, adding support for new indicator fields, outcome/output management, and improved UI. The changes introduce additional categorization capabilities and a management interface for outcomes and outputs.
Key changes include:
- Addition of new indicator fields (outcome, output, type, disaggregation, unit of measure, etc.)
- Implementation of outcome and output management with CRUD operations
- Enhanced UI with improved filtering, search, and visual organization
- Database schema updates with new entities and Hibernate mappings
Reviewed Changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Java entity classes | Added AmpOutcome and AmpOutput entities with bidirectional relationships |
| Hibernate mappings | New mapping files for outcome/output entities and updated indicator mappings |
| Service classes | New service for outcome/output CRUD operations and updated indicator service |
| Database patches | SQL script to create indicator category values |
| Frontend components | New React pages for outcome/output management with improved UI |
| API endpoints | New REST endpoints for outcome/output operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| public List<AmpOutputDTO> getOutputsByOutcomeId(Long outcomeId) { | ||
| Session session = PersistenceManager.getSession(); | ||
| AmpOutcome outcome = (AmpOutcome) session.get(AmpOutcome.class, outcomeId); |
There was a problem hiding this comment.
Unnecessary explicit casting. The session.get() method with a Class parameter already returns the correct type. Remove the explicit cast: AmpOutcome outcome = session.get(AmpOutcome.class, outcomeId);
| AmpOutcome outcome = (AmpOutcome) session.get(AmpOutcome.class, outcomeId); | |
| AmpOutcome outcome = session.get(AmpOutcome.class, outcomeId); |
|
|
||
| public void deleteOutcome(Long outcomeId) { | ||
| Session session = PersistenceManager.getSession(); | ||
| AmpOutcome outcome = (AmpOutcome) session.get(AmpOutcome.class, outcomeId); |
There was a problem hiding this comment.
Same unnecessary explicit casting as above. Remove the cast: AmpOutcome outcome = session.get(AmpOutcome.class, outcomeId);
| AmpOutcome outcome = (AmpOutcome) session.get(AmpOutcome.class, outcomeId); | |
| AmpOutcome outcome = session.get(AmpOutcome.class, outcomeId); |
| @@ -1,2 +1,2 @@ | |||
| export const TRN_PREFIX = 'amp.inidcatormanager.dashboard:'; | |||
| export const TRN_PREFIX_DASHBOARD = 'amp.inidcatormanager.dashboard:'; | |||
| export const TRN_PREFIX = 'amp.indicatormanager.dashboard:'; | |||
There was a problem hiding this comment.
[nitpick] Fixed typo in constant name from 'inidcator' to 'indicator', but consider if TRN_PREFIX and TRN_PREFIX_DASHBOARD should have different values since they're now identical.
| export const TRN_PREFIX = 'amp.indicatormanager.dashboard:'; | |
| export const TRN_PREFIX = 'amp.indicatormanager:'; |
| }, | ||
| csvFormatter: (_cell: any, row: any) => { | ||
| if (outcomesReducer.loading) return ''; | ||
| const foundOutcome = !outcomesReducer.loading && outcomesReducer.programs.find((outcome: any) => outcome.id === row.outcomeId); |
There was a problem hiding this comment.
Incorrect property access. Should be outcomesReducer.outcomes instead of outcomesReducer.programs to find outcomes correctly.
| const foundOutcome = !outcomesReducer.loading && outcomesReducer.programs.find((outcome: any) => outcome.id === row.outcomeId); | |
| const foundOutcome = !outcomesReducer.loading && outcomesReducer.outcomes.find((outcome: any) => outcome.id === row.outcomeId); |
| calculationMethod: values.calculationMethod, | ||
| responsibleOrganizations: values.responsibleOrganizations, | ||
| frequency: values.frequency, | ||
| programId: values.programId ? values.programId: null, |
There was a problem hiding this comment.
[nitpick] Missing space after colon. Should be values.programId ? values.programId : null for consistent formatting.
| programId: values.programId ? values.programId: null, | |
| programId: values.programId ? values.programId : null, |
| .collect(Collectors.toSet()); | ||
| indicator.setResponsibleOrganizations(orgs); | ||
| } else { | ||
| indicator.setResponsibleOrganizations(new java.util.HashSet<>()); |
There was a problem hiding this comment.
[nitpick] Use the already imported HashSet class instead of the fully qualified name: new HashSet<>()
| indicator.setResponsibleOrganizations(new java.util.HashSet<>()); | |
| indicator.setResponsibleOrganizations(new HashSet<>()); |
| .collect(Collectors.toSet()); | ||
| indicator.setResponsibleOrganizations(orgs); | ||
| } else { | ||
| indicator.setResponsibleOrganizations(new java.util.HashSet<>()); |
There was a problem hiding this comment.
[nitpick] Same issue as above - use the imported HashSet class: new HashSet<>()
| indicator.setResponsibleOrganizations(new java.util.HashSet<>()); | |
| indicator.setResponsibleOrganizations(new HashSet<>()); |
AMP-31027: List/View Existing Outcomes and Outputs AMP-31028:Add New Outcome AMP-31029:Add New Output
AMP-31044: break down target and original value by dissagregation
jdeanquin-dg
force-pushed
the
task/AMP-31022/Indicators-module-changes
branch
from
a6db9b2 to
8c3fb35
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…t similar logic for NPO/Theme/Program x indicator.
.../main/java/org/dgfoundation/amp/nireports/amp/indicators/IndicatorTextualTokenBehaviour.java
Show resolved
Hide resolved
2 participants
No description provided.