Skip to content
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: no json connectedStatus on skipconnection #27

Merged
merged 10 commits into from
Feb 16, 2021

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented Feb 9, 2021

What does this PR do?

toolbelt: --json --skipconnectionstatus would have no property for connectedStatus on non-scratch orgs
plugin: set the connectedStatus to 'Unknown'

This change fixes that.

We also never update isDevHub, and a viable customer scenario is using a prod org for CLI work (ex w/ sandboxes) and then turning on DevHub to get scratch orgs. Now org:list re-checks on the server. forcedotcom/cli#482

h/t @SCWells72 for all the help

What issues does this PR fix or reference?

@W-8871284@
@W-8881661@ forcedotcom/cli#482

if (flags.skipconnectionstatus) {
fields.connectedStatus = fields.connectedStatus ?? 'Unknown';
} else {
if (!flags.skipconnectionstatus) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mshanemc is there a test to cover this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it('skipconnectionstatus', async () => {

the command is just passing that flag to orgListUtil so the test is at that level instead of on the command.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mshanemc that test verifies what happens when skipconnectionstatus is true. Is there an existing test that verifies results when it is false?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to put it, is there a way this bug could have been caught with a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that test now asserts that --skipconnectionstatus produces nonScratchOrgs with no connectedStatus in the json

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mshanemc Does the test below represent verification of the correct behavior when flags.skipconnectionstatus is false?

it('readLocallyValidatedMetaConfigsGroupedByOrgType', async () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when migrating this command, I tried to keep the tests on orgListUtil as close to the original as possible.

I don't like the original either, so I'm gonna fix them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better?

Copy link
Contributor

@peternhale peternhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@075a14c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #27   +/-   ##
=======================================
  Coverage        ?   83.21%           
=======================================
  Files           ?        6           
  Lines           ?      292           
  Branches        ?       42           
=======================================
  Hits            ?      243           
  Misses          ?       35           
  Partials        ?       14           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 075a14c...784998d. Read the comment docs.

@WillieRuemmele WillieRuemmele merged commit ea42676 into main Feb 16, 2021
@WillieRuemmele WillieRuemmele deleted the sm/json-regression-fix branch February 16, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants