add indicators screen #471

eperedo · 2024-12-02T02:56:37Z

📌 References

Issue: Closes https://app.clickup.com/t/8696ewg1c

📝 Implementation

outputs indicators are saved as dataElements within dataSet and sections
outcomes indicators are saved as indicators within dataSet and sections
outcomes indicators can include extra dataElements when the indicator (DHIS) include references to dataElements in: numerator, denominator or code.

📹 Screenshots/Screen capture

Project-Configuration-app_indicators.webm

🔥 Notes to the tester

Using PR 470 as base branch to facilitate code review

#8696ewg1c

…onfiguration into feature/add-indicators

tokland

In-line comments:

src/data/repositories/CoreCompetencyD2Repository.ts

tokland · 2024-12-06T11:36:19Z

src/data/repositories/D2ApiConfig.ts

+                        d2Response.dataElementGroupSets,
+                        metadataCodes.dataElementGroupSets.status
+                    ),
+                },


Writing one-liner is not generally a goal, but for this kind of code, the structure will look much nicer if are able to have one-liners. For that, we create function helpers that abstract the repetitions and local variables (shortening the name, only if necessary). For example, the previous three calls can be captured with something like that:

const { dataElementGroupSets } = metadataCodes; const getGroupSet = (groupSetCode: string) => getOrThrow(d2Response.dataElementGroupSets, groupSetCode);

And now is used like this:

coreCompetency: getGroupSet(dataElementGroupSets.coreCompetency),

Same for the rest of gets (name of the helper is just a proposal)

tokland · 2024-12-09T11:22:06Z

src/data/repositories/D2ApiConfig.ts

@@ -6,11 +6,27 @@ import { apiToFuture } from "$/data/api-futures";

 export const metadataCodes = {


The filename looks a bit confusing to me, as by D2Api we think of the library. Maybe D2Metadata?

src/data/repositories/DataElementD2Repository.ts

tokland · 2024-12-09T11:48:47Z

src/data/repositories/DataSetD2Repository.ts

@@ -101,7 +103,7 @@ export class DataSetD2Repository implements DataSetRepository {
        const ids = dataSets.map(dataSet => dataSet.id);

        return this.d2DataSetApi.getConfig().flatMap(config => {


save method too long, split steps

tokland · 2024-12-09T12:16:02Z

src/domain/usecases/GetCoreCompetencyUseCase.ts

+import { FutureData } from "$/domain/entities/generic/Future";
+import { CoreCompetencyRepository } from "$/domain/repositories/CoreCompetencyRepository";
+
+export class GetCoreCompetencyUseCase {


I see that at least this use case has no usage in the app. Typically we won't do that, only create it when known to be used, but of course, you know more context and you may be sure it will be used isolatedly in the UI.

tokland · 2024-12-09T12:17:51Z

src/domain/usecases/ValidateDataSetNameUseCase.ts

-                    dataSet.name.toLowerCase() === options.name.toLowerCase()
-            );
-        });
+        return this.dataSetUtils.dataSetNameExists(options);


dataSetNameExists typically we want a verb in a method.

tokland · 2024-12-09T12:20:07Z

src/webapp/components/dataset-wizard/FilterIndicators.tsx

    onClose: () => void;
-    onFilterChange: (scope: string, type: FilterType) => void;
+    onFilterChange: (scope: ChipItem | ChipItem[], type: FilterType) => void;


typically, we try to avoid this JS (T | T[]) acceptance (we accept T[] and create another function that wrap the single case)

tokland · 2024-12-09T12:21:41Z

src/webapp/components/dataset-wizard/IndicatorsDataSet.tsx

+            const isArray = Array.isArray(value);
+            switch (filterType) {
+                case "scope":
+                    if (!isArray) {


... and then we can avoid this ugly code :)

tokland · 2024-12-09T12:26:59Z

src/webapp/pages/register-dataset/RegisterDataSetPage.tsx

-        return DataSet.initial(getUid(new Date().getTime().toString()));
-    });
+    const [dataSet, updateDataSet] = React.useState<DataSet>(
+        DataSet.initial(getUid(new Date().getTime().toString()))


abstract to get RandomUid or similar

tokland

Some in-line comments:

tokland · 2024-12-16T09:17:24Z

src/domain/entities/DataSet.ts

-    validate(): Either<ValidationError<DataSet>[], DataSet> {
-        const allErrors = this.getValidationErrors();
-        return allErrors.length === 0 ? Either.success(this) : Either.error(allErrors);
+    validate(): ValidationError<DataSet>[] {


minor, no need to change, just a general note: Using ValidationError<DataSet>[] constrains the return type to always be an array. For a more flexible approach, consider ValidationErrors<DataSet> instead, allowing it to represent any structure. This way, the type can be reshaped in the future without requiring changes to all signatures where it is passed, focusing only on the code that interacts with its internals.

tokland · 2024-12-16T09:18:34Z

src/domain/entities/Indicator.ts

@@ -12,7 +12,7 @@ export type IndicatorAttrs = {
    theme: string;
    status: string;
    type: "outputs" | "outcomes";
-    scope: "core" | "local" | "donor";
+    scope: ScopeType;


To keep the naming consistent: Scope or IndicatorScope

tokland · 2024-12-16T09:19:44Z

src/utils/uid.ts

@@ -45,6 +45,9 @@ export function generateUid(): string {
    return randomChars;
 }

+// @ts-ignore
+window.generateUid = generateUid;
+


Do we need this?

tokland · 2024-12-16T09:24:05Z

src/webapp/components/dataset-wizard/FilterIndicators.tsx

@@ -135,7 +138,7 @@ export const FilterIndicators = React.memo((props: FilterIndicatorsProps) => {
 export type ChipFilterProps = {
    items: ChipItem[];
    label: string;
-    onChange: (item: ChipItem | ChipItem[]) => void;
+    onChange: (item: ChipItem[]) => void;
    value: string | string[];


sorry, I hadn't seen that. Can we follow the same idea for value, to be only string[]?

The general idea is that, when using TS code in our control, we never have to call functions like isArray, isObject, isString, etc.

(When we really need to have this kind of distinctions, in other scenarios, we use type-safe discriminating union types)

tokland · 2024-12-16T09:26:59Z

src/data/repositories/D2ApiMetadata.ts

-                    coreCompetency: getOrThrow(
-                        d2Response.dataElementGroupSets,
+                    coreCompetency: getOrThrowMetadata(
+                        "dataElementGroupSets",
                        metadataCodes.dataElementGroupSets.coreCompetency


We were not able to get single lines, but yes, it may hinder readability if we push it too much. let's keep as is.

(other strategies: move to separate function to reduce indentation level, create shorter name for helper functions, rename to shorter names the variables in the block)

tokland

All good by me, code-wise

add indicators screen

6e708eb

eperedo requested review from tokland and MiquelAdell December 2, 2024 02:56

MiquelAdell approved these changes Dec 2, 2024

View reviewed changes

Merge branch 'feature/setup-screen' of github.com:EyeSeeTea/dataset-c…

8530908

…onfiguration into feature/add-indicators

eperedo mentioned this pull request Dec 6, 2024

add sharing step #472

Merged

2 tasks

Base automatically changed from feature/setup-screen to development-project-configurator December 9, 2024 08:53

tokland reviewed Dec 9, 2024

View reviewed changes

refactor and delete unused files

b297bbb

eperedo requested a review from tokland December 10, 2024 15:35

tokland reviewed Dec 16, 2024

View reviewed changes

refactor filter indicator, rename scope type

683b36c

eperedo requested a review from tokland December 16, 2024 17:28

tokland approved these changes Dec 17, 2024

View reviewed changes

MiquelAdell merged commit 6fe29f3 into development-project-configurator Dec 17, 2024
1 check passed

MiquelAdell deleted the feature/add-indicators branch December 17, 2024 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add indicators screen #471

add indicators screen #471

eperedo commented Dec 2, 2024

tokland left a comment

tokland Dec 6, 2024

tokland Dec 9, 2024

tokland Dec 9, 2024

tokland Dec 9, 2024

tokland Dec 9, 2024

tokland Dec 9, 2024

tokland Dec 9, 2024

tokland Dec 9, 2024

tokland left a comment

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading

tokland left a comment

		@@ -6,11 +6,27 @@ import { apiToFuture } from "$/data/api-futures";

		export const metadataCodes = {

		@@ -101,7 +103,7 @@ export class DataSetD2Repository implements DataSetRepository {
		const ids = dataSets.map(dataSet => dataSet.id);

		return this.d2DataSetApi.getConfig().flatMap(config => {

add indicators screen #471

add indicators screen #471

Conversation

eperedo commented Dec 2, 2024

📌 References

📝 Implementation

📹 Screenshots/Screen capture

🔥 Notes to the tester

tokland left a 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

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 Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokland Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

tokland Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

tokland Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

tokland left a comment

Choose a reason for hiding this comment

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading