feat: add groups for assets #306

sebtiz13 · 2023-06-01T08:35:53Z

Depend to #307

What does this PR do ?

This PR implement groups for assets.
The groups can have sub groups and groups can have many assets, but assets can only belong to one group.

How should this be manually tested?

// TODO

Step 1 :
Step 2 :
Step 3 :

Boyscout

Add @types/lodash in devDependencies to get autocomplete and correct type on lodash functions

Aschen

😘

lib/modules/asset/AssetModule.ts

lib/modules/asset/AssetsGroupsController.ts

lib/modules/asset/types/AssetGroupContent.ts

lib/modules/plugin/DeviceManagerEngine.ts

Aschen · 2023-06-12T22:01:38Z

tests/fixtures/assetsGroups.ts

@@ -0,0 +1,34 @@
+import { AssetsGroupsBody } from "../../lib/modules/asset/types/AssetGroupContent";
+
+export const assetGroupTestId = "test-group";


It's really nitpicking but IMHO the example would be more explicit with "real" hierarchic groups like Occitanie, Montpellier, Ecusson, Rue des écoles Pies

tests/scenario/modules/assets/asset-group.test.ts

lib/modules/asset/AssetsGroupsController.ts

lib/modules/asset/types/AssetGroupsAPI.ts

lib/modules/asset/types/AssetGroupContent.ts

lib/modules/plugin/DeviceManagerEngine.ts

tdislay · 2023-06-15T08:32:39Z

It's hard to review force pushed code. It means re-reading everything, and not just the newly added code

sebtiz13 · 2023-06-15T17:28:55Z

It's hard to review force pushed code. It means re-reading everything, and not just the newly added code

Ok it's noted, now I rebase only when the review is finished

Aschen · 2023-06-15T21:12:27Z

Also the documentation is missing, here is some ideas of things to do:

document the controller's actions in https://doc.kuzzle.io/official-plugins/device-manager/2
add the controller to https://doc.kuzzle.io/iot-backend/3/concepts/architecture/#device-manager
tell about the existence of groups here https://doc.kuzzle.io/official-plugins/device-manager/2/concepts/digital-twins-devices-assets/#assets and here https://doc.kuzzle.io/iot-backend/3/getting-started/asset-model/

tests/scenario/modules/assets/asset-group.test.ts

lib/modules/asset/AssetsGroupsController.ts

tdislay · 2023-06-29T08:24:46Z

lib/modules/asset/AssetsGroupsController.ts

+        query: {
+          bool: {
+            must: [
+              {
+                regexp: {
+                  name: {
+                    case_insensitive: true,
+                    value: body.name,
+                  },
+                },
+              },
+            ],
+            must_not: [
+              {
+                terms: {
+                  _id: typeof assetId === "string" ? [assetId] : [],
+                },
+              },
+            ],
+          },
+        },


Why not using koncorde syntax ?

I didn't manage to use regexp in case-insensitive mode in Koncorde, the flag i like it's explain in documentation throw an error and like I haven't the time to search more at the moment I have preferred to use Elasticsearch syntax that works correctly.

But it's true, maybe we need to refactor that later

I dug a little bit and found, that atm, regexp is not usable in search queries. Because it's not translated from Koncorde DSL to ES DSL (Idk why).

So if we don't implement it (because there are maybe good reasons 🤷), you can either add a comment to explain why we use a ES query, or I also suggest to use the regexp syntax from ES (yes, therefore you can mix koncorde & es lol):

query: { and: [ { regexp: { name: { case_insensitive: true, value: body.name, }, }, }, { not: { "equals": { _id: typeof assetId === "string" ? [assetId] : [] } } } ], },

Koncorde regex and ES regex does not have the same syntax that's why

Koncorde regex and ES regex does not have the same syntax that's why. We could allow the keyword though but it would be confusing since the filter will have ES syntax in Koncorde DSL

I don't understand. Actually the keyword is allowed when searching for documents, via Koncorde.
The query above is working (although it may return an error 🤷)

Koncorde requests are supposed to be used indifferently in realtime filters and in search queries but if you use a regex then it cannot works in both because the syntax is difference

lib/modules/asset/AssetsGroupsController.ts

tests/scenario/modules/assets/asset-group.test.ts

…sing

* feat(assets-groups): implement CRUD actions * feat(assets-groups): implement add assets to group * feat: implement remove assets to group * feat: export group api types * feat(createGroup): id is not mandatory anymore, auto generated if missing * feat(groupName): add check of name unicity * feat(groupAsset): handle group hierarchy * feat(groupName): check presence of name when create

* feat(assetGroups): implement CRUD actions * feat(assetGroups): implement add assets to group * feat(assetGroups): implement remove assets to group * chore(types): export group api types * feat(createGroup): id is not mandatory anymore, auto generated if missing * feat(groupName): add check of name unicity * feat(groupAsset): handle group hierarchy * feat(groupName): check presence of name when create

# [2.3.0](v2.2.8...v2.3.0) (2023-08-14) ### Bug Fixes * composite measures should be correctly exported to CSV ([#309](#309)) ([487c1e8](487c1e8)) * **docs:** wrong arguments in models' getDevice request ([#310](#310)) ([028c65c](028c65c)) ### Features * **assetGroups:** add assetGroups roles ([b9d0fae](b9d0fae)) * **assetGroups:** add groups for assets ([#306](#306)) ([10de8b4](10de8b4)) * **assetGroups:** add lastUpdate on changes ([#311](#311)) ([36a4575](36a4575)) * **semantic-release:** add semantic release support to device manager ([99b1683](99b1683))

sebtiz13 added the changelog:new-features label Jun 1, 2023

sebtiz13 requested review from fmauNeko, alexandrebouthinon, rolljee and Shiranuit June 1, 2023 08:35

sebtiz13 self-assigned this Jun 1, 2023

sebtiz13 force-pushed the feat/assets-groups branch 8 times, most recently from e943ed6 to 1126661 Compare June 2, 2023 15:19

sebtiz13 marked this pull request as ready for review June 12, 2023 10:11

Aschen reviewed Jun 12, 2023

View reviewed changes

tdislay reviewed Jun 13, 2023

View reviewed changes

tests/scenario/modules/assets/asset-group.test.ts Outdated Show resolved Hide resolved

tests/scenario/modules/assets/asset-group.test.ts Outdated Show resolved Hide resolved

lib/modules/asset/AssetsGroupsController.ts Outdated Show resolved Hide resolved

OlivierCavadenti suggested changes Jun 13, 2023

View reviewed changes

lib/modules/asset/AssetsGroupsController.ts Show resolved Hide resolved

lib/modules/asset/types/AssetGroupsAPI.ts Outdated Show resolved Hide resolved

lib/modules/asset/types/AssetGroupContent.ts Show resolved Hide resolved

lib/modules/plugin/DeviceManagerEngine.ts Show resolved Hide resolved

sebtiz13 force-pushed the feat/assets-groups branch 6 times, most recently from 19c03d9 to 5c4d928 Compare June 14, 2023 21:32

sebtiz13 requested review from OlivierCavadenti and tdislay June 15, 2023 17:29

tdislay approved these changes Jun 16, 2023

View reviewed changes

tests/scenario/modules/assets/asset-group.test.ts Show resolved Hide resolved

tests/scenario/modules/assets/asset-group.test.ts Outdated Show resolved Hide resolved

sebtiz13 force-pushed the feat/assets-groups branch from 0149810 to 5861164 Compare June 22, 2023 11:49

tdislay force-pushed the feat/assets-groups branch from f73f246 to 411df3d Compare June 22, 2023 15:48

tdislay suggested changes Jun 29, 2023

View reviewed changes

rolljee approved these changes Jul 5, 2023

View reviewed changes

sebtiz13 and others added 14 commits July 11, 2023 11:53

feat: implement dummy methods

90c4f7c

feat(assets-groups): implement CRUD actions

a5895c3

feat(assets-groups): implement add assets to group

4fdced6

refactor: check children with mExists instead loop

47934a4

feat: implement remove assets to group

12b896d

fix: tests use toMatchObject instead toStrictEqual

fb92170

fix: export group api types

7757b66

feat(createGroup): id is not mandatory anymore, auto generated if mis…

d6e1930

…sing

feat(deleteGroup): remove link for deleted group

eeda79b

fix(searchGroup): correct search test

0f172a7

feat(groupName): add check of name unicity

18f5a25

feat(groupAsset): handle group hierarchy

846a1f3

fix(groupName): correct check of name unicity

f307bea

refactor: correct check function parameters

448e8d7

sebtiz13 force-pushed the feat/assets-groups branch from 321fecf to 448e8d7 Compare July 11, 2023 09:54

sebtiz13 added 3 commits July 12, 2023 17:36

fix: correct link in parent when create children group

1cca25e

refactor: resolve comments

bf0ecbc

feat(groupName): check presence of name when create

18499af

tdislay approved these changes Jul 17, 2023

View reviewed changes

sebtiz13 merged commit c49e5c7 into 2-dev Jul 17, 2023
3 checks passed

sebtiz13 deleted the feat/assets-groups branch July 17, 2023 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add groups for assets #306

feat: add groups for assets #306

sebtiz13 commented Jun 1, 2023 •

edited

Aschen left a comment

Aschen Jun 12, 2023

tdislay commented Jun 15, 2023 •

edited

sebtiz13 commented Jun 15, 2023

Aschen commented Jun 15, 2023

tdislay Jun 29, 2023

sebtiz13 Jun 29, 2023

tdislay Jun 30, 2023

Aschen Jul 5, 2023

Aschen Jul 5, 2023

tdislay Jul 5, 2023 •

edited

Aschen Jul 13, 2023

		@@ -0,0 +1,34 @@
		import { AssetsGroupsBody } from "../../lib/modules/asset/types/AssetGroupContent";

		export const assetGroupTestId = "test-group";

feat: add groups for assets #306

feat: add groups for assets #306

Conversation

sebtiz13 commented Jun 1, 2023 • edited

What does this PR do ?

How should this be manually tested?

Boyscout

Aschen left a comment

Choose a reason for hiding this comment

Aschen Jun 12, 2023

Choose a reason for hiding this comment

tdislay commented Jun 15, 2023 • edited

sebtiz13 commented Jun 15, 2023

Aschen commented Jun 15, 2023

tdislay Jun 29, 2023

Choose a reason for hiding this comment

sebtiz13 Jun 29, 2023

Choose a reason for hiding this comment

tdislay Jun 30, 2023

Choose a reason for hiding this comment

Aschen Jul 5, 2023

Choose a reason for hiding this comment

Aschen Jul 5, 2023

Choose a reason for hiding this comment

tdislay Jul 5, 2023 • edited

Choose a reason for hiding this comment

Aschen Jul 13, 2023

Choose a reason for hiding this comment

sebtiz13 commented Jun 1, 2023 •

edited

tdislay commented Jun 15, 2023 •

edited

tdislay Jul 5, 2023 •

edited