-
Notifications
You must be signed in to change notification settings - Fork 113
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
[CEPF-1955] Added aouth implementation #428
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.
Code Sanity wise looks good to me
.map(([key, value]) => key === "redirect_uri" | ||
? `${key}=${value}` | ||
: `${key}=${encodeURIComponent(value)}`) | ||
.flatMap(([key, value]) => |
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.
why use flatMap instead of map ? can you explain that in a comment
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.
to handle values like ['read_only', 'read_write']
and map to scope[]=read_only&scope[]=read_write
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.
please take @shwatang's approval on this
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.
Discussed over call.
to handle values like ['read_only', 'read_write'] and map to scope[]=read_only&scope[]=read_write
This logic is expected. Though how will this be implemented in JS is something you folks can take a call on.
lib/oAuthTokenClient.js
Outdated
if (Object.keys(errors).length > 0) return Promise.reject(errors); | ||
return this.post({ | ||
url: '/revoke', | ||
data: params | ||
}, callback) | ||
} | ||
|
||
validateInput(inputData, schema) { |
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.
any reason why is this removed ?
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.
Added in
function validateInput(inputData, schema) { |
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.
Approving given @shwatang 's approval on usage of flatMap is there
Documentation of end points : https://docs.google.com/document/d/1JP4WAmprhFSQtqWO2VD1jcpU9o-ZEMwWzY81OH9YxV8/edit?usp=sharing
Tech Spec Doc : https://docs.google.com/document/d/1JP4WAmprhFSQtqWO2VD1jcpU9o-ZEMwWzY81OH9YxV8/edit?usp=sharing
Note :- Please follow the below points while attaching test cases document link below:
- If label
Tested
is added then test cases document URL is mandatory.- Link added should be a valid URL and accessible throughout the org.
- If the branch name contains hotfix / revert by default the BVT workflow check will pass.