Issue/5386 embedded entity composer #5387

matborowczyk · 2023-12-13T14:32:07Z

Description

When preparing for QA session i noticed two bugs which I fixed

PR also contain E2E which were written to double check if there was no more bugs in core use-cases, which were, so also fixes for those issues
Also fix for delayed "Add in composer" button

closes #5386 #5276

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

Attached issue to pull request
Changelog entry
Code is clear and sufficiently documented
Sufficient test cases (reproduces the bug/tests the requested feature)
Correct, in line with design
End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

…86-embedded-entity-composer

src/UI/Components/Diagram/Canvas.tsx

src/UI/Components/Diagram/helpers.ts

LukasStordeur · 2023-12-19T11:25:23Z

src/UI/Components/Diagram/helpers.ts

I'm losing myself in the code in this file. There seems to be lots of repetitive pieces of code, and a whole chain of nested if cases. Either it needs a rewrite to be more readable, or add more comments to the new parts you added. Because if we come back to this later on, it will be a major headache to figure everything out again. It's about the entire for loop starting on 286.

valid point, I'll extract it into separate function

…86-embedded-entity-composer

src/UI/Components/Diagram/interfaces.ts

LukasStordeur · 2023-12-20T09:32:57Z

src/UI/Components/Diagram/init.ts

-        targetCell.addRelation(sourceCell.id as string, sourceName);
+        targetCell.addRelation(
+          sourceCell.id as string,
+          doesTargetHaveRule.attrName as string,


doesTargetHaveRule is a boolean value for me when I read the variable name. Why is it an object?

LukasStordeur · 2023-12-20T09:33:25Z

src/UI/Components/Diagram/init.ts

-        sourceCell.addRelation(targetCell.id as string, targetName);
+        sourceCell.addRelation(
+          targetCell.id as string,
+          doesSourceHaveRule.attrName as string,


same here, this is a boolean name, not inferring it might be an object.

LukasStordeur · 2023-12-20T09:33:48Z

src/UI/Components/Diagram/helpers.ts

    }
  }

  //convert relatedTo property into valid attribute
  if (instance.relatedTo) {
-    instance.relatedTo.forEach((attrName, id) => {
+    instance.relatedTo.forEach((test, testOne) => {
+      console.log(test, testOne);


console.log

LukasStordeur · 2023-12-20T09:34:21Z

src/UI/Components/Diagram/helpers.ts

@@ -388,8 +395,15 @@ export const bundleInstances = (
  const embeddedInstances = deepCopiedMapToArray.filter(
    (instance) => !topServicesNames.includes(instance.service_entity),
  );
+  return topInstances.map((instance) => {


topInstances, are those the root instances?

topInstance are non-embedded entities, as they at the top and receive eventual embedded ones

can you add that in comment please?

LukasStordeur · 2023-12-20T09:35:02Z

src/UI/Components/Diagram/helpers.ts

+    return shapesDataTransform(
+      embeddedInstances,
+      instance,
+      serviceModel as ServiceModel,


if you type serviceModel correctly in line 399, you're not supposed to cast it.

the reason why it is casted as I need to find the serviceModel from the array to pass down into the function, there it becomes serviceModel | undefined, I can add if statement and then filter out possible undefined values from array, but theoretically we should have all serviceModels in the first place, that why I chose this solution

I don't really like assuming we can trust something that would theoretically never be undefined, casting doesn't remove the eventuality your value could be undefined, it's only a type annotation telling your TS linter to ignore the warning. It's not a safe way of programming. Casting should only be done in very specific situations, and in my opinion, this ain't one.

I already find that there is way too much casting in the composer code, and I know it's mostly due to using JointJS, and maybe we should invest a bit of time to find a solution for that. Because we are discarding the purpose of having Typescript in the first place

LukasStordeur · 2023-12-20T09:50:20Z

src/UI/Components/Diagram/helpers.ts

-        );
-
-        areEmbeddedEdited = instance.action === null && data.action !== null;
+          shouldEmbeddedBeSingular =


I know performance is not much a concern, but allocating a variable like this isn't ideal. It would be more efficient to have a method to which you pass as argument the model and return whether it's singular or not. When I read the flow of code here, it makes me wonder why it has to be done between the two if cases too.

const isSingularRelation: boolean = (model: ModelType ) => { !!model.upper_limit && model.upper_limit === 1; }

I don't know by heart which name the Type is supposed to have for the model. But you get my drift. You can then simply pass the method in line 323. And also reuse the method wherever needed. If I remember well, it's a logic that appears now and then, to check if it's one or more relations.

…86-embedded-entity-composer

LukasStordeur · 2024-01-09T06:26:37Z

src/UI/Components/Diagram/helpers.ts

@@ -388,8 +395,15 @@ export const bundleInstances = (
  const embeddedInstances = deepCopiedMapToArray.filter(
    (instance) => !topServicesNames.includes(instance.service_entity),
  );
+  return topInstances.map((instance) => {


can you add that in comment please?

LukasStordeur · 2024-01-09T06:30:08Z

src/UI/Components/Diagram/helpers.ts

+    return shapesDataTransform(
+      embeddedInstances,
+      instance,
+      serviceModel as ServiceModel,


I don't really like assuming we can trust something that would theoretically never be undefined, casting doesn't remove the eventuality your value could be undefined, it's only a type annotation telling your TS linter to ignore the warning. It's not a safe way of programming. Casting should only be done in very specific situations, and in my opinion, this ain't one.

LukasStordeur · 2024-01-09T06:32:07Z

src/UI/Components/Diagram/helpers.ts

+    return shapesDataTransform(
+      embeddedInstances,
+      instance,
+      serviceModel as ServiceModel,


I already find that there is way too much casting in the composer code, and I know it's mostly due to using JointJS, and maybe we should invest a bit of time to find a solution for that. Because we are discarding the purpose of having Typescript in the first place

LukasStordeur · 2024-01-09T06:33:41Z

src/UI/Components/Diagram/init.ts

@@ -185,6 +185,7 @@ export default function diagramInit(
    const target = linkView.model.target();
    let didSourceChanged = false;
    let didTargetChanged = false;
+    let didConnectionWasSet = false;


wasConnectionSet

LukasStordeur · 2024-01-09T07:09:39Z

src/UI/Components/Diagram/init.ts

      }
    }

+    if (!didConnectionWasSet) {


You have a lot of repetition in here, which makes it hard to read. Why not extract the logic into a function? I quickly drafted it, you might need to make adjustments and add typings

const checkUpdated = (element) => { const cell = graph.getCell( element.id as dia.Cell.ID, ) as ServiceEntityBlock; const relations = cell.getRelations(); const name = cell.getName(); if (relations) { const elementConnectionRule = connectionRules[name].find((rule) => rule.name === name); if (elementConnectionRule) { cell.addRelation( sourceCell.id as string, targetConnectionRule.attributeName as string, ); updateInstancesToSend(cell, ActionEnum.UPDATE); return; // you don't need further information, so you can early return the function } } if (cell.get("isEmbedded") && cell.get("embeddedTo") !== null) { cell.set("embeddedTo", cell.id); updateInstancesToSend(cell, ActionEnum.UPDATE); return; } } const source = linkView.model.source(); const target = linkView.model.target(); checkUpdated(source); checkUpdated(target);

LukasStordeur · 2024-01-10T11:21:37Z

src/Data/Managers/GetInstanceWithRelations/QueryManager.ts

@@ -83,33 +83,42 @@ export function GetInstanceWithRelationsQueryManager(

      if (Either.isRight(instanceWithReferences)) {
        if (instanceWithReferences.value.data.referenced_by !== null) {
-          await Promise.all(
+          const resolvedNestedInstances = await Promise.all(


There are a few downsides to use promise.all, but I'm not sure it's something we need to care about.

It doesn't timeout, so if one of the operations hangs, it won't abort on it's own.

It's all or nothing, if one element fails, the entire promise will fail. It makes it hard to have individual error-handling.

It isn't sequential, but executes all the promises concurrently.

LukasStordeur · 2024-01-10T11:24:21Z

src/UI/Components/Diagram/actions.ts

Looks like this is almost the same as in the init file, I'll make the same comment, extract the repetitive code, to avoid having those boolean variables keeping track of the changes.

we check on the same things but the code in those if statement is different, so I don't think extracting here would be just for the sake of extracting

I'm not wanting to extract just for the sake of it. Functions have arguments, for minor differences like you have in this file. The element you pass is either a target or a source, but inherently, they are the same thing, an instance.

Having so many if cases like this makes the code not readable enough. Extracting methods with decent function names can improve readability, and get rid of some complexity. That's why you would extract, not just for the sake of it.

I'm really trying to follow along what's going on in this file (and the init one), but there's so much look-alike code, that I'm drowning in it. I don't even how I'd try to make changes to this code without breaking my head as to why some if cases are used. Splitting up your code in smaller pieces, that make more sense, is not a bad practice.

Anyway, let's discuss the PR tomorrow in the meeting.

LukasStordeur · 2024-01-10T11:26:04Z

src/UI/Components/Diagram/helpers.ts

+          (relation) => relation.name === attributeName,
+        );
+        if (model) {
+          if (model.upper_limit !== 1) {


Use a switch case maybe, to make it more readable?

LukasStordeur · 2024-01-10T11:43:35Z

src/UI/Components/Diagram/init.ts

-      if (doesSourceHaveRule) {
-        sourceCell.addRelation(targetCell.id as string, targetName);
-        didSourceChanged = true;
+    const checkUpdated = (sourceElement, targetElement) => {


I see it's a bit better, but still confusing, why do we need to execute in both directions in lines 230 and 231? Can you also add some docs strings to the method?

Because we can make the connections from the both sides, so we don't know which instance can get the data about being embedded/relatedTo, and previous version did that both way but there was some repetition, so avoiding repetition we need to check now from both perspectives, otherwise we have identical code but wrapped into extra function body.

I'll add docstring

What do you mean, we don't know which instance gets the data, don't you have that information 🤔 You know the two elements that are being concerned by the event, you have the target, and the source, two instances the should get an update. Now, the way you are using this, you are mixing up source and target...

I'm lost on the reasoning to be fair, maybe the Kind field in the typing would already resolve some of the complexity of all those if cases throughout the file(s).

Extracting logic doesn't mean you have to plainly copy some content into a separate method. I made the example previously, as a direction to try to explain what I meant, I didn't have all the pieces of the reasoning. Sometimes it helps to write out in bullet points what you are trying to achieve.

For me, the two instances get an event, either it's adding something, or removing something. Then I see what kind of thing I need to add or remove, and update accordingly.

This part of code checks which functions should get information about connection, as it's only stored in the children, I don't think that kind would help as theoretically both of them can be the same type and only one should get the data, so that's whats was done here.

We were checking both source and target, as depending from which instance we started it can be swapped on every attempt, that's why there was code repetition, we were checking the same thing on both instances, so as we now removed that into non-repetitive function we need to check it twice, for source and target

But aren't you already doing the check on both directions in lines 213 and further?
Honestly, I'm drowning in the amount of nested if cases all over, I can't keep track of what's happening.
Right now, the only thing that got removed were those boolean values, but the repetition and complexity is still there.

Lets discuss this tomorrow, have a call about this PR, it will be more efficient than discussing here.

LukasStordeur · 2024-01-10T11:45:02Z

src/UI/Components/Diagram/interfaces.ts

@@ -21,6 +21,7 @@ interface DictDialogData {
 interface Rule {
  name: string;
  type: TypeEnum;
+  attributeName?: string; //only used for inter-service relations


If this is required for inter-service relations, isn't it worth having a type extending the default rule set? I'm no expert in TS, so maybe I'm wrong :D But it seems like this is what has been done so far in the codebase

but the function that use that interface accept both embedded and inter-service relation, so there is no other place that use that interface so that would be dubious in that case

yes, I understand that, but it means that in the case of an inter-service relation, you have an optional where it would be a required field... Unless it's not required in the inter-service relation, then it's fine. But if it is required, then it biases your code, and enforces you to eliminate undefined cases, or cast values to bypass the linter..

Why not go for the kind:"InterserviceRelation" technique that is being used all over the rest of the app? It's just a suggestion

I see that more like if there is that attributeName then it's inter-service_relation, if not then it's embedded, and it's simple enough.

Again I'm not strongly behind or against one of those solutions so I can adjust if you feel like current solution is unclear, I will take step back to look on the code, to check whether you proposition will improve readability, which could be the case, as I have biased POV at the moment as I stare at it for quite some time :)

defining what kind of instance your dealing with, based on a field being present or not, is flakey and is not a clear solution that is maintainable. If someone new comes up in the team, and reads the code, they need explicit explanation to understand what's going on. That's something we don't want.

LukasStordeur · 2024-01-10T12:13:05Z

src/UI/Components/Diagram/actions.ts

@@ -258,7 +261,7 @@ export function appendInstance(
          if (neighborRelations) {
            const connectedInstance = Array.from(
              neighborRelations,
-              ([id, attrName]) => ({ id, attrName }),
+              ([id, attributeName]) => ({ id, attributeName }),


Isn't this the same as in line 238? if yes, maybe worth having it as a variable/function

LukasStordeur

I'll approve the PR as is now, Link it into the ticket about refactoring so we can go back to some of the comments later.

inmantaci · 2024-01-12T09:38:09Z

Processing this pull request

… array of embedded entities (Issue #5386, PR #5387) # Description When preparing for QA session i noticed two bugs which I fixed PR also contain E2E which were written to double check if there was no more bugs in core use-cases, which were, so also fixes for those issues Also fix for delayed "Add in composer" button closes #5386 #5276 # Self Check: Strike through any lines that are not applicable (`~~line~~`) then check the box - [x] Attached issue to pull request - [x] Changelog entry - [x] Code is clear and sufficiently documented - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] Correct, in line with design - [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

inmantaci · 2024-01-12T09:38:19Z

Merged into branches master, iso7 in 0c0005a

matborowczyk added 2 commits December 13, 2023 22:13

Fix connecting embedded entities

c8a931f

test fix

04ad221

matborowczyk self-assigned this Dec 13, 2023

matborowczyk added 3 commits December 14, 2023 15:39

change timeout of redirect

365d06c

relation fix, tests to improve

7cc2af8

Merge branch 'master' of github.com:inmanta/web-console into issue/53…

eef5c31

…86-embedded-entity-composer

matborowczyk marked this pull request as ready for review December 19, 2023 10:14

matborowczyk requested a review from LukasStordeur December 19, 2023 10:14

LukasStordeur reviewed Dec 19, 2023

View reviewed changes

matborowczyk added 3 commits December 19, 2023 20:45

remarks

5d96e03

Merge branch 'master' of github.com:inmanta/web-console into issue/53…

6eaed90

…86-embedded-entity-composer

remarks & fix

a8db899

LukasStordeur reviewed Dec 20, 2023

View reviewed changes

matborowczyk added 8 commits January 2, 2024 18:10

Merge branch 'master' of github.com:inmanta/web-console into issue/53…

13ba59e

…86-embedded-entity-composer

Merge branch 'master' of github.com:inmanta/web-console into issue/53…

6ac5e26

…86-embedded-entity-composer

fixes + e2e

0f1787f

instanceComposer button late render fix

5a9fad1

Merge branch 'master' of github.com:inmanta/web-console into issue/53…

d8831d6

…86-embedded-entity-composer

test flake fix

68e4596

test fix

2391e5f

remarks

4c2fb4f

matborowczyk requested a review from LukasStordeur January 8, 2024 14:43

LukasStordeur reviewed Jan 9, 2024

View reviewed changes

matborowczyk added 2 commits January 9, 2024 21:18

remarks

10386a2

fix

dfc9abe

LukasStordeur reviewed Jan 10, 2024

View reviewed changes

remarks + fix

293fcd4

LukasStordeur approved these changes Jan 12, 2024

View reviewed changes

matborowczyk added the merge-tool-ready This ticket is ready to be merged in label Jan 12, 2024

inmantaci mentioned this pull request Jan 12, 2024

Issue/5386 embedded entity composer (#5387) #5442

Closed

inmantaci closed this Jan 12, 2024

inmantaci deleted the issue/5386-embedded-entity-composer branch January 12, 2024 09:38

This was referenced Jan 12, 2024

Refactor Instance Composer functions #5443

Closed

Composer- Button to add is slower than rest of the UI #5276

Closed

Issue/5386 embedded entity composer #5387

Issue/5386 embedded entity composer #5387

Conversation

matborowczyk commented Dec 13, 2023 • edited

Description

Self Check:

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

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

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

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

LukasStordeur left a comment

Choose a reason for hiding this comment

inmantaci commented Jan 12, 2024

inmantaci commented Jan 12, 2024

matborowczyk commented Dec 13, 2023 •

edited