add sharing step #472

eperedo · 2024-12-05T19:11:20Z

📌 References

Issue: Closes https://app.clickup.com/t/8696ewgjx https://app.clickup.com/t/8696x5ufq

📝 Implementation

If project is selected hide the share step. Use the project code (first two digits) to get the users groups associated.
If project is not selected show the share step. By default show countries from the org. units selected in the "Setup" step.

📹 Screenshots/Screen capture

Sharing settings from project

Project-Configuration-app_create_dataset.webm

Sharing settings from org. units

Project-Configuration-app_sharing_org_units.webm

🔥 Notes to the tester

This pull request depends on the "indicators step PR"

#8696ewgjx
#8696x5ufq

…-configuration into feature/share-step

Align the search input with the data table component

…-configuration into feature/share-step

…figuration into feature/share-step

MiquelAdell

functionally tested and it works. thanks!

tokland

In-line comments:

tokland · 2024-12-16T09:41:00Z

src/data/repositories/ConfigD2Repository.ts

+                paging: false,
+            })
+        ).map(d2Response => {
+            return d2Response.objects.map(region => ({


I guess we want "userGroup" here instead of "region".

tokland · 2024-12-16T09:42:42Z

src/data/repositories/ConfigD2Repository.ts

+    }
+
+    private extractRegionCode(code: string): string {
+        return (code.slice(0, 2) || "").toUpperCase();


Add comment with an example, so the reader knows why are we processing it.

tokland · 2024-12-16T09:42:49Z

src/data/repositories/ConfigD2Repository.ts

+    }
+
+    private extractCode(code: string): string {
+        return (code.split("_")[0] || "").toUpperCase();


tokland · 2024-12-16T09:45:57Z

src/data/repositories/DataSetD2Repository.ts

-                .compactMap(access => {
-                    if (access.type !== "groups") return undefined;
+                .filter(access => access.type === "groups")
+                .map(access => {


declarative (nit-picking): after the filter, it's not just an "access", it's a "groupAccess"

tokland · 2024-12-16T09:47:01Z

src/data/repositories/DataSetD2Repository.ts

@@ -326,6 +330,20 @@ export class DataSetD2Repository implements DataSetRepository {
        };
    }

+    private convertSharingGroupsToAccessData(d2UserGroups: Maybe<SharingUserGroup>): AccessData[] {


Is this method used?

tokland · 2024-12-16T09:56:25Z

src/domain/entities/DataSet.ts

+    }
+
+    private setAccessPermissionGroup(userGroup: UserGroup): AccessData {
+        const isAdmin = userGroup.name.split("_")[1]?.toLowerCase() === "administrators";


Are the same?

$ grep -ir 'name.split("_")' src/ src/webapp/components/dataset-wizard/SummaryDataSet.tsx: return access.name.split("_")[0]; src/domain/entities/DataSet.ts: .compactMap(access => access.name.split("_")[0]) src/domain/entities/DataSet.ts: const isAdmin = userGroup.name.split("_")[1]?.toLowerCase() === "administrators";

tokland · 2024-12-20T12:04:24Z

src/domain/entities/DataSet.ts

        const name = project ? `${project.name} DataSet` : "";
-        return this._update({ project, name });
+        const orgUnits = project ? project.orgsUnits : this.orgUnits;


typo: orgsUnits -> orgUnits

tokland · 2024-12-20T12:08:20Z

src/domain/entities/Permission.ts

@@ -1,3 +1,4 @@
+import { Permissions } from "$/domain/entities/DataSet";


does it make sense to move Permissions to this entity?

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

src/domain/entities/Permission.ts

@@ -10,4 +11,14 @@ export class Permission extends Struct<PermissionAttrs>() {
    static buildWithOutAccess(): Permission {
        return Permission.create({ read: false, write: false });
    }
+
+    static setDefaultPermissionsForGroups(isAdmin: boolean): Permissions {
+        const adminDataPermission = Permission.create({ read: true, write: true });


declarative: adminDataPermission -> readWritePermission (name things for what they are, not how they are used / assigned)

tokland · 2024-12-20T12:13:22Z

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

+    const regionsCodes = _(dataSet.access)
+        .filter(access => access.type === "groups")
+        .compactMap(access => {
+            return access.name.split("_")[0];


This kind of processing the view as a bit of a code smell, do we need to know this here? The solution (if any) depends on the context.

Not sure if I follow, do you mean this should be in the domain?

…rove functions names

tokland

In-line comments:

tokland · 2025-01-07T08:50:43Z

src/data/repositories/ConfigD2Repository.ts

@@ -5,6 +5,7 @@ import { Region } from "$/domain/entities/Region";
 import { Future, FutureData } from "$/domain/entities/generic/Future";
 import { ConfigRepository } from "$/domain/repositories/ConfigRepository";
 import { D2Api } from "$/types/d2-api";
+import { extractFirstTwoLetters, extractPrefix } from "$/utils/string";


Adding methods to some utils file in the root folder is only advisable for extremely generic functions. I don't think that's the case. The abstraction we are looking for is not "extractFirstTwoLetters", but some function that references the processing over the code (similar to the naming you had before).

This function is used in data and domain, right? So if this is something the domain must be aware of, let's move the function there, with another name (possible in Region and/or OrgUnit).

tokland · 2025-01-07T08:55:12Z

src/utils/string.ts

+ * Output: "US"
+ *
+ */
+export function extractPrefix(value: string): string {


Both functions should be moved, they are not generic string functions, they hold domain knowledge.

tokland

[code-review] all good by me!

eperedo added 2 commits December 4, 2024 13:22

add sharing step

330badf

Merge branch 'feature/add-indicators' of github.com:EyeSeeTea/dataset…

85d8b17

…-configuration into feature/share-step

eperedo requested review from tokland and MiquelAdell December 5, 2024 19:11

eperedo and others added 5 commits December 5, 2024 14:38

Align the search input with the data table component

1a1bc02

use a custom className

f9a8c05

Merge pull request #473 from EyeSeeTea/fix/search-space

d85d170

Align the search input with the data table component

Merge branch 'feature/add-indicators' of github.com:EyeSeeTea/dataset…

8ee1c9c

…-configuration into feature/share-step

Merge branch 'feature/share-step' of github.com:EyeSeeTea/dataset-con…

61c7841

…figuration into feature/share-step

MiquelAdell approved these changes Dec 16, 2024

View reviewed changes

Base automatically changed from feature/add-indicators to development-project-configurator December 17, 2024 10:14

tokland reviewed Dec 20, 2024

View reviewed changes

refactor fn to get region code, move permissions to its own file, imp…

1f048b7

…rove functions names

eperedo requested a review from tokland January 6, 2025 22:41

tokland reviewed Jan 7, 2025

View reviewed changes

move extract code logic to domain

e39eb53

eperedo requested a review from tokland January 7, 2025 20:21

tokland approved these changes Jan 8, 2025

View reviewed changes

MiquelAdell merged commit d6f91fa into development-project-configurator Jan 13, 2025
1 check passed

MiquelAdell deleted the feature/share-step branch January 13, 2025 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add sharing step #472

add sharing step #472

eperedo commented Dec 5, 2024 •

edited

Loading

MiquelAdell left a comment

tokland left a comment

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024

tokland Dec 16, 2024

tokland Dec 20, 2024

tokland Dec 20, 2024

tokland Dec 20, 2024 •

edited

Loading

tokland Dec 20, 2024 •

edited

Loading

eperedo Jan 6, 2025

tokland left a comment

tokland Jan 7, 2025

tokland Jan 7, 2025

tokland left a comment

		@@ -1,3 +1,4 @@
		import { Permissions } from "$/domain/entities/DataSet";

add sharing step #472

add sharing step #472

Conversation

eperedo commented Dec 5, 2024 • edited Loading

📌 References

📝 Implementation

📹 Screenshots/Screen capture

🔥 Notes to the tester

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

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

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

Choose a reason for hiding this comment

tokland Dec 20, 2024 • edited Loading

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

Choose a reason for hiding this comment

tokland left a comment

Choose a reason for hiding this comment

eperedo commented Dec 5, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 16, 2024 •

edited

Loading

tokland Dec 20, 2024 •

edited

Loading

tokland Dec 20, 2024 •

edited

Loading