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

feat: Add crowd.dev node and trigger node #6082

Merged
merged 26 commits into from
Jul 3, 2023

Conversation

perseus-algol
Copy link
Contributor

@perseus-algol perseus-algol commented Apr 24, 2023

Adds new node and trigger node crowd.dev

@CLAassistant
Copy link

CLAassistant commented Apr 24, 2023

CLA assistant check
All committers have signed the CLA.

@n8n-assistant n8n-assistant bot added community Authored by a community member node/new Creation of an entirely new node labels Apr 24, 2023
@perseus-algol perseus-algol marked this pull request as draft April 25, 2023 08:40
@perseus-algol perseus-algol changed the title Added Crowd.dev Node and Trigger Node to base nodes feat(core): add crowd.dev node and trigger node Apr 25, 2023
@janober
Copy link
Member

janober commented May 8, 2023

Thanks a lot for the submission. I always thought the company is called "crowd.dev", is there a reason why credentials + nodes have the name "crowd" and not rather "crowdDev"?

It also looks like the trigger node is using the old and deprecated request-based helper requestWithAuthentication instead of the new httpRequestWithAuthentication.

@perseus-algol
Copy link
Contributor Author

@janober, Thanks for the review, the specified issues have been fixed.

@perseus-algol perseus-algol marked this pull request as ready for review June 12, 2023 13:56
@Joffcom
Copy link
Member

Joffcom commented Jun 16, 2023

Hey @perseus-algol,

Just noticed this is now ready for review, I have it on my list for Monday 👍🏻

import type { INodeType, INodeTypeDescription } from 'n8n-workflow';
import { allProperties } from './descriptions';

export class CrowdDevNode implements INodeType {
Copy link
Member

Choose a reason for hiding this comment

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

Should not be "crowdDevNode" just "crowdDev". Is obviously the same for the name, defaults, context file, ...

@@ -0,0 +1,18 @@
{
"node": "n8n-nodes-base.crowdDev",
Copy link
Member

Choose a reason for hiding this comment

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

That has to actually match with the name of the node. So is wrong right now, but will be correct once the node name got fied.

@perseus-algol
Copy link
Contributor Author

perseus-algol commented Jun 17, 2023

I was made specified renames.
I am not quite sure about an option for self signed certificates. Did you meant to add this one?

{
  displayName: 'Ignore SSL Issues',
  name: 'allowUnauthorizedCerts',
  type: 'boolean',
  default: false,
  description: 'Whether to connect even if SSL certificate validation is not possible',
},

@Joffcom
Copy link
Member

Joffcom commented Jun 17, 2023

@perseus-algol that is the one, then in the node you would set the option based on that value.

@Joffcom
Copy link
Member

Joffcom commented Jun 22, 2023

@perseus-algol looking good, I will give it another proper look over tomorrow morning and see if we can can get this into next weeks release.

@Joffcom
Copy link
Member

Joffcom commented Jun 26, 2023

Hey @perseus-algol,

Just finishing up the docs side, This all looks good although one thing that confuses me as a user is Activity > Create or Update with a member.

What is this actually for?
image

I am able to add multiple key / values for a username but I don't know what those fields should actually be, I can also have multiple emails in there.

@garrrikkotua
Copy link

Hi @Joffcom

This is needed because in crowd.dev member can have a usernames at several platforms. E.g twitter and github. And here users will submit "twitter: oneusername, github: anotherusername". So we leave completely up to users, which platforms and usernames they want to include. But I believe some description can be added here, showing that it's key-value pairs of platform name (e.g twitter, github, etc) and username on this platform.

Regarding emails - it is an array. In case if a member has several emails associated with him.

Hope that helps!

@Joffcom
Copy link
Member

Joffcom commented Jun 27, 2023

@garrrikkotua That would work, Maybe calling it "platform" instead of key for the display, Would you typically see people adding all of the options at one? From what I can see in the API docs only username is required for the member object so maybe the others can be under an additional field within that section so it doesn't look so confusing.

@garrrikkotua
Copy link

Yeah renaming key into platform would be great (not sure if it's possible).

True, usually users submit only one platform. But putting it into a another section doesn't make sense. I would leave it as it is.

@Joffcom
Copy link
Member

Joffcom commented Jun 27, 2023

Hey @garrrikkotua,

By keeping it like it is and showing the optional fields it won't fit our normal UI pattern for nodes, It wouldn't need to be in a new section just moved under an additional options or extra fields button.

So right now to get to that I have press Choose for the Member then we see the 4 different member options, My assumption looking at it would be that I need to complete all of those fields which doesn't appear to be the case. We could have display name, Emails and Joined At as other options that can be added as a user needs them to keep things looking clean.

Not sure how long we have until we are locked from the 0.235.0 release but if we can work this out before then we will be able to get this out there :)

@perseus-algol
Copy link
Contributor Author

image
These fields are required. Does't that mean that they should stay as they are and shouldn't be moved into "Additional Options"?

@Joffcom
Copy link
Member

Joffcom commented Jun 27, 2023

Hey @perseus-algol,

Yeah anything required should be visible. The lock for the next release is tomorrow morning so I will do one more check of this first thing and hopefully we will be good to go.

github.com:perseus-algol/n8n into n8n-nodes-crowd
Joffcom
Joffcom previously approved these changes Jun 28, 2023
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Patch coverage: 37.93% and project coverage change: +0.08 🎉

Comparison is base (f0ab023) 28.65% compared to head (919c3ae) 28.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6082      +/-   ##
==========================================
+ Coverage   28.65%   28.74%   +0.08%     
==========================================
  Files        3000     3046      +46     
  Lines      186310   187223     +913     
  Branches    20601    20705     +104     
==========================================
+ Hits        53390    53814     +424     
- Misses     132110   132573     +463     
- Partials      810      836      +26     
Impacted Files Coverage Δ
packages/cli/src/api/e2e.api.ts 0.00% <0.00%> (ø)
packages/editor-ui/src/api/cloudPlans.ts 30.76% <0.00%> (ø)
...es/editor-ui/src/components/RunDataJsonActions.vue 52.54% <0.00%> (ø)
...ages/editor-ui/src/views/SettingsSourceControl.vue 89.24% <ø> (-0.03%) ⬇️
.../nodes-base/credentials/CrowdDevApi.credentials.ts 0.00% <0.00%> (ø)
...se/credentials/GoogleDriveOAuth2Api.credentials.ts 0.00% <ø> (ø)
...ackages/nodes-base/nodes/CrowdDev/CrowdDev.node.ts 0.00% <0.00%> (ø)
.../nodes-base/nodes/CrowdDev/CrowdDevTrigger.node.ts 0.00% <0.00%> (ø)
...ages/nodes-base/nodes/CrowdDev/GenericFunctions.ts 0.00% <0.00%> (ø)
...base/nodes/CrowdDev/descriptions/activityFields.ts 0.00% <0.00%> (ø)
... and 57 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Joffcom Joffcom changed the title feat(core): add crowd.dev node and trigger node feat: add crowd.dev node and trigger node Jun 28, 2023
@Joffcom Joffcom changed the title feat: add crowd.dev node and trigger node feat: Add crowd.dev node and trigger node Jun 28, 2023
@Joffcom
Copy link
Member

Joffcom commented Jun 28, 2023

Morning,

Looks like we have missed the 0.235.0 window by about 20 minutes so this node will be released in 0.236.0 next week.

@Joffcom Joffcom merged commit 238a78f into n8n-io:master Jul 3, 2023
MiloradFilipovic added a commit that referenced this pull request Jul 5, 2023
* master:
  fix(core): Remove typeorm patches, but still enforce transactions on every migration (#6594)
  feat(Strava Node): Add hide_from_home field in Activity Update (#5883)
  feat(Notion Node): Add option to update icon when updating a page (#5670)
  refactor: Setup node context API, and consolidate code between Webhook and Wait nodes (no-changelog) (#6464)
  fix(core): Fix migrations for MySQL/MariaDB (#6591)
  fix(editor): Show retry information in execution list only when it exists (#6587)
  fix(Strapi Node): Fix issue with pagination (#4991)
  refactor(Item Lists Node): Refactoring (#6575)
  feat: Add crowd.dev node and trigger node (#6082)
  ci: Add Github Action to enforce template to issues (#5295)
  fix(core): Route `/rest/workflows/new` correctly (no-changelog) (#6572)
  fix(Brevo Node): Rename SendInBlue node to Brevo node (#6521)
  fix(XML Node): Fix issue with not returning valid data (#6565)
  feat(HTTP Request Node): New http request generic custom auth credential (#5798)
  fix(core): Fix credentials test (#6569)
  fix(core): Ensure valid `logger` is passed to every migration (no-changelog) (#6563)
  docs: Add irreversible change warning for n8n@0.234.0 (no-changelog) (#6558)
  feat: Add various source control improvements (#6533)
  feat(Twitter Node): Node overhaul (#4788)
This was referenced Jul 5, 2023
@janober
Copy link
Member

janober commented Jul 5, 2023

Got released with n8n@0.236.0

@github-actions github-actions bot mentioned this pull request Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member node/new Creation of an entirely new node Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants