Unique Beneficiaries #551

eperedo · 2024-11-06T21:42:46Z

📌 References

Issue: Closes https://app.clickup.com/t/8695qbyrz
d2-api: Requires allow dataElement param in dataValueSets endpoint d2-api#152

📝 Implementation

I'm using the branch feature/clone-projects as the base branch to facilitate code review

🔥 Notes for the reviewer

🎨 Screenshots

New options for periods, indicators validation and unique beneficiaries report

Unique Periods administration

Unique indicators selection

Indicators Validation

Unique Beneficiaries Report

#8695qbyrz

ifoche · 2024-11-06T21:42:55Z

Task linked: CU-8695qbyrz Unique Beneficiaries

Ramon-Jimenez

Thanks @eperedo , some things I've noticed while I wait for the technical validation:

'Manage Unique Beneficiaries Periods'

You can create two periods with different labels but exactly the same Start and End month. We should not allow to create two periods with exactly same Start and End month (that includes the default ones, so we should not allow either a custom period with exactly the same months as the Semi-annual or Annual one)
You can configure a period where End month < Start month. We should not allow it as it doesn't make any sense. In fact, if you have such period configured, you cannot access the 'Project Indicators Validation' as it prompts a Conflict error.
Is there a reason why we identify the months with numbers instead of the actual name? I'd rather have the names, unless this is an specific request from the client I am not aware of.

'Project Indicators Validation'

If you try to enter a Negative value in any field of the 'Project Indicators Validation', app crashes. I understand this is not a valid use case, but we should show a pop-up preventing such config instead of the app crashing.
If you change a value in the Editable New and click on Save, it doesn't get saved. Only the Returning field and the Comments field changes get saved.
The identification and validation of updated data only seems to work for the custom periods if you have clicked on Save at least once for such period. It should work right for the beginning, no need to click on Save at least once (actually the default periods behave this way)

'Unique Beneficiaries' (the menu on top)

The period dropdown should behave slightly different. We should get all the periods configured for all the projects, but instead of showing the period labels, show a unique list of Start and End month for all the periods available (e.g., if Project A has a period called 'Q1' from Jan-Mar and Project B has a period called 'Quarter1' from Jan-Mar, we should show in the dropdown the text 'January - March (Q1, Quarter1)', so it is understood that such period is configured with two different labels.
If you have a Period selected and move to a different Country where that Period is not configured, a message says that 'No projects found for selected Period', which is true but it does not make sense to show the message it as the Period itself is not selected nor shown in the dropdown. We should just clear the Period dropdown if the selected one doesn't exist for the new country selected and not show any message.
If you select one of the Default Periods, only one project is loaded. This does not happen when selecting a Custom Period, for which all projects are loaded.
Right now for each project it is shown <project_name> and below <project_start_date> - <project_end_date>. We should add one extra line below to show the <period_label> that is matching for each project the period selected in the dropdown
Save button behavior is odd. If you change different periods, all changes are 'saved' for that session, but only the periods where you actually clicked on 'Saved' are saved from one session to the other (by session I mean getting back to the main screen and inside 'Unique Beneficiaries' again. I think by default we should save every change in the report without the need of a Save button.
Excel sheet being downloaded only shows the indicator ID, but not the name. It should be name (ID), just like it is shown in the app
When downloading data for a project that doesn't have such period configured (and shows N/A in the web), the spreadsheet shows 0 and 'No' instead of the N/A label
The spreadsheet is missing a row at the end with the sum of all beneficiaries from all projects downloaded with the label 'Country Unique Beneficiaries'

tokland

Looking very good, cool to start having some Clean Arch patterns in the project :+1

Some comments inline, mostly very minor:

tokland · 2024-11-11T09:55:19Z

i18n/en.pot

+msgstr ""
+
+msgid "January"
+msgstr ""


Typically, we'd want to use the translations from some library, but as the project only has EN, it's not really a problem. It would be different for CPR, for example, with dozens of languages to translate.

tokland · 2024-11-11T10:19:14Z

src/components/steps/data-elements/DataElementsTable.tsx

    } = props;
    const snackbar = useSnackbar();
-    const [filter, setFilter] = useState<Filter>({});
+    const [filter, setFilter] = useState<Filter>(initialFilters);


cool, now we can remove <Filter>

tokland · 2024-11-11T10:22:30Z

src/components/steps/unique-beneficiaries/UniqueIndicatorsStep.tsx

+
+const UniqueIndicatorsStep: React.FC<StepProps> = props => {
+    const { project } = props;
+    const getSelection: DataElementsStepProps["onSelect"] = React.useCallback(


Typically, we put the type to the type argument of useCallback, but maybe you prefer this style for some reason?

tokland · 2024-11-11T10:22:53Z

src/components/steps/unique-beneficiaries/UniqueIndicatorsStep.tsx

+            {...props}
+            onSelect={getSelection}
+            dataElementsSet={project.uniqueIndicators}
+            initialFilters={{ peopleOrBenefit: "people" }}


It can be moved to root const so we keep the prop immutable

tokland · 2024-11-11T10:26:18Z

src/components/unique-beneficiaries/UniqueBeneficiariesTable.tsx

+}
+
+function showAction(rows: UniqueBeneficiariesPeriodsAttrs[]): boolean {
+    return rows.filter(row => !UniqueBeneficiariesPeriod.isProtected(row)).length === 1;


Typically the Attrs/Props pair of a Struct class is used solely to create it. Then you work with the instance. A more idiomatic approach: UniqueBeneficiariesPeriod.create(row).isProtected().

tokland · 2024-11-11T11:56:08Z

src/components/unique-beneficiaries/UniqueBeneficiariesTable.tsx

+} from "../../domain/entities/UniqueBeneficiariesPeriod";
+import i18n from "../../locales";
+
+export type ActionTable = { action: string; id: string };


Is it possible to narrow down the type of action?

tokland · 2024-11-11T11:58:14Z

src/data/repositories/IndicatorReportD2Repository.ts

+    private async getProjectsByIds(projectIds: Id[]): Promise<ProjectCountry[]> {
+        const projects = await promiseMap(projectIds, id => Project.get(this.api, this.config, id));
+        return projects.map(project => ({
+            closedDate: project.endDate?.toISOString() || "",


move after openingDate?

tokland · 2024-11-11T12:00:10Z

src/domain/entities/IndicatorCalculation.ts

+    static calculateTotalValue(editable: Maybe<number>, returning: Maybe<number>): number {
+        if (!editable) return returning || 0;
+        if (!returning) return editable;
+        return editable + returning;


Probably more readable with if / else if / else

tokland · 2024-11-11T12:02:02Z

src/domain/entities/IndicatorValidation.ts

+
+        const errors: ValidationError<IndicatorValidation>[] = [periodProperty].filter(
+            validation => validation.errors.length > 0
+        );


Typically we won't specify the type when the source is already typed: return [periodProperty].filter(validation => validation.errors.length > 0)

tokland · 2024-11-11T12:02:46Z

src/pages/unique-periods/UniqueBeneficiariesPeriodsPage.tsx

+            if (options.action === "add" || options.action === "edit") {
+                setSavePeriodModal(true);
+            } else if (options.action === "delete") {
+                setDeleteModal(true);


maybe with a switch?

tokland

In-line:

tokland · 2024-11-13T10:19:02Z

src/components/steps/unique-beneficiaries/UniqueIndicatorsStep.tsx

-    const getSelection: DataElementsStepProps["onSelect"] = React.useCallback(
-        (sectorId, dataElementIds) => {
+    const getSelection = React.useCallback(
+        (sectorId: Id, dataElementIds: Id[]) => {


Sorry, I should've been more specific, I mean that pattern: React.useCallback<DataElementsStepProps["onSelect"]>, and now the arguments of the function are automatically typed.

Again, not saying this is the best style, but for consistency. The final goal is too get the most descriptive error at the most relevant point if the types do not match.

tokland · 2024-11-13T10:27:18Z

src/domain/entities/IndicatorCalculation.ts

+            return editable;
+        } else {
+            return editable + returning;
+        }


Now that I see it like this, isn't equivalent to: return (editable ?? 0) + (returning ?? 0) ?

tokland · 2024-11-13T10:31:39Z

src/domain/usecases/GetProjectsByCountryUseCase.ts

+                        item =>
+                            item.period.startDateMonth === period.startDateMonth &&
+                            item.period.endDateMonth === period.endDateMonth
+                    )


There are lots of comparisons for matching start/end. In these cases. the first thought is, can I move somehow the logic to entities? does this make sense? item.period.equalDates(period) (or whatever name)

tokland · 2024-11-13T10:34:37Z

src/domain/usecases/SaveUniqueBeneficiariesSettingsUseCase.ts

        }
+
        return this.saveSettings(settings, periodExist, options);


[subjective] when we are writing the final logic of a method (not an early return), it's very declarative to use a full "if/else if/else" block.

tokland · 2024-11-13T10:36:52Z

src/pages/country-indicator-report/CountryIndicatorReport.tsx

-        .compact()
-        .uniqBy(getId)
-        .value();
+function getAllPeriods(settings: IndicatorReport[]): UniqueBeneficiariesPeriod[] {


settings: IndicatorReport[] variable name does not match type.

tokland

Cool, all code looks good to me!

Ramon-Jimenez

Thanks @eperedo . A couple of things we still need to fix:

In the 'Unique Beneficiaries' menu, in the dropdown we are doing it right, <Start_Month> - <End_Month> and then a list of all the custom period labels that match such period within brackets. But in the project list, we are not doing it right. Each project should show - <End_Month> and ONLY in brackets its period label, so the user gets an understanding on which is the period label that is matching such criteria.
The pop-up for changed values has an 'odd' behavior. To be reviewed in a call as it is difficult to understand the rationale for current logic.

Ramon-Jimenez

All good now @eperedo , thanks!!!

eperedo added 10 commits October 18, 2024 20:05

Save+Delete periods

97b6c8a

add unique indicators step

62ba313

fixed tests

4838eca

add project indicators validation

5036f56

update i18n

b691307

add country project indicators report

120ebde

update i18n

8156728

update i18n

494e0f0

update i18n

74522dc

remove estimation file

49823aa

eperedo requested review from tokland, adrianq and Ramon-Jimenez November 6, 2024 21:42

Ramon-Jimenez requested changes Nov 8, 2024

View reviewed changes

tokland reviewed Nov 11, 2024

View reviewed changes

eperedo added 5 commits November 11, 2024 11:39

add validations for periods + pr observations

a04dd4c

update i18n

01e9151

fixed errors in project indicators validation

5a7914c

fix errors in country project and indicators

a783a89

refactor hook for confirm changes in form

80060ef

eperedo requested review from tokland and Ramon-Jimenez November 12, 2024 13:43

tokland reviewed Nov 13, 2024

View reviewed changes

improvements for periods and indicator report

f27173f

eperedo requested a review from tokland November 13, 2024 12:26

tokland approved these changes Nov 13, 2024

View reviewed changes

eperedo added 2 commits November 13, 2024 10:09

validate periods with the same months

aada02e

update i18n

3a64f2f

Ramon-Jimenez requested changes Nov 14, 2024

View reviewed changes

eperedo added 2 commits November 14, 2024 07:28

avoid generate indicators on saving periods

645520e

add period information in country report

325d051

eperedo requested a review from Ramon-Jimenez November 14, 2024 13:08

Ramon-Jimenez approved these changes Nov 14, 2024

View reviewed changes

adrianq changed the base branch from feature/clone-projects to development November 15, 2024 09:05

adrianq merged commit 896d487 into development Nov 15, 2024
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unique Beneficiaries #551

Unique Beneficiaries #551

eperedo commented Nov 6, 2024 •

edited

Loading

ifoche commented Nov 6, 2024

Ramon-Jimenez left a comment •

edited

Loading

tokland left a comment

tokland Nov 11, 2024 •

edited

Loading

tokland Nov 11, 2024

tokland Nov 11, 2024 •

edited

Loading

tokland Nov 11, 2024 •

edited

Loading

tokland Nov 11, 2024

tokland Nov 11, 2024

tokland Nov 11, 2024

tokland Nov 11, 2024

tokland Nov 11, 2024

tokland Nov 11, 2024

tokland left a comment

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024

tokland left a comment •

edited

Loading

Ramon-Jimenez left a comment •

edited

Loading

Ramon-Jimenez left a comment

Unique Beneficiaries #551

Unique Beneficiaries #551

Conversation

eperedo commented Nov 6, 2024 • edited Loading

📌 References

📝 Implementation

🔥 Notes for the reviewer

🎨 Screenshots

ifoche commented Nov 6, 2024

Ramon-Jimenez left a comment • edited Loading

Choose a reason for hiding this comment

tokland left a comment

Choose a reason for hiding this comment

tokland Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokland Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

tokland Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokland left a comment

Choose a reason for hiding this comment

tokland Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

tokland Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

tokland Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

tokland Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokland left a comment • edited Loading

Choose a reason for hiding this comment

Ramon-Jimenez left a comment • edited Loading

Choose a reason for hiding this comment

Ramon-Jimenez left a comment

Choose a reason for hiding this comment

eperedo commented Nov 6, 2024 •

edited

Loading

Ramon-Jimenez left a comment •

edited

Loading

tokland Nov 11, 2024 •

edited

Loading

tokland Nov 11, 2024 •

edited

Loading

tokland Nov 11, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland left a comment •

edited

Loading

Ramon-Jimenez left a comment •

edited

Loading