Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

Updates to 401 Handling #64

Merged
merged 2 commits into from
Nov 5, 2015

Conversation

nathan-boyd
Copy link
Contributor

No description provided.

options.headers.Authorization = options.headers.Authorization || 'Bearer ' + authResponse.accessToken;

if(options.headers.Authorization) {
retry = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing options.headers.Authorization = options.headers.Authorization in this block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, nevermind, just realized how dumb that would be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on that new realization, we should probably just var retry = false at the top, and these two blocks can just become one:

if(!options.headers.Authorization) {
    options.headers.Authorization = 'Bearer ' + authResponse.accessToken;
    retry = options.retry || false;
}

@dougwilson
Copy link
Contributor

Seems good to me. @vernak2539 ?


if(!options.headers.Authorization) {
options.headers.Authorization = 'Bearer ' + authResponse.accessToken;
retry = options.retry || false;
Copy link
Contributor

Choose a reason for hiding this comment

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

So only retry if they don't provide custom auth headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

At first I wondered, but now I think it's fine. Since the auth header shouldn't really diff from the one provided (just there for an extra option), this is totally cool. 👍

@vernak2539
Copy link
Contributor

So yeah, good to go @dougwilson if you want to publish. I'll be pto for a till next week.

dougwilson added a commit that referenced this pull request Nov 5, 2015
@dougwilson dougwilson merged commit b68f503 into salesforce-marketingcloud:master Nov 5, 2015
dougwilson added a commit that referenced this pull request Nov 5, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants