-
Notifications
You must be signed in to change notification settings - Fork 10
@W-18480483: Flexipage wrapper assess #330
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
@W-18480483: Flexipage wrapper assess #330
Conversation
package.json
Outdated
@@ -15,6 +15,7 @@ | |||
"@types/jsdom": "^21.1.7", | |||
"@types/lodash.chunk": "^4.2.9", | |||
"@types/shelljs": "^0.8.15", | |||
"fast-xml-parser": "^5.2.5", |
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.
LWC migration also does XML parsing so do we need new library for XML parsing?
'status', | ||
flexipageAssessmentInfo.status, | ||
false, | ||
1, |
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 have component name, file reference, assessment status columns along with diff, errors/summary
RequestMethod.POST | ||
); | ||
response.results.forEach((result) => { | ||
results.set(result.referenceId, { |
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.
is it formatted only or any other changes?
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.
format
src/utils/XMLUtil.ts
Outdated
class XMLUtil { | ||
/** XML parser instance configured for Salesforce metadata */ | ||
private parser: XMLParser; | ||
/** XML builder instance configured for formatted output */ |
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.
common XML parser or specific to flexipage?
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.
specific to flexipage, I'll rename the file and class
.readdirSync(path.join(this.projectPath, 'force-app', 'main', 'default', 'flexipages')) | ||
.filter((file) => file.endsWith('.xml')); | ||
Logger.info(this.messages.getMessage('successfullyRetrievedFlexiPages', [files.length])); | ||
const progressBar = createProgressBar('Migrating', 'Flexipage'); |
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 the progress bar logic for other related objects as well, apex, lwc and exp sites
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.
Is this in scope of this story?
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.
Added note to make a note of the task
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 can do it later post the story is complete
@snehaljha-sf we should check-in into |
errors: [error instanceof Error ? error.message : JSON.stringify(error)], | ||
path: filePath, | ||
diff: '', | ||
status: 'Errors', |
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.
Failed or Error?
if (mode === 'migrate') { | ||
fs.writeFileSync(filePath, modifiedContent); |
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.
Lets connect on this?
if (!typeSubtypeLanguage) { | ||
throw new Error('target property not found for component ' + item.componentInstance.componentName); | ||
} | ||
const newProps = createNewProps(typeSubtypeLanguage); |
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 do have additional properties which we need fill them along with standard ones and also flex card follows differently
}, | ||
{ | ||
name: 'Can be Automated', | ||
count: flexipageAssessmentInfos.filter((info) => info.status === 'Can be Automated').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.
why it is warning here?
} from '../../../src/migration/interfaces'; | ||
|
||
describe('transformFlexipageBundle', () => { | ||
const namespace = 'runtime_omnistudio'; |
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.
here namespace should be the package one for the wrapper, this is wrong
...Object.entries(newProps).map(([key, value]: [string, string]) => ({ name: key, value })), | ||
]; | ||
changes = true; | ||
item.componentInstance.componentInstanceProperties = replacedProps; |
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.
Need to additional properties along with these ones check the target metadata once
// return { | ||
// language: 'English', | ||
// subType: 'OSForCustomLWC', | ||
// theme: 'lightning', |
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.
clean up the comment code
language: migratedScriptName.language || 'English', | ||
subType: migratedScriptName.subtype, | ||
type: migratedScriptName.type, | ||
theme: 'OSForCustomLWC', |
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.
why theme is hard coded @snehaljha-sf ?
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 need add logic here based on metadata file shared
|
||
return { | ||
componentName: targetComponentNameFlexCard, | ||
identifier: targetIdentifierFlexCard, |
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 don't need objectApiName and recordId in flexipages
|
||
export interface FlexiPageAssessmentInfo extends FileChangeInfo { | ||
errors: string[]; | ||
status: 'Can be Automated' | 'Errors' | 'No Changes' | 'Complete' | 'Failed'; |
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.
Do we need "No Changes"
rowspan: 1, | ||
}, | ||
{ | ||
name: 'Status', |
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 should be assessment status
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.
added comments
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.
changes looks good to me
shell.cd(this.projectPath); | ||
sfProject.retrieve(Constants.FlexiPage, this.org.getUsername()); | ||
const files = fs | ||
.readdirSync(path.join(this.projectPath, 'force-app', 'main', 'default', 'flexipages')) |
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.
readdirSync will return the immediate contents of the folder. Can you please confirm if flexipages retrieved in the folder don't have nested folder structure.
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.
that's expected
const filePath = path.join(this.projectPath, 'force-app', 'main', 'default', 'flexipages', file); | ||
try { | ||
const flexPageAssessmentInfo: FlexiPageAssessmentInfo = this.processFlexiPage(file, filePath, mode); | ||
flexPageAssessmentInfos.push(flexPageAssessmentInfo); |
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 flexPageAssessmentInfo has no changes, are we still showing it in the report?
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.
no, I am filtering it later
@@ -186,7 +186,8 @@ export default class Migrate extends OmniStudioBaseCommand { | |||
const relatedObjectMigrationResult = omnistudioRelatedObjectsMigration.migrateAll(objectsToProcess); | |||
generatePackageXml.createChangeList( | |||
relatedObjectMigrationResult.apexAssessmentInfos, | |||
relatedObjectMigrationResult.lwcAssessmentInfos | |||
relatedObjectMigrationResult.lwcAssessmentInfos, |
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 believe lwc is not supported in this version
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.
that's alright, as it'll be blank with LWC enabled change is already in progress
79449ae
into
salesforcecli:prerelease/develop-ga
What does this PR do?
What issues does this PR fix or reference?