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.

Sign up for GitHub

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

Jump to bottom

Authorize orgs against different URLs (production, sandbox, or custom) #818

Merged
merged 15 commits into from
Dec 20, 2018
Merged

Authorize orgs against different URLs (production, sandbox, or custom) #818

merged 15 commits into from
Dec 20, 2018

Conversation

brpowell
Copy link
Contributor

@brpowell brpowell commented Dec 14, 2018 •
edited by lcampos
Loading

What does this PR do?

When users authorize an org, they now choose to log-in against production, sandbox, or enter a custom URL.

What issues does this PR fix or reference?

W-5459248, #610

ruthemmanuelle
ruthemmanuelle approved these changes Dec 14, 2018
View reviewed changes
Copy link
Contributor

@ruthemmanuelle ruthemmanuelle left a comment

Choose a reason for hiding this comment

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

This is based on the new message in i18n.ts. I didn't review the code.

@codecov
Copy link

codecov bot commented Dec 14, 2018 •
edited
Loading

Codecov Report

Merging #818 into develop will increase coverage by 0.1%.
The diff coverage is 91.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #818     +/-   ##
==========================================
+ Coverage    73.17%   73.28%   +0.1%     
==========================================
  Files          174      174             
  Lines         6997     7037     +40     
  Branches      1094     1102      +8     
==========================================
+ Hits          5120     5157     +37     
- Misses        1599     1602      +3     
  Partials       278      278
Impacted Files Coverage Δ
...cedx-vscode-core/src/util/sfdxProjectJsonParser.ts 93.1% <100%> (+0.79%) ⬆️
...cedx-vscode-core/src/commands/forceAuthWebLogin.ts 74.71% <90.47%> (+10.71%) ⬆️
...forcedx-vscode-core/src/channels/channelService.ts 85.71% <0%> (+2.38%) ⬆️

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 a7385ab...13f0643. Read the comment docs.

ruthemmanuelle
ruthemmanuelle approved these changes Dec 19, 2018
View reviewed changes
Copy link
Contributor

@ruthemmanuelle ruthemmanuelle left a comment

Choose a reason for hiding this comment

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

This is just based on the messages. I didn't review anything else.

lcampos
lcampos reviewed Dec 19, 2018
View reviewed changes
packages/salesforcedx-vscode-core/src/commands/forceAuthWebLogin.ts
};

public static readonly validateUrl = (url: string) => {
const expr = /https?:\/\/(.*)/;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you did there, this will work for internal envs too 🕺

lcampos
lcampos reviewed Dec 19, 2018
View reviewed changes
packages/salesforcedx-utils-vscode/src/cli/sfdxProjectJsonParser.ts Outdated
public async getValue(workspacePath: string, key: string) {
const sfdxProject = await SfdxProject.resolve(workspacePath);
const sfdxProjectJson = await sfdxProject.resolveProjectConfig();
return sfdxProjectJson[key];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome. I tried to break it by deleting sfdcLoginUrl on my sfdx-project.json file and it turns out that the cli has a default for this type of scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was hoping that would be the case and it looks like it is! I'm taking advantage of it when determining to render the "Project Default" option with the org type selection.

lcampos
lcampos approved these changes Dec 19, 2018
View reviewed changes
Copy link
Contributor

@lcampos lcampos left a comment

Choose a reason for hiding this comment

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

Merge it once the issue with packaging the extensions gets fixed.

@brpowell brpowell merged commit 2d07f88 into develop Dec 20, 2018
@brpowell brpowell deleted the bpowell/authWithDifferentUrls branch December 20, 2018 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vazexqi vazexqi vazexqi left review comments

@ruthemmanuelle ruthemmanuelle ruthemmanuelle approved these changes

@lcampos lcampos lcampos approved these changes

@allileong allileong Awaiting requested review from allileong

@AnanyaJha AnanyaJha Awaiting requested review from AnanyaJha

@JimSuplizio JimSuplizio Awaiting requested review from JimSuplizio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brpowell @vazexqi @lcampos @allileong @ruthemmanuelle