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

EZP-25409: Make CAPI site access aware #75

Merged
merged 1 commit into from Feb 24, 2016
Merged

EZP-25409: Make CAPI site access aware #75

merged 1 commit into from Feb 24, 2016

Conversation

wizhippo
Copy link

https://jira.ez.no/browse/EZP-25409

This allows the siteacces to be passed as an option to CAPI

@@ -69,6 +71,10 @@ define(["structures/Response", "structures/Request", "structures/CAPIError"],
}
}

if (null !== this._siteAccess) {
headers = extend({}, headers, {'X-Siteaccess': this._siteAccess});
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems quite complicated, can't you just write ?

headers['X-Siteaccess'] = this._siteAcess;

Copy link
Contributor

Choose a reason for hiding this comment

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

also the same operation has to be done in notAuthorizedRequest method below

@andrerom
Copy link
Contributor

@wizhippo up for it? :)

@@ -11,11 +11,13 @@ define(["structures/Response", "structures/Request", "structures/CAPIError"],
* @param endPointUrl {String} url to REST root
* @param authenticationAgent {object} Instance of one of the AuthAgents (e.g. SessionAuthAgent, HttpBasicAuthAgent)
* @param connectionFactory {ConnectionFeatureFactory} the factory which is choosing compatible connection from connections list
* @param siteAccess {String|Null} SiteAccess to use for requests
Copy link
Contributor

Choose a reason for hiding this comment

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

you should mark the parameter as optional ie

@param [siteAccess] {String} SiteAccess to use for requests

@dpobel
Copy link
Contributor

dpobel commented Feb 23, 2016

besides the last 2 inline comments, +1
(small remark: to ease the review, we usually don't build the library in the PR and we do it after merge, this also avoid some conflicts).

@wizhippo
Copy link
Author

@dpobel thanks for the review

@andrerom
Copy link
Contributor

+1

1 similar comment
@yannickroger
Copy link
Contributor

+1

dpobel added a commit that referenced this pull request Feb 24, 2016
EZP-25409: Make CAPI site access aware
@dpobel dpobel merged commit b750a64 into ezsystems:master Feb 24, 2016
@wizhippo wizhippo deleted the EZP-25409 branch February 24, 2016 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants