-
Notifications
You must be signed in to change notification settings - Fork 42
feature/2252 resolve Update Contact Data activities in Journeys #2420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
feature/2252 resolve Update Contact Data activities in Journeys #2420
Conversation
…of calls in tests to accomodate the change
Coverage ReportCommit:14d2477Base: develop@36e9bb6 Details (changed files):
|
lib/metadataTypes/Journey.js
Outdated
| const activityArguments = activity.arguments; | ||
| for ( | ||
| let i = 0; | ||
| i < activityArguments.activityData.updateContactFields.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are assuming activityData and updateContactFields both exist here. Better to use ?. notation to make sure you dont trigger a js error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
lib/metadataTypes/Journey.js
Outdated
| ) { | ||
| const contactField = activityArguments.activityData.updateContactFields[i]; | ||
| try { | ||
| activityArguments.activityData.updateContactFields[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use contactField var instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
8ab518e to
4957506
Compare
lib/metadataTypes/Journey.js
Outdated
| Util.logger.warn(' - Caching dependent Metadata: dataExtensionField'); | ||
| DataExtensionField.buObject = this.buObject; | ||
| DataExtensionField.client = this.client; | ||
| DataExtensionField.properties = this.properties; | ||
| const dataExtensionFieldCache = | ||
| await DataExtensionField.retrieveForCache(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only re-cache if dataExtensionFields were not already cached please. where you placed it, this caching gets executed once for every journey in your retrieve
You can find an example of how i did that in Journey.deploy and Journey._postRetrieveTasksBulk:
I would recommend putting YOUR caching also into those 2 methods in a similar fashion (quick loop throuh journeys and the activities to check if the activity is even in the retrieve/deploy payload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just realized, there might be a slightly better option here:
u can use something like in DataExtension._retrieveFieldsForSingleDe() - that way it works nicely even on BUs with a extremely large amount of DEs / fields.
that said, this will mean quite a bit of extra work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a function in the DataExtensionField like the one in the DataExtension retrieveFieldsForSingleDe. Let me know if that is ok 🙂
lib/metadataTypes/Journey.js
Outdated
| dataExtensionFieldCache.metadata | ||
| ); | ||
| try { | ||
| contactField.field = cache.searchForField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i get why you wanted to stick with "field" but the standard here would be r__dataExtensionField_name
lib/metadataTypes/Journey.js
Outdated
| try { | ||
| contactField.field = cache.searchForField( | ||
| 'dataExtensionField', | ||
| `[${contactField.r__dataExtension_key}].[${contactField.field}]`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `[${contactField.r__dataExtension_key}].[${contactField.field}]`, | |
| `[${contactField.r__dataExtension_key}].[${contactField.r__dataExtensionField_name}]`, |
…https://github.com/Accenture/sfmc-devtools into feature/2252-resolve-update-contact-data-in-Journeys
Co-authored-by: Jörn Berkefeld <JoernBerkefeld@users.noreply.github.com>
Co-authored-by: Jörn Berkefeld <JoernBerkefeld@users.noreply.github.com>
| const fields = await DataExtensionField.retrieveFieldsForSingleDe( | ||
| contactField.r__dataExtension_key | ||
| ); | ||
| cache.setMetadata('dataExtensionField', fields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wont work because now your cache for dataExtensionField is existing - but you only inserted fields for a single DE. Therefore, if you run this code for 2 updatecontactdata activities with a different dataExtension then the second one will not recache fields and hence fail to find any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what could work is if you were to reset the dataExtensionField cache after running searchForField again - but only if you didnt find a dataExtensionField cache in the first place? bit hacky but should be most efficient that way.
let tempCachedFields = false;
if (!cache.getCache().dataExtensionField) {
tempCachedFields = true;
...
}
... cache.searchForField(..);
if(tempCachedFields) {
cache.clearCache(this.buObject.mid, 'dataExtensionField');
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, makes sense. I made the changes as suggested, in preDeploy and postRetrieve. Since the function retrieves all fields per DE, and one Update Contact activity is related to one DE only, cache is cleared once per UPDATECONTACTDATA activity. It will not re-cache once for every field, but once per every data extension / Update Contact activity.
| if (!cache.getCache().dataExtensionField) { | ||
| DataExtensionField.buObject = this.buObject; | ||
| DataExtensionField.client = this.client; | ||
| DataExtensionField.properties = this.properties; | ||
| const fields = await DataExtensionField.retrieveFieldsForSingleDe( | ||
| contactField.r__dataExtension_key | ||
| ); | ||
| cache.setMetadata('dataExtensionField', fields); | ||
| } | ||
| contactField.r__dataExtensionField_name = cache.searchForField( | ||
| 'dataExtensionField', | ||
| contactField.field, | ||
| 'ObjectID', | ||
| 'Name' | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on resetting the cache in _preDeployTasks_activities and solve similarly here
| const cachedDEs = cache.getCache().dataExtension; | ||
| if (cachedDEs) { | ||
| await DataExtension.attachFields(cachedDEs); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert as you are not using this change. i believe whenever we want to cache fields, we ended up looking at them through dataExtensions alone and hence had the above logic in place
|
|
||
| // Assign definition to static attributes | ||
| import MetadataTypeDefinitions from '../MetadataTypeDefinitions.js'; | ||
| import cache from '../util/cache.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please restore in favor of old version of retrieveForCache()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted retrieveForCache() and brought back the imports
PR details
What changes did you make? (Give an overview)
Before

After

Added some changes to have the Data Extension field name resolved and add the Data Extension key to the journey instead of ObjectID.
Further details (optional)
...
Checklist