-
Notifications
You must be signed in to change notification settings - Fork 10
feat: @W-18480458: Implementation of update references across Omnistudio components #353
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
feat: @W-18480458: Implementation of update references across Omnistudio components #353
Conversation
In case of OS and FC, both can have dependencies on each other. Is the correct order change mentioned taking care of that. |
The order of migration happens like DM, IP, OS and FC so if OS has any FC dependency it will be taken care by the namingRegistry because it has the oldName <-> cleanedName so it will update automatically. |
This will not ensure the correct reference update. The unmigrated or Error items will not be handled correctly. |
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
public preProcessComponents( | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-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.
Please try avoiding es lint checks
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.
will check
if (type === Constants.DataRaptorComponentName || type === 'DataRaptor') { | ||
// Handle DataRaptor using registry | ||
const originalBundle = dataSource.value?.bundle || ''; | ||
if (originalBundle && this.nameRegistry.hasDataMapperMapping(originalBundle)) { |
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.
In what scenarios will the nameRegistry might not have the nameMapping.
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.
In most cases, registry should have the key->value but added fallback so that any missing case also update will be done based on cleaned Name
if (originalBundle && this.nameRegistry.hasDataMapperMapping(originalBundle)) { | ||
dataSource.value.bundle = this.nameRegistry.getDataMapperCleanedName(originalBundle); | ||
} else { | ||
dataSource.value.bundle = this.cleanName(originalBundle); |
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.
Does the nameregistry need to be updated?
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.
Ideally we shouldn't see any misses because registry is prepared based on pre-migrated data and else logic is fallback to avoid not clean up names
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.
We should add some logs to let us know about any such miss
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.
sure @snehaljha-sf
/** | ||
* Get migration objects in the correct dependency order: | ||
* 1. DataMappers (lowest dependencies) | ||
* 2. OmniScripts/Integration Procedures |
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.
Update the comment to reflect the correct order.
break; | ||
case Constants.GlobalAutoNumber: | ||
migrationObjects.push(new GlobalAutoNumberMigrationTool(namespace, conn, this.logger, messages, this.ux)); | ||
migrationObjects.push( | ||
new GlobalAutoNumberMigrationTool(namespace, conn, this.logger, messages, this.ux, nameRegistry) |
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.
If this is independent item, do we still have dependency on nameRegistry.
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.
For now, I'm passing the namingRegistry to GlobalAutoNumber migration process as well so that we can use it in future for any reference updates
The failure/ error ones should be remigrated then name registry will take care of it of update references |
nameRegistry.clear(); // Clear any previous mappings | ||
|
||
Logger.log(messages.getMessage('startingComponentPreProcessing')); | ||
await this.preProcessAllComponents(nameRegistry, namespace, conn, migrateOnly); |
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.
nameRegistry follows a singleton pattern, we don't need to add this dependency while calling a function
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 mean NameMappingRegistry.getInstance() should be good instead of passing
// Use ProjectPathUtil for APEX project folder selection (matches assess.ts logic) | ||
projectPath = await ProjectPathUtil.getProjectPath(messages, true); | ||
targetApexNamespace = await this.getTargetApexNamespace(objectsToProcess, targetApexNamespace); | ||
await this.handleExperienceSitePrerequisites( |
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 has been moved to premigrate class.
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.
yes, its merge conflict issue taking care of it
@snehaljha-sf @sf-aastha-paruthi Please review the changes |
isExperienceBundleMetadataAPIProgramaticallyEnabled | ||
); | ||
Logger.logVerbose( | ||
'The objects to process after handleExpSitePrerequisite are ' + JSON.stringify(objectsToProcess) |
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 has been moved to message.json
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.
Even on latest, we aren't using the messages
allVersions: any | ||
): MigrationTool[] { | ||
if (!migrateOnly) { | ||
// Correct order: DataMapper -> OmniScript/IP -> FlexCard -> GlobalAutoNumber |
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.
nit, remove duplicate comment, comment in line 383 already covers the ordering.
/** | ||
* Query FlexCards from the org | ||
*/ | ||
private async queryFlexCards(conn: any, namespace: string): Promise<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.
All the 4 functions are making the same call to query and then return the resuts, just the query is different. The query can be separated and the other part can be put to a common function to avoid duplicacy.
src/migration/omniscript.ts
Outdated
@@ -196,10 +200,11 @@ export class OmniScriptMigrationTool extends BaseMigrationTool implements Migrat | |||
flexCardAssessmentInfos: FlexCardAssessmentInfo[] | |||
): Promise<OmniAssessmentInfo> { | |||
try { | |||
Logger.log(this.messages.getMessage('startingOmniScriptAssessment')); | |||
const exportComponentType = | |||
this.exportType === OmniScriptExportType.IP ? 'Integration Procedures' : 'Omniscripts'; |
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.
getName() can be used.
…dio components Changes: - Handling references udpates post migration clean up changes - Added logic to perform migrate in correct order DM -> IP -> OS and FC - Created the namingRegistry to store old and cleanup name for lookup - Added the test cases
…dio components Changes: - Handling references udpates post migration clean up changes - Added logic to perform migrate in correct order DM -> IP -> OS and FC - Created the namingRegistry to store old and cleanup name for lookup - Added the test cases
Changes: - Handling of skipping Angular Omniscript dependencies FC and OS - Split the Omniscript and IP migration and also corrected the labels - Added labels and test cases
34f3881
to
747cdb9
Compare
@snehaljha-sf @sf-aastha-paruthi addressed the review comments |
a8affc3
into
salesforcecli:prerelease/develop-ga
Changes:
What does this PR do?
What issues does this PR fix or reference?