-
Notifications
You must be signed in to change notification settings - Fork 70
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
fix: add a did you mean action to AuthInfo #1079
Conversation
try { | ||
await shouldThrow(AuthInfo.create({ username: '[email protected]', isDevHub: true })); | ||
} catch (e) { | ||
expect(e).to.have.property('name', expectedErrorName); | ||
expect(e).to.have.property('message', 'No authorization information found for [email protected].'); | ||
expect(e).to.have.property('actions'); | ||
expect((e as SfError).actions).to.deep.equal([ |
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 was failing to assert the actions[]
was equal to the set array, had to break the assertion up
src/org/authInfo.ts
Outdated
name: 'NamedOrgNotFoundError', | ||
message: messages.getMessage('namedOrgNotFound', [username]), | ||
actions: [ | ||
`It looks like you mistyped the username or alias. Did you mean "${findSuggestion(username, [ |
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.
Could it be misleading to tell the user that they mistyped the username or alias? What happens if they provide a username or alias that simply doesn't exist?
Maybe simplifying it to just Did you mean "<suggestion>"?
would be the most flexible
Also do we need to remove the action if no suggestion is found?
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.
Juliet and I had some back and forth on this. We thought the current messaging was friendlier, but we also had exactly what you suggested 🤷♂️
I'll remove it if nothing is passed, or we couldn't deduce any similar 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.
I'll happily defer to Juliet on the message 👍
src/org/authInfo.ts
Outdated
message: messages.getMessage('namedOrgNotFound', [username]), | ||
actions: [ | ||
`It looks like you mistyped the username or alias. Did you mean "${findSuggestion(username, [ | ||
...(await this.stateAggregator.orgs.list()).map((f) => f.split('.json')[0]), |
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 splitting the filename better than getting the username
prop from the contents? Or can we not guarantee that that property will be there? More curious than anything
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, username is optional in the contents
QA linked sfdx-core to plugin-org and linked plugin-org to sf Aliases in my env:
🟢
🟢
🟢 unset all aliases and then
❓ What are your thoughts on making this a bit smarter? Seems weird to give a username suggestion when the user provided something that doesn't look like a username. Maybe we can check for 🔴 unset all aliases and delete all orgs then
I'd expect this to not suggest anything |
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 QA
QA #2 🟢 no aliases and no orgs,
|
What does this PR do?
adds a "did you mean" suggestion to the
SfError
thrown when we can't find a passed in username/alias when constructing anAuthInfo
class.What issues does this PR fix or reference?
@W-15921235@