refactor(core): Consolidate `CredentialsService.getMany()` (no-changelog) #7028

ivov · 2023-08-28T10:02:44Z

Consolidate CredentialsService.getMany() in preparation for adding list query middleware to GET /credentials.

codecov · 2023-08-28T14:11:57Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (9dd5f0e) 32.11% compared to head (90ed65e) 32.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7028      +/-   ##
==========================================
- Coverage   32.11%   32.11%   -0.01%     
==========================================
  Files        3188     3188              
  Lines      195892   195897       +5     
  Branches    21363    21362       -1     
==========================================
  Hits        62908    62908              
- Misses     131951   131957       +6     
+ Partials     1033     1032       -1

Files Changed	Coverage Δ
packages/cli/src/WorkflowHelpers.ts	`50.25% <ø> (ø)`
...ages/cli/src/credentials/credentials.service.ee.ts	`93.93% <ø> (+0.75%)`	⬆️
packages/cli/src/requests.ts	`100.00% <ø> (ø)`
...s/cli/src/credentials/credentials.controller.ee.ts	`78.08% <100.00%> (+0.58%)`	⬆️
...ages/cli/src/credentials/credentials.controller.ts	`85.71% <100.00%> (ø)`
...ackages/cli/src/credentials/credentials.service.ts	`77.30% <100.00%> (+2.11%)`	⬆️
packages/cli/src/services/ownership.service.ts	`85.29% <100.00%> (+7.03%)`	⬆️
...kages/cli/src/workflows/workflows.controller.ee.ts	`77.04% <100.00%> (ø)`
...ackages/cli/src/workflows/workflows.services.ee.ts	`82.95% <100.00%> (ø)`

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2023-08-28T14:14:30Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

krynble

Overall it looks great, I think there are some small improvements that can be made.

krynble · 2023-09-01T11:44:27Z

packages/cli/src/credentials/credentials.service.ts

+			Container.get(OwnershipService).addOwnedByAndSharedWith(c);
+
+		if (returnAll) {
+			const credentials = await Db.collections.Credentials.find({ select, relations });


Suggested change

const credentials = await Db.collections.Credentials.find({ select, relations });

const credentials = await Container.get(CredentialsRepository).find({ select, relations });

We can try to remove references to Db.collections.Credentials and use the repository instead wdyt?

My reasoning for why this wasn't worth it is that, from what I understand, the root issue here is that CredentialsService (and many others like it) do not currently allow for dependency injection. This means, I can add calls to Container.get() but we'd have to touch this again when we refactor this properly, so it seems not worth it at this time.

krynble · 2023-09-01T11:44:52Z

packages/cli/src/credentials/credentials.service.ts

@@ -24,6 +24,7 @@ import type { User } from '@db/entities/User';
 import type { CredentialRequest } from '@/requests';
 import { CredentialTypes } from '@/CredentialTypes';
 import { RoleService } from '@/services/role.service';
+import { OwnershipService } from '@/services/ownership.service';


Suggested change

import { OwnershipService } from '@/services/ownership.service';

import { OwnershipService } from '@/services/ownership.service';

import { CredentialsRepository } from '@/databases/repositories';

Sorry, what do you mean? I don't see CredentialsRepository used in this file?

krynble · 2023-09-01T11:47:16Z

packages/cli/src/services/ownership.service.ts

@@ -50,4 +51,25 @@ export class OwnershipService {
 			ownedBy: ownerId ? { id: ownerId } : null,
 		});
 	}
+
+	addOwnedByAndSharedWith(_credential: CredentialsEntity): Credentials.WithOwnedByAndSharedWith {


Suggested change

addOwnedByAndSharedWith(_credential: CredentialsEntity): Credentials.WithOwnedByAndSharedWith {

addOwnedByAndSharedWithToCredentials(_credential: CredentialsEntity): Credentials.WithOwnedByAndSharedWith {

Simply because we might do the same for workflows, right? We have the addOwnedBy above that could be renamed to contain also clarification about what it applies to (maybe out of context of this PR)

Also can we add some unit tests covering this function?

Simply because we might do the same for workflows, right? We have the addOwnedBy above that could be renamed to contain also clarification about what it applies to (maybe out of context of this PR)

I think this is clear from the typing? Different entities require different fields, and the caller cannot misuse the methods - the typing will block them if they try to.

Also can we add some unit tests covering this function?

Will do! Edit: Done!

krynble · 2023-09-01T11:50:13Z

packages/cli/src/credentials/credentials.service.ts

-				select: SELECT_FIELDS,
-				relations: options?.relations,
-			});
+	static async getMany(user: User, options?: { disableGlobalRole: boolean }) {


There's a good bit of logic in this function, can we add some tests to it? We need to assert that the repository is called with the correct relations, select dolumns and the correct query is performed depending on the user types and arguments.

This is covered in the followup: https://github.com/n8n-io/n8n/pull/7041/files#diff-39f41209012fda27b308597ac9471c22beab53d2d0975bc0bb87e91b597c7c94

krynble

LGTM! Thanks

github-actions · 2023-09-01T14:55:51Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

cypress · 2023-09-01T14:55:57Z

Passing run #2069 ↗︎

0	238	0	0	0

Details:

🌳 pay-647 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project: n8n	Commit: `90ed65e28f`
Status: Passed	Duration: 08:27 💡
Started: Sep 4, 2023 8:28 AM	Ended: Sep 4, 2023 8:36 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

github-actions · 2023-09-01T16:00:50Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

github-actions · 2023-09-04T08:36:47Z

✅ All Cypress E2E specs passed

janober · 2023-09-06T12:25:21Z

Got released with n8n@1.6.0

ivov added 21 commits August 23, 2023 11:45

feat(core): Add filtering, selection and pagination to users

ea1a3c8

Complete implementation

936a14c

De-duplicate validation

c509bc3

Cleanup

be06c1f

Consolidate feature flags

d4506af

More deduplication

28c3cf1

Readability

294c890

Remove unrelated change

61eb6ba

Merge master, resolve conflicts

ac4c694

Fix MFA issue

d242209

Fix test

954b97d

Fix pending tests

b7e8eb3

Support firstName and lastName filters

160daa9

Fix combination of options requiring auxiliary fields

8ae8522

Merge master, resolve conflicts

87e8d00

Fix take without selection

07b6fcd

Remove early return

9fb9cb5

Fix lint

22c8684

Cleanup

ed185c5

Add clarifying comment

460b591

Initial setup

c023c83

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Aug 28, 2023

ivov added 6 commits August 28, 2023 15:19

Fix build

b8e7253

Cleanup

d763fb4

Fix credentials.test.ts

a078178

Fix workflows.controller.ee.test.ts

e58f46e

Remove logging

bdf9ff0

Cleanup

89f72b3

ivov changed the title ~~feat(core): Add filtering, selection and pagination to credentials~~ refactor(core): Consolidate CredentialsService.getMany() (no-changelog) Aug 28, 2023

Base automatically changed from pay-646-allow-filtering-and-querying-for-users to master August 28, 2023 14:13

Add missing param

69f57ba

ivov added 3 commits August 28, 2023 16:14

Merge branch 'master' into pay-647

5a875f4

Move method

f1cd686

Reference base service

e154504

ivov marked this pull request as ready for review August 28, 2023 14:40

ivov added 4 commits August 28, 2023 16:47

Skip test causing timeouts

b8af009

Remove duplication

802c2af

Improve typing

e805ee1

Simplify typings

006f2ff

ivov mentioned this pull request Aug 29, 2023

feat(core): Add list query middleware to credentials #7041

Merged

krynble reviewed Sep 1, 2023

View reviewed changes

ivov added 3 commits September 1, 2023 15:56

Merge branch 'master' into pay-647

b7fad9c

Fix lint

23af1ad

Unit tests for addOwnedByAndSharedWith()

a3d282c

krynble approved these changes Sep 1, 2023

View reviewed changes

Merge branch 'master' into pay-647

90ed65e

krynble approved these changes Sep 4, 2023

View reviewed changes

ivov merged commit 442b910 into master Sep 4, 2023
18 checks passed

ivov deleted the pay-647 branch September 4, 2023 08:37

janober added the Released label Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Consolidate `CredentialsService.getMany()` (no-changelog) #7028

refactor(core): Consolidate `CredentialsService.getMany()` (no-changelog) #7028

ivov commented Aug 28, 2023 •

edited

Loading

codecov bot commented Aug 28, 2023 •

edited

Loading

github-actions bot commented Aug 28, 2023

krynble left a comment

krynble Sep 1, 2023

krynble Sep 1, 2023

ivov Sep 1, 2023

krynble Sep 1, 2023

ivov Sep 1, 2023

krynble Sep 1, 2023

krynble Sep 1, 2023

ivov Sep 1, 2023 •

edited

Loading

krynble Sep 1, 2023

ivov Sep 1, 2023

krynble left a comment

github-actions bot commented Sep 1, 2023

cypress bot commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023

github-actions bot commented Sep 4, 2023

janober commented Sep 6, 2023

	const credentials = await Db.collections.Credentials.find({ select, relations });
	const credentials = await Container.get(CredentialsRepository).find({ select, relations });

	import { OwnershipService } from '@/services/ownership.service';
	import { OwnershipService } from '@/services/ownership.service';
	import { CredentialsRepository } from '@/databases/repositories';

	addOwnedByAndSharedWith(_credential: CredentialsEntity): Credentials.WithOwnedByAndSharedWith {
	addOwnedByAndSharedWithToCredentials(_credential: CredentialsEntity): Credentials.WithOwnedByAndSharedWith {

refactor(core): Consolidate CredentialsService.getMany() (no-changelog) #7028

refactor(core): Consolidate CredentialsService.getMany() (no-changelog) #7028

Conversation

ivov commented Aug 28, 2023 • edited Loading

codecov bot commented Aug 28, 2023 • edited Loading

Codecov Report

github-actions bot commented Aug 28, 2023

krynble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krynble left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 1, 2023

cypress bot commented Sep 1, 2023 • edited Loading

Passing run #2069 ↗︎

github-actions bot commented Sep 1, 2023

github-actions bot commented Sep 4, 2023

janober commented Sep 6, 2023

refactor(core): Consolidate `CredentialsService.getMany()` (no-changelog) #7028

refactor(core): Consolidate `CredentialsService.getMany()` (no-changelog) #7028

ivov commented Aug 28, 2023 •

edited

Loading

codecov bot commented Aug 28, 2023 •

edited

Loading

ivov Sep 1, 2023 •

edited

Loading

cypress bot commented Sep 1, 2023 •

edited

Loading