[wip don't review]

Testing

Include any additional information about the testing you have completed to ensure your changes behave as expected. For a speedy review, please check any of the tasks you completed below during your testing.

[ ] Added unit tests for new functionality
[ ] Tested end-to-end using the local server
[ ] [Segmenters] Tested in the staging environment

Jun 14 '24 23:06 nick-Ag

hi @nick-Ag - I know this is still in draft, so just out of curiosity what will this do?

  groupConditions: {
    anyOf: ['external_id', 'user_alias', 'email', 'braze_id']
  },

Will it hide or show fields in the UI depending on which fields have a mapping populated? Or will it perform a runtime validation which will prevent the perform() function from being invoked if none of the fields have a value?

Keen to understand the intent here as I think the second approach would be really valuable.

Jun 20 '24 09:06 joe-ayoub-segment

hi @nick-Ag - I know this is still in draft, so just out of curiosity what will this do?
  groupConditions: {
    anyOf: ['external_id', 'user_alias', 'email', 'braze_id']
  },
Will it hide or show fields in the UI depending on which fields have a mapping populated? Or will it perform a runtime validation which will prevent the perform() function from being invoked if none of the fields have a value?

Keen to understand the intent here as I think the second approach would be really valuable.

Planning on the second approach primarily. I also want the validation to appear in the UX so that users can't save mappings where the group condition has not been met. Looking forward to showing more soon!

Jun 20 '24 17:06 nick-Ag

New required fields detected

[!WARNING] Your PR adds new required fields to an existing destination. Adding new required settings/mappings for a destination already in production requires updating existing customer destination configuration. Ignore this warning if this PR is for a new destination with no active customers in production.

The following required fields were added in this PR:

Destination: Salesforce (Actions), Action Field(s):last_name

Add these new fields as optional instead and assume default values in perform or performBatch block.

Nov 14 '24 17:11 github-actions[bot]

Codecov Report

Attention: Patch coverage is 93.63636% with 7 lines in your changes missing coverage. Please review.

Project coverage is 79.03%. Comparing base (4888857) to head (2fb6b01). Report is 34 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/core/src/destination-kit/fields-to-jsonschema.ts	93.57%	7 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2099      +/-   ##
==========================================
+ Coverage   78.47%   79.03%   +0.56%     
==========================================
  Files        1032     1086      +54     
  Lines       18590    21453    +2863     
  Branches     3521     4307     +786     
==========================================
+ Hits        14588    16955    +2367     
- Misses       2825     3300     +475     
- Partials     1177     1198      +21

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Dec 02 '24 06:12 codecov[bot]

hi @nick-Ag I can't wait to start using this new feature :). Can you tell me what happens when null, '' and undefined values are passed in to a string field which is conditionally required? Also what if the field is defined as AllowNull: true?

Dec 03 '24 11:12 joe-ayoub-segment

Hi @nick-Ag , qq:

does/should the generated-types.ts file change if conditions are used?
Can you show examples how to use multiple conditions, with all/any (assuming those use-cases are supported)?

Dec 03 '24 16:12 joe-ayoub-segment

Hi @nick-Ag , qq:

does/should the generated-types.ts file change if conditions are used?

Can you show examples how to use multiple conditions, with all/any (assuming those use-cases are supported)?

The generated-types file won't change, fields which are conditionally required will be optional (ex: last_name?: string). See here
Those cases are supported, see the unit test here for an example. I've tried to test every possible use-case I could think of in that test file

Dec 03 '24 17:12 nick-Ag

hi @nick-Ag I can't wait to start using this new feature :). Can you tell me what happens when null, '' and undefined values are passed in to a string field which is conditionally required? Also what if the field is defined as AllowNull: true?

I'm going to make a small update to the PR to handle null values as well, thanks for pointing out this case

Dec 04 '24 00:12 nick-Ag

hi @nick-Ag. Couple of questions about how this code will behave:

what will happen in the following scenario? I assume the UI will not fail validation. However when the event gets sent for processing it will fail? Does this mean that validation on the front end needs to be slightly different to validation that happens when an event is being processed?

{
   ... 
   label: 'last_name'
   conditions: [
          {
            fieldKey: 'operation',
            operator: 'is',
            value: 'create'
          }
    ]
 },
{
   ... 
   label: 'operation'
   default: {
      '@path': '$.propertries.operation'
   }
 }

what will the error messages be when validation fails? How useful will they be? Are there any examples you can share? The reason I ask is that JSON schema error messages aren't always that helpful for the end user.

Jan 13 '25 09:01 joe-ayoub-segment

Looks great 🙌 This is not part of the scope? right?

    address: {
      label: 'Address',
      type: 'object',
      properties: {
        city: {
          label: 'city',
          type: 'string',
          required: [Condition Here]
        },

wondering if we handle it gracefully if someone puts a condition on the required field ( ideal if we can prevent it via typescript )

Jan 13 '25 17:01 pooyaj

hi @nick-Ag. Couple of questions about how this code will behave:

what will happen in the following scenario? I assume the UI will not fail validation. However when the event gets sent for processing it will fail? Does this mean that validation on the front end needs to be slightly different to validation that happens when an event is being processed?
{
   ... 
   label: 'last_name'
   conditions: [
          {
            fieldKey: 'operation',
            operator: 'is',
            value: 'create'
          }
    ]
 },
{
   ... 
   label: 'operation'
   default: {
      '@path': '$.propertries.operation'
   }
 }
what will the error messages be when validation fails? How useful will they be? Are there any examples you can share? The reason I ask is that JSON schema error messages aren't always that helpful for the end user.

Really good question here. In this case the UI will consider that a valid mapping and pass validation. However if there is no value at $.properties.operation for the event then the validation will fail at the actions level. The validation between the UI and event delivery is only different in the sense that UI validation checks only the users mapping (before path directives are applied) and the event delivery validation will validate against the final payload (path directives applied to the event).
Here's an example of an error message in the case that last_name is missing.

[
	{
		"statusCode": 500,
		"message": "The root value is missing the required field 'last_name'. The root value must match \"then\" schema.",
		"stack": [
			"AggregateAjvError: The root value is missing the required field 'last_name'. The root value must match \"then\" schema.",
...

Jan 13 '25 19:01 nick-Ag

Looks great 🙌 This is not part of the scope? right?
    address: {
      label: 'Address',
      type: 'object',
      properties: {
        city: {
          label: 'city',
          type: 'string',
          required: [Condition Here]
        },
wondering if we handle it gracefully if someone puts a condition on the required field ( ideal if we can prevent it via typescript )

Fantastic question. Based on a unit test I just wrote that case is not handled correctly. Going to circle back and address it before merging. Will consider both preventing it entirely vs. implementing it correctly

Jan 13 '25 19:01 nick-Ag

Looks great 🙌 This is not part of the scope? right?
    address: {
      label: 'Address',
      type: 'object',
      properties: {
        city: {
          label: 'city',
          type: 'string',
          required: [Condition Here]
        },
wondering if we handle it gracefully if someone puts a condition on the required field ( ideal if we can prevent it via typescript )
Fantastic question. Based on a unit test I just wrote that case is not handled correctly. Going to circle back and address it before merging. Will consider both preventing it entirely vs. implementing it correctly

Nice question @pooyaj . My vote is to implement it if possible (if @nick-Ag can do it!). The reason is that Object fields are a neat way to squeeze a lot of fields into a small amount of page real estate. and they also allow us to locate related fields close together. It would be a missed opportunity if 'required' didn't work on these sub fields.

Jan 14 '25 10:01 joe-ayoub-segment

Looks great 🙌 This is not part of the scope? right?
    address: {
      label: 'Address',
      type: 'object',
      properties: {
        city: {
          label: 'city',
          type: 'string',
          required: [Condition Here]
        },
wondering if we handle it gracefully if someone puts a condition on the required field ( ideal if we can prevent it via typescript )
Fantastic question. Based on a unit test I just wrote that case is not handled correctly. Going to circle back and address it before merging. Will consider both preventing it entirely vs. implementing it correctly
Nice question @pooyaj . My vote is to implement it if possible (if @nick-Ag can do it!). The reason is that Object fields are a neat way to squeeze a lot of fields into a small amount of page real estate. and they also allow us to locate related fields close together. It would be a missed opportunity if 'required' didn't work on these sub fields.

Have implemented that use case & updated the README with some documentation about this feature. This one is ready to go out. Looking for final review @joe-ayoub-segment , @varadarajan-tw , @pooyaj

Jan 16 '25 00:01 nick-Ag