-
Notifications
You must be signed in to change notification settings - Fork 16.5k
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
feat(n8n AWS Cognito Node): New node #11767
base: master
Are you sure you want to change the base?
Conversation
Hey @valentina98, Thanks for the PR, We have created "GHC-455" as the internal reference to get this reviewed. One of us will be in touch if there are any changes needed, in most cases this is normally within a couple of weeks but it depends on the current workload of the team. |
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
…into node-aws-cognito
…into node-aws-cognito
…into node-aws-cognito
@@ -354,6 +354,7 @@ export class Aws implements ICredentialType { | |||
} else if (service === 'rekognition' && credentials.rekognitionEndpoint) { |
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.
Add validation to ensure service is defined and valid before constructing the endpoint string.
hints: [ | ||
{ | ||
message: 'Please select a parameter in the options to update the user', | ||
displayCondition: | ||
'={{$parameter["resource"] === "user" && $parameter["operation"] === "update" && Object.keys($parameter["additionalOptions"]).length === 0}}', | ||
whenToDisplay: 'always', | ||
location: 'outputPane', | ||
type: 'warning', | ||
}, | ||
{ |
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.
Use concise messages like "Select a parameter to update this resource" to improve clarity.
@Parvezkhan0 @netroy this node is still in a review phase, please wait for confirmation to continue. |
@@ -353,8 +353,6 @@ export class Aws implements ICredentialType { | |||
endpointString = credentials.sesEndpoint; | |||
} else if (service === 'rekognition' && credentials.rekognitionEndpoint) { | |||
endpointString = credentials.rekognitionEndpoint; | |||
} else if (service === 'sqs' && credentials.sqsEndpoint) { |
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 are these lines 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.
It was a duplicate, there was also a comment previously with this as a to-do
…into node-aws-cognito
@@ -339,7 +339,7 @@ export class Aws implements ICredentialType { | |||
region = parsed.region; | |||
} | |||
} else { | |||
if (!requestOptions.baseURL && !requestOptions.url) { | |||
if (!requestOptions.baseURL && !requestOptions.url && service) { |
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.
This check is redundant
if (!requestOptions.baseURL && !requestOptions.url && service) { | |
if (!requestOptions.baseURL && !requestOptions.url) { |
hints: [ | ||
{ | ||
message: 'Select a parameter to update this resource', | ||
displayCondition: | ||
'={{$parameter["resource"] === "user" && $parameter["operation"] === "update" && Object.keys($parameter["additionalOptions"]).length === 0}}', | ||
whenToDisplay: 'always', | ||
location: 'outputPane', | ||
type: 'warning', | ||
}, | ||
{ | ||
message: 'Select a parameter to update this resource', | ||
displayCondition: | ||
'={{$parameter["resource"] === "group" && $parameter["operation"] === "update" && Object.keys($parameter["options"]).length === 0}}', | ||
whenToDisplay: 'always', | ||
location: 'outputPane', | ||
type: 'warning', | ||
}, | ||
], |
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.
do these hints work? also there is so much duplication between the two
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.
@ShireenMissi is this good?
outputs: [NodeConnectionType.Main], | ||
hints: [ | ||
{ | ||
message: 'Select a parameter to update this resource', |
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.
@gandreini isn't this message very generic?
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.
@ShireenMissi yes too generic. Looking at the code it seems that this is fired when I update a user and send no data. We could rephrase it as Select at least one user field to update
Same for the warning below about the group
/* eslint-disable n8n-local-rules/no-uncaught-json-parse */ | ||
/* eslint-disable @typescript-eslint/no-unsafe-assignment */ | ||
/* eslint-disable @typescript-eslint/no-unsafe-member-access */ |
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 don't disable rules like this
/* eslint-disable n8n-local-rules/no-uncaught-json-parse */ | |
/* eslint-disable @typescript-eslint/no-unsafe-assignment */ | |
/* eslint-disable @typescript-eslint/no-unsafe-member-access */ |
}); | ||
} | ||
} | ||
} |
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.
the error mapping can be simplified and extracted
for example you could use
const errorMapping: Record<string, Record<string, { message: string; description: string }>> = {
group: {
delete: {
message: 'The group you are deleting could not be found.',
description: 'Adjust the "Group" parameter setting to delete the group correctly.',
},
....
return data; | ||
} | ||
|
||
export async function awsRequest( |
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.
for naming consistency I suggest renaming this to
export async function awsRequest( | |
export async function makeAwsRequest( |
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.
The code in this file is complex and could be significantly simplified. It is hard to read due to excessive type casting, some of which are unnecessary. Additionally, API calls, such as checking if a user is in a group, could be optimised for better readability and efficiency.
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.
Could you please update the PR descriptions with details about the changes you made for example what Node was added and all resources/operations
Additionally on the User resource/ Update I find the attributes part hard to fill without checking the API docs
I think the standard attributes https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-settings-attributes.html these should be added as named options, and [name, value] pairs can be kept for any custom attributes
![]() ![]() |
@adina-hub Thanks for making the changes, what I meant by my comment is actually adding the following options directly so users don't have to look up the API docs to update them |
Co-authored-by: Shireen Missi <[email protected]>
@ShireenMissi the code is ready for review. |
Summary
Added new node for the AWS Cognito API.
https://docs.aws.amazon.com/cognito-user-identity-pools/latest/APIReference/Welcome.html
The following operations were implemented for the specific resources:
Resource: User Pool
Resource: User
Resource: Group