-
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
feat: added notification when scratch org is about to expire #4304
Conversation
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.
Awesome stuff! Looking forward to seeing this feature in action 😎
if (isDemoMode()) { | ||
decorators.showDemoMode(); | ||
} | ||
await processPostProjectOpened(extensionContext); |
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 love moving all of this into a new function 🎉 I'm not sure about the method name here though, it took me a second to get what it was doing. Maybe something like initializeProject
?
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.
Yea, there wasn't really an "obvious" name for this. I like renaming it to initializeProject(). I'll go make this change.
check once a day, uncomment the following code. | ||
|
||
// And run again once every 24 hours. | ||
setInterval(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.
This would be good, especially for users that keep their projects open.
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! - I 100% agree, but Ananya doesn't. An argument she brought up was that users complain that our extension is too noisy (too many notifications). The plan is to release w/only displaying on activation, and then see what users think.
|
||
export async function checkForExpiredOrgs() { | ||
try { | ||
const daysBeforeExpire = 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.
Is this something we could put behind a custom setting, and maybe allow for no reminder? I usually made my scratch orgs last 7 days (some folks I know make theirs only for 1-2), so it would be a bit more frequent/intrusive. Also cool if that's a better fit for a subsequent item 👍
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 think it would be good for a followup item. Let's see how the users like this feature, and how they use it. If we add the "also check daily" feature in, it would be good then to revisit this and maybe add a custom setting. I think for right now, I'd like to not make all the options user configurable (too many options = too many choices for the user)
decorators.showOrg(); | ||
decorators.monitorOrgConfigChanges(); | ||
|
||
await setUpOrgExpirationWatcher(); |
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.
@randi274 Also, I initially had the impl for setUpOrgExpirationWatcher()
in index.ts, but it wasn't testable (there are no unit tests for the code in this index.ts... it's all tested implicitly by the integration tests) so I moved setUpOrgExpirationWatcher() into its own file (packages/salesforcedx-vscode-core/src/util/orgUtil.ts) and created unit tests.
@@ -643,5 +643,11 @@ export const messages = { | |||
'Unable to rename the component. Try renaming the component manually and then redeploying your changes.', | |||
error_function_type: 'Unable to determine type of executing function.', | |||
error_unable_to_get_started_function: | |||
'Unable to access the function in "{0}".' | |||
'Unable to access the function in "{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.
This text was provided by @sbudhirajadoc
if (isDemoMode()) { | ||
decorators.showDemoMode(); | ||
} | ||
await initializeProject(extensionContext); |
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.
👍🏽
daysUntilExpiration.setDate(daysUntilExpiration.getDate() + daysBeforeExpire); | ||
|
||
const orgList = new OrgList(); | ||
const authInfoObjects = await orgList.getAuthInfoObjects(); |
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 the draft PR that updates orgList to not read from the .sfdx directory, the return type of this method changes to return the OrgAuthorization type from core.
FileInfo is a type that is created after reading the files in the ~/.sfdx directory.
That change would require some reworking of this methods internals, to work with OrgAuthorization type and the AuthFields type for auth fields that are stored there in core v3, like expiration date: https://github.com/forcedotcom/salesforcedx-vscode/pull/4297/files#diff-2b72c15ab3c2e3d66f269772e80aed9902d2d85d9acb2a462116d03541484d4fR97
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.
Maybe this file could import and use AuthInfo.listAllAuthorizations()
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.
Good find @klewis-sfdc! Looks like this one will be behind the core v3 change in the queue. @jeffb-sfdc did you want to merge that work in now, or wait til we finish sorting out all of the larger issues?
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 spoke with Ken about this, and the plan is to merge in now and not be held up, then pull develop into his branch and then address this issue.
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'm good on this pending any QA findings 👍
Tested on Mac:
Attempted testing on Windows, but building there is still pretty difficult, so limited to Mac. Once the duplicate button is resolved, this is good to 🚢 |
Confirmed that duplicate button is now gone! ⚓ away! |
What does this PR do?
This PR adds a new feature which notifies the user if their scratch org expires in a few days
What issues does this PR fix or reference?
@W-11463274@
Functionality Before
Upon startup and activation, the Salesforce extension for VS Code would active.
Functionality After
The Salesforce extension for VS Code actives, and it now also checks the user's org list for active orgs about to expire.