-
Notifications
You must be signed in to change notification settings - Fork 407
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
Retrieve default org's metadata types #1237
Conversation
const workspaceChecker = new SfdxWorkspaceChecker(); | ||
const parameterGatherer = new EmptyParametersGatherer(); | ||
|
||
export async function forceDescribeMetadata(outputPath?: string) { |
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 get called whenever the view is refreshed
xmlName: string; | ||
}; | ||
|
||
export function buildTypesList(metadataTypesPath: string): string[] { |
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 would be used to populate the top level child nodes for the metadata types
return metadataTypes; | ||
} else { | ||
const err = | ||
'There was an error retrieving metadata type information. Refresh the view to retry.'; |
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.
@ruthemmanuelle If we run into an error when trying to retrieve the metadata types from the generated output file, I think we should throw an error. If the user runs into this scenario, they should be able to refresh the view to generate the output file and load the view again.
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.
Sorry, I just saw this. Can we tell them how to refresh the view? Or is it obvious? Not sure what this UI looks like.
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 taken out this message with my latest change, but at some point we will have to display a message similar to this. I think the view will include a refresh button in the top right corner, so we would want to point the user to that.
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.
Ok, it sounds like refreshing will be intuitive, so the message you had (or something similar to it) should be good. Let me know when you're ready to add the new message.
Codecov Report
@@ Coverage Diff @@
## develop #1237 +/- ##
===========================================
- Coverage 70.98% 70.95% -0.04%
===========================================
Files 196 198 +2
Lines 7397 7471 +74
Branches 781 788 +7
===========================================
+ Hits 5251 5301 +50
- Misses 1990 2012 +22
- Partials 156 158 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## develop #1237 +/- ##
===========================================
- Coverage 71% 70.95% -0.05%
===========================================
Files 196 198 +2
Lines 7397 7469 +72
Branches 781 787 +6
===========================================
+ Hits 5252 5300 +48
- Misses 1989 2012 +23
- Partials 156 157 +1
Continue to review full report at Codecov.
|
}; | ||
|
||
export function buildTypesList(metadataTypesPath: string): string[] { | ||
if (fs.existsSync(metadataTypesPath)) { |
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.
Since we're parsing a json, I think we should wrap the method on a try catch that surfaces that throws the error and have the code calling this handle surfacing the message.
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.
Okay, that works
@@ -164,6 +164,19 @@ export class TelemetryService { | |||
} | |||
} | |||
|
|||
public sendMetadataTypes( |
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.
Should this be a more generic method that we use to log things that are not errors or command executions ? It could be something like sendEventData
or something like that.
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.
Yeah, I think that's a good idea!
getUsernameStub.restore(); | ||
}); | ||
|
||
it('returns the path for a given username', async () => { |
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.
Nitpick, change the title to Should return ...
to follow the current test conventions.
}); | ||
|
||
it('should return the path for a given username', async () => { | ||
getDefaultUsernameStub.returns('defaultUsername'); |
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 always return an username in email format e.g. [email protected]
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 kept the getDefaultUsernameStub
as defaultAlias
because it can return an alias or a username, but I updated the getUsername
stub to be in an email format, since that ensures we only have the username.
@@ -164,6 +164,16 @@ export class TelemetryService { | |||
} | |||
} | |||
|
|||
public sendEventData( |
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 add a test for this in test/vscode-integration/telemetry/index.test.ts
since no other method is testing sending measures.
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 does this PR do?
This PR adds the functionality to get the metadataTypes for the default org and save the results in the .sfdx/orgs folder. It does not include the activation of this process so this code will not get triggered.
What issues does this PR fix or reference?
@W-5980289@