SALTO-6493: Change Validator that validates Flows have the referenced Elements #6978

AlmogMesilaty · 2024-12-19T13:55:49Z

SALTO-6493: Change Validator that validates Flows have the referenced Elements

Additional context for reviewer:
Error message format:

Release Notes:
None

User Notifications:
None

coveralls · 2024-12-19T14:16:23Z

coverage: 93.787% (+0.1%) from 93.66%
when pulling 885f71c on AlmogMesilaty:SALTO-6493
into b65522e on salto-io:main.

idoamit8

Great job!! 💯
I added some comments but will leave the rest of the review to @Uri-Bar and @tamtamirr

As we discussed, I noticed you're not using any get methods from the Map and are treating it purely as key-value pairs.
In this case, I would personally recommend using Record. While both have similar capabilities, Record is a simpler data structure and should be more than sufficient here.

idoamit8 · 2024-12-19T14:27:28Z

packages/salesforce-adapter/src/change_validators/flow_referenced_elements.ts

+
+const isFlowNode = (fields: FieldMap): boolean => 'locationX' in fields && 'locationY' in fields
+
+const isFlowNodeName = (value: unknown, path: ElemID, parent: ObjectType): value is string =>


I am might missing something, but I don't like the return value here because it doesn't just check if the value is a string; it also includes additional logic. I think the function should return a boolean instead.

idoamit8 · 2024-12-19T14:27:40Z

packages/salesforce-adapter/src/change_validators/flow_referenced_elements.ts

+const isFlowNodeName = (value: unknown, path: ElemID, parent: ObjectType): value is string =>
+  isFlowNode(parent.fields) && path.name === 'name' && _.isString(value)
+
+const isTargetReference = (value: unknown, path: ElemID): value is string =>


idoamit8 · 2024-12-19T14:31:40Z

packages/salesforce-adapter/src/change_validators/flow_referenced_elements.ts

+  const flowNodes = new Map<string, ElemID>()
+  const targetReferences = new Map<string, ElemID>()
+  const findFlowNodesAndTargetReferences: TransformFuncSync = ({ value, path, field }) => {
+    if (!field || !path) return value


When a variable is not a boolean, avoid using ! to check if it's undefined, as it will also return the same result for values like 0, an empty string, or other falsy values.

idoamit8 · 2024-12-19T14:39:19Z

packages/salesforce-adapter/src/change_validators/flow_referenced_elements.ts

+  const errors: ChangeError[] = []
+  const isReferenceToMissingElement = ([reference, _elemId]: [string, ElemID]): boolean => !flowNodes.has(reference)
+  const missingReferencedElementsErrors = Array.from(targetReferences).filter(isReferenceToMissingElement)
+  const isUnreferencedElement = ([reference, _elemId]: [string, ElemID]): boolean => !targetReferences.has(reference)
+  const unreferencedElementsErrors = Array.from(flowNodes).filter(isUnreferencedElement)
+  if (missingReferencedElementsErrors.length > 0) {
+    missingReferencedElementsErrors.forEach(([targetReference, elemId]) =>
+      errors.push(createMissingReferencedElementChangeError(targetReference, elemId)),
+    )
+  }
+  if (unreferencedElementsErrors.length > 0) {
+    unreferencedElementsErrors.forEach(([flowNode, elemId]) =>
+      errors.push(createUnreferencedElementChangeError(flowNode, elemId)),
+    )
+  }
+  return errors


I find this a bit clearer

Suggested change

const errors: ChangeError[] = []

const isReferenceToMissingElement = ([reference, _elemId]: [string, ElemID]): boolean => !flowNodes.has(reference)

const missingReferencedElementsErrors = Array.from(targetReferences).filter(isReferenceToMissingElement)

const isUnreferencedElement = ([reference, _elemId]: [string, ElemID]): boolean => !targetReferences.has(reference)

const unreferencedElementsErrors = Array.from(flowNodes).filter(isUnreferencedElement)

if (missingReferencedElementsErrors.length > 0) {

missingReferencedElementsErrors.forEach(([targetReference, elemId]) =>

errors.push(createMissingReferencedElementChangeError(targetReference, elemId)),

)

}

if (unreferencedElementsErrors.length > 0) {

unreferencedElementsErrors.forEach(([flowNode, elemId]) =>

errors.push(createUnreferencedElementChangeError(flowNode, elemId)),

)

}

return errors

const targetEntries = Array.from(targetReferences)

const flowEntries = Array.from(flowNodes)

const missingReferenceErrors = targetEntries

.filter(([reference]) => !flowNodes.has(reference))

.map(([reference, elemId]) => createMissingReferencedElementChangeError(reference, elemId))

const unreferencedErrors = flowEntries

.filter(([reference]) => !targetReferences.has(reference))

.map(([reference, elemId]) => createUnreferencedElementChangeError(reference, elemId))

return [...missingReferenceErrors, ...unreferencedErrors]

(But if the lists can be very long sometimes ... is not a good approach and its better to push)

tamtamirr

Great work 👑 Added some comments

tamtamirr · 2024-12-22T08:54:53Z

packages/salesforce-adapter/src/change_validators/flow_referenced_elements.ts

+const isFlowNodeName = (value: unknown, path: ElemID, parent: ObjectType): value is string =>
+  isFlowNode(parent.fields) && path.name === 'name' && _.isString(value)
+
+const isTargetReference = (value: unknown, path: ElemID): value is string =>
+  path.name === 'targetReference' && _.isString(value)


These methods seem redundant to me. I think it makes more sense to have these assertions inlined within the code of your transformFunc instead of having a type-guard here :)

tamtamirr · 2024-12-22T08:56:31Z

packages/salesforce-adapter/src/change_validators/flow_referenced_elements.ts

+const hasFlowReferenceError = ({
+  targetReferences,
+  flowNodes,
+}: {
+  targetReferences: Map<string, ElemID>
+  flowNodes: Map<string, ElemID>
+}): boolean =>


Why do we run assertion twice? Can we avoid that?

tamtamirr · 2024-12-22T08:59:58Z

packages/salesforce-adapter/test/change_validators/flow_referenced_elements.test.ts

+  const FlowConnector = createMetadataObjectType({
+    annotations: {
+      metadataType: 'FlowConnector',
+    },
+    fields: {
+      targetReference: { refType: BuiltinTypes.STRING },
+    },
+  })
+  const FlowNode = createMetadataObjectType({
+    annotations: {
+      metadataType: 'FlowNode',
+    },
+    fields: {
+      name: { refType: BuiltinTypes.STRING },
+      locationX: { refType: BuiltinTypes.NUMBER, annotations: { constant: 1 } },
+      locationY: { refType: BuiltinTypes.NUMBER, annotations: { constant: 1 } },
+      connector: { refType: FlowConnector, annotations: { required: false } },
+    },
+  })
+  const Flow = createMetadataObjectType({
+    annotations: {
+      metadataType: 'Flow',
+    },
+    fields: {
+      start: { refType: FlowNode },
+      actionCalls: { refType: new ListType(FlowNode), annotations: { required: false } },
+      assignments: { refType: new ListType(FlowNode), annotations: { required: false } },
+      decisions: { refType: new ListType(FlowNode), annotations: { required: false } },
+      recordCreates: { refType: new ListType(FlowNode), annotations: { required: false } },
+    },
+  })


nit: Move this into a beforeEach. Since Elements are mutable, it's better to initialize them for each test.

tamtamirr · 2024-12-22T09:00:35Z

packages/salesforce-adapter/test/change_validators/flow_referenced_elements.test.ts

+        {
+          fullName: 'TestFlow',
+          start: {
+            connector: { targetReference: 'ActionCall' },


Can we const targetReference and use it both in the code and UTs?

tamtamirr · 2024-12-22T09:01:00Z

packages/salesforce-adapter/test/change_validators/flow_referenced_elements.test.ts

+    })
+    it('should not return any errors', async () => {
+      const errors = await flowReferencedElements([flowChange])
+      expect(errors).toHaveLength(0)


Suggested change

expect(errors).toHaveLength(0)

expect(errors).toBeEmpty()

tamtamirr · 2024-12-22T09:06:37Z

packages/salesforce-adapter/test/change_validators/flow_referenced_elements.test.ts

+      expect(errors).toHaveLength(1)
+      const [error] = errors
+      expect(error.severity).toEqual('Error')
+      expect(error.message).toEqual('Reference to missing Flow Element')
+      expect(error.detailedMessage).toEqual(`The Flow Element "${'ActionCall'}" does not exist.`)


Suggested change

expect(errors).toHaveLength(1)

const [error] = errors

expect(error.severity).toEqual('Error')

expect(error.message).toEqual('Reference to missing Flow Element')

expect(error.detailedMessage).toEqual(`The Flow Element "${'ActionCall'}" does not exist.`)

expect(errors).toEqual([{

message: '...',

detailedMessge: '...',

severity: 'Error',

elemID: missingRefElemID,

}])

Nit: Can be greatly simplified

tamtamirr · 2024-12-22T09:06:57Z

packages/salesforce-adapter/test/change_validators/flow_referenced_elements.test.ts

+      expect(errors).toHaveLength(1)
+      const [error] = errors
+      expect(error.severity).toEqual('Info')
+      expect(error.message).toEqual('Unused Flow Element')
+      expect(error.detailedMessage).toEqual(`The Flow Element “${'ActionCall'}” isn’t being used in the Flow.`)


Nit: Can be greatly simplified

tamtamirr · 2024-12-22T09:07:39Z

packages/salesforce-adapter/test/change_validators/flow_referenced_elements.test.ts

+      )
+      flowChange = toChange({ after: flow })
+    })
+    it('should create change error per error', async () => {


Suggested change

it('should create change error per error', async () => {

it('should create change error per issue', async () => {

Nit: I think it's confusing to use the term error on both sides

tamtamirr · 2024-12-22T09:07:59Z

packages/salesforce-adapter/test/change_validators/flow_referenced_elements.test.ts

+    })
+    it('should create change error per error', async () => {
+      const errors = await flowReferencedElements([flowChange])
+      expect(errors).toHaveLength(3)


Let's validate the errors explicitly :)

tamtamirr · 2024-12-22T09:09:12Z

packages/salesforce-adapter/config_doc.md

@@ -355,3 +355,4 @@ For more details see the DeployOptions section in the [salesforce documentation
 | accountSettings              | true                   | Cannot set a value for enableAccountOwnerReport without proper org setting.                       |
 | unknownPicklistValues        | true                   | Disallow any usage of unknown pickList values.                                                    |
 | customApplications           | true                   | Check for multiple action overrides for the same action in customApplications instances.          |
+| flowReferencedElements       | true                   | Check that the referenced elements in Flow exist.                                                    |


nit: Shouldn't the | be aligned with the rest? (I think prettier should solve that)

Updated tests according to CR Added test

idoamit8

NICE! 💯
Added two small nits questions

idoamit8 · 2024-12-23T12:04:30Z

packages/salesforce-adapter/src/change_validators/flow_referenced_elements.ts

+type ReferenceToElemId = {
+  referenceOrName: string
+  elemId: ElemID
+  kind?: string


Can kind ever be a string other than FLOW_NODE or TARGET_REFERENCE?
If not, it might be clearer to make this explicit.

Suggested change

kind?: string

kind?: typeof FLOW_NODE | typeof TARGET_REFERENCE

idoamit8 · 2024-12-23T12:07:32Z

packages/salesforce-adapter/src/change_validators/flow_referenced_elements.ts

+type ReferenceToElemId = {
+  referenceOrName: string
+  elemId: ElemID
+  kind?: string


Why do you allow kind to be undefined in the type? It seems it always defined in your code

Updated according to CR

AlmogMesilaty added 11 commits December 17, 2024 10:04

Created CV file

92daab5

Implemented CV

73b29eb

Name change and pipeline

e6d3008

extra

4a469bd

Changes

f4b5792

better implementation

dd73ec9

better implementation

a553051

Removed duplication related code

24b71c2

Replaced flowelement with flownode, added unreferenced nodes check

ff5bef0

change error per error

6e1ba32

Added tests

2104626

AlmogMesilaty requested review from tamtamirr, Uri-Bar and idoamit8 December 19, 2024 13:55

combined get flow nodes and get target references

c2e8dd2

idoamit8 reviewed Dec 19, 2024

View reviewed changes

Added CV to doc

9d88f67

tamtamirr reviewed Dec 22, 2024

View reviewed changes

Updated according to CR

c8fb7f2

Updated tests according to CR Added test

AlmogMesilaty requested review from tamtamirr and idoamit8 December 23, 2024 11:53

idoamit8 reviewed Dec 23, 2024

View reviewed changes

Started changes

885f71c

Updated according to CR

AlmogMesilaty force-pushed the SALTO-6493 branch from c5f670b to 885f71c Compare December 24, 2024 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SALTO-6493: Change Validator that validates Flows have the referenced Elements #6978

SALTO-6493: Change Validator that validates Flows have the referenced Elements #6978

AlmogMesilaty commented Dec 19, 2024

coveralls commented Dec 19, 2024 •

edited

Loading

idoamit8 left a comment

idoamit8 Dec 19, 2024

idoamit8 Dec 19, 2024

idoamit8 Dec 19, 2024

idoamit8 Dec 19, 2024

tamtamirr left a comment

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

tamtamirr Dec 22, 2024

idoamit8 left a comment

idoamit8 Dec 23, 2024

idoamit8 Dec 23, 2024


		const isFlowNode = (fields: FieldMap): boolean => 'locationX' in fields && 'locationY' in fields

		const isFlowNodeName = (value: unknown, path: ElemID, parent: ObjectType): value is string =>

	it('should create change error per error', async () => {
	it('should create change error per issue', async () => {

	kind?: string
	kind?: typeof FLOW_NODE \| typeof TARGET_REFERENCE

SALTO-6493: Change Validator that validates Flows have the referenced Elements #6978

Are you sure you want to change the base?

SALTO-6493: Change Validator that validates Flows have the referenced Elements #6978

Conversation

AlmogMesilaty commented Dec 19, 2024

coveralls commented Dec 19, 2024 • edited Loading

idoamit8 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

tamtamirr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idoamit8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 19, 2024 •

edited

Loading