Associate projects to data sets #469

eperedo · 2024-10-17T01:58:00Z

📌 References

Issue: Closes https://app.clickup.com/t/86960mp8j

📝 Implementation

Add tabs to show list for dataSets and projects
Script to migrate project information from category options to custom attribute (metadata attached in issue)

📹 Screenshots/Screen capture

Project-Configuration-app_tabs_datasets_projects.webm

🔥 Notes to the tester

Run script:

yarn run migrate-ds-project -u 'http://localhost:8080' --username 'username' --password 'mypassword'

#86960mp8j

…ion into feat/skeleton-app

tokland

I've tried to review only new things, in the future I should be more careful in the order of review so we don't have PRs with common code, which makes review harder.

tokland · 2024-10-28T12:04:34Z

package.json

@@ -28,6 +28,8 @@
        "d2-manifest": "1.0.0",
        "eslint-plugin-no-relative-import-paths": "^1.5.3",
        "font-awesome": "4.7.0",
+        "lodash.keyby": "4.6.0",
+        "lodash.mapvalues": "4.6.0",


est-lodash cannot do that?

tokland · 2024-10-28T12:08:36Z

src/domain/repositories/MetadataRepository.ts

@@ -0,0 +1,6 @@
+import { MetadataItem } from "$/domain/entities/MetadataItem";
+import { FutureData } from "$/data/api-futures";


Move FutureData to the domain, we cannot reference the data layer from here.

tokland · 2024-10-28T12:09:54Z

src/domain/entities/MetadataItem.ts

+
+export type NamedCodeRef = NamedRef & { code: string };
+
+export type MetadataItem = {


That names is too generic, from the name we cannot imagine what it contains

tokland · 2024-10-28T12:11:09Z

src/domain/entities/MetadataItem.ts

+export type NamedCodeRef = NamedRef & { code: string };
+
+export type MetadataItem = {
+    attributes: { project: NamedCodeRef; createdByApp: NamedCodeRef };


I'd say "attributes" is a implementation detail, so it should appear referenced in the domain layer.

tokland · 2024-10-28T12:17:08Z

src/data/repositories/ProjectD2Repository.ts

+        this.d2DataSetApi = new DataSetD2Api(this.api, metadata);
+    }
+
+    getAll(): FutureData<Project[]> {


As a general rule, be careful with getAll methods that return an array, it's unbounded. There are solutions with Generators, but it's complex. Not saying it has to be changed here, but so we are aware of this.

tokland · 2024-10-28T12:18:07Z

src/domain/usecases/GetProjectsUseCase.ts

+
+export class GetProjectsUseCase {
+    constructor(private projectRepository: ProjectRepository) {}
+    execute(options: GetDataSetOptions): FutureData<Paginated<Project>> {


tokland

Very close, just some details:

src/data/repositories/D2ApiCategoryOption.ts

tokland · 2024-11-04T12:41:08Z

src/data/repositories/D2ApiConfig.ts

+            );
+
+            return {
+                attributes: { ...this.buildAttributes(attributes) },


No need to pack/unpack

tokland · 2024-11-04T12:42:49Z

src/data/repositories/D2ApiConfig.ts

+                (d2Attribute): D2NamedCodeRef => {
+                    return { id: d2Attribute.id, name: d2Attribute.name, code: d2Attribute.code };
+                }
+            );


The 3 blocks are not changing anything, right?

tokland · 2024-11-04T12:43:56Z

src/data/repositories/D2ApiConfig.ts

+        };
+    }
+
+    private getOrThrow(modelData: D2NamedCodeRef[], code: string): D2NamedCodeRef {


[subjective] Generic functions that do not use anything from the instance can go to the root level.

tokland · 2024-11-04T12:44:31Z

src/data/repositories/DataSetD2Repository.ts

+            });
+
+            return $requests.map(d2Response => {
+                const allDataSets = d2Response.flatMap(d2Response => d2Response.objects);


tokland · 2024-11-04T13:16:30Z

src/data/repositories/DataSetD2Api.ts

+                                  },
+                              }
+                            : {}),
+                    },


Conditional fields will bring problems. If you check the response type, organisationUnits won't be nice to work with. It can be done with generics, but typically is better to keep it simple.

As a rule of thumb: If we need a method that does not get orgunits, then we have a separate method and a separate dataSet type (without orgUnits). Setting empty fields is a code smell.

tokland · 2024-11-04T13:17:15Z

src/data/repositories/DataSetD2Api.ts

    ): DataSet {
        const projectAttributeId = d2DataSet.attributeValues.find(
-            attribute => attribute.attribute.id === this.metadata.attributes.project.id
+            attribute => attribute.attribute.id === attributes.project.id
        )?.value;
        const projectDetails = projects.find(project => project.id === projectAttributeId);
        const dataElementGroups = this.buildDataElementsGroupsCodes(d2DataSet);
        return DataSet.create({


I'd add a blank before the return to let the code breath

tokland · 2024-11-04T13:19:28Z

src/data/repositories/DataSetD2Api.ts

+    generateFullPermission(permissions: Permissions): OctalNotationPermission {
+        return `${this.convertPermissionToOctal(
+            permissions.metadata
+        )}${this.convertPermissionToOctal(permissions.data)}----`;


I think this will be more clear: [a,b,c].join("")

tokland · 2024-11-04T13:21:21Z

src/webapp/routes.ts

+        [history]
+    );
+
+    return { navigateTo };


Not important, but we plan to add more methods here? if we are sure we are not, then return the plain object.

tokland · 2024-11-04T13:33:16Z

src/webapp/routes.ts

+            history.push(path);
+        },
+        [history]
+    );


Nice, very close. There is only an edge case, when you try to use it like this: navigateTo("editDataSets"), right? TS won't warn about the missing params. In this scenario, the solution is using ...args somehow. I think that covers all cases (confirm):

const navigateTo = React.useCallback( <Key extends RouteKey>(route: Key, ...args: Parameters<(typeof routes)[Key]>) => { const path = (routes[route] as any)(...args); history.push(path); }, [history] );

MiquelAdell

tested against gorstraining and it works
thanks!

tokland

In-line:

tokland · 2024-11-13T10:49:11Z

src/data/repositories/DataSetD2Api.ts

+                    "attributeValues.attribute.id": { in: attributesToFilter },
+                    "attributeValues.value": options.filters.projectsIds
+                        ? { in: [...this.getOrThrow(options.filters.projectsIds)] }
+                        : { eq: "true" },


This looks confusing to me. We have a filter id/value (that's a problem with DHIS2, as we already saw), and the filter is used to both filter by projectIds and something boolean (I guess, createdByApp). Not sure the best approach, but I think this needs some refactoring, first to be more declarative (join the key/value that you want to filter, in the options), and second how to manage the DHIS2 part.

tokland · 2024-11-13T10:49:50Z

src/data/repositories/DataSetD2Api.ts

+                    identifiable: { token: options.filters.search },
+                    id: { in: options.filters.ids },
+                },
+                fields: {


a lot of repeated code with the previous method, we can abstract it (if it's conceptually the same)

tokland · 2024-11-13T10:53:33Z

src/data/repositories/DataSetD2Repository.ts

            paging: { page, pageSize },
            filters: {},
            sorting: { field: "name", order: "asc" },
-            includeOrgUnits: true,


type D2DataSetOptions = GetDataSetOptions & { includeOrgUnits?: boolean };

is includeOrgUnits used, after the refactor?

eperedo added 8 commits October 1, 2024 13:47

add dataset list and actions

33479cc

Merge branch 'development' of github.com:EyeSeeTea/dataset-configurat…

9451c77

…ion into feat/skeleton-app

fix eslint problems

10e6a57

update i18n

4ea550c

add create/edit page and setup dataset step

c5abf2c

add steps for indicators, share and summary

ef3cbea

add back button and update i18n

4449929

associate projects to data sets

fea1bed

eperedo requested review from tokland and MiquelAdell October 17, 2024 01:58

tokland mentioned this pull request Oct 22, 2024

Create a list of data sets #465

Merged

2 tasks

tokland reviewed Oct 28, 2024

View reviewed changes

eperedo added 2 commits October 30, 2024 20:56

PR observations

2d89ed5

update i18n

0ca0c93

eperedo requested a review from tokland November 4, 2024 02:11

tokland reviewed Nov 4, 2024

View reviewed changes

MiquelAdell approved these changes Nov 4, 2024

View reviewed changes

PR observations

6b168dd

eperedo requested a review from tokland November 13, 2024 00:13

eperedo mentioned this pull request Nov 13, 2024

Add wizard UI #468

Merged

2 tasks

tokland reviewed Nov 13, 2024

View reviewed changes

eperedo added 4 commits November 13, 2024 20:57

refactor build datasets from api response

a2f6cfe

add getList method to avoid getting all dataSet information

b9e40d8

update i18n

6d0ab49

update tests

fb2c170

eperedo requested a review from tokland November 14, 2024 17:20

MiquelAdell merged commit dc05678 into development-project-configurator Nov 18, 2024
1 check passed

MiquelAdell deleted the feature/associate-projects-datasets branch November 18, 2024 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Associate projects to data sets #469

Associate projects to data sets #469

eperedo commented Oct 17, 2024

tokland left a comment

tokland Oct 28, 2024

tokland Oct 28, 2024

tokland Oct 28, 2024

tokland Oct 28, 2024

tokland Oct 28, 2024

tokland Oct 28, 2024

tokland left a comment

tokland Nov 4, 2024

tokland Nov 4, 2024 •

edited

Loading

tokland Nov 4, 2024

tokland Nov 4, 2024

tokland Nov 4, 2024 •

edited

Loading

tokland Nov 4, 2024 •

edited

Loading

tokland Nov 4, 2024

tokland Nov 4, 2024

tokland Nov 4, 2024 •

edited

Loading

MiquelAdell left a comment

tokland left a comment

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

		@@ -0,0 +1,6 @@
		import { MetadataItem } from "$/domain/entities/MetadataItem";
		import { FutureData } from "$/data/api-futures";


		export type NamedCodeRef = NamedRef & { code: string };

		export type MetadataItem = {

Associate projects to data sets #469

Associate projects to data sets #469

Conversation

eperedo commented Oct 17, 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

tokland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokland Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokland Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

tokland Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokland Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

MiquelAdell left a 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 4, 2024 •

edited

Loading

tokland Nov 4, 2024 •

edited

Loading

tokland Nov 4, 2024 •

edited

Loading

tokland Nov 4, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading

tokland Nov 13, 2024 •

edited

Loading