Task/use audit info in events #6872
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 10 findings(s) 🚩
- Improving Code Health: 54 findings(s) ✅
- Affected Hotspots: 4 files(s) 🔥
View detailed results in CodeScene
Absence of Expected Change Pattern
- unleash/src/test/e2e/api/admin/event.e2e.test.ts is usually changed with: unleash/src/test/e2e/health.e2e.test.ts
| @@ -7,10 +7,14 @@ import type { | |||
| } from './dependent-features'; | |||
| import type { IDependentFeaturesReadModel } from './dependent-features-read-model-type'; | |||
| import type { EventService } from '../../services'; | |||
| import type { IUser } from '../../server-impl'; | |||
| import { SKIP_CHANGE_REQUEST } from '../../types'; | |||
| import type { IAuditUser, IUser } from '../../server-impl'; | |||
There was a problem hiding this comment.
✅ No longer an issue: Primitive Obsession
The ratio of primivite types in function arguments is no longer above the threshold
| @@ -7,10 +7,14 @@ import type { | |||
| } from './dependent-features'; | |||
| import type { IDependentFeaturesReadModel } from './dependent-features-read-model-type'; | |||
| import type { EventService } from '../../services'; | |||
| import type { IUser } from '../../server-impl'; | |||
| import { SKIP_CHANGE_REQUEST } from '../../types'; | |||
| import type { IAuditUser, IUser } from '../../server-impl'; | |||
There was a problem hiding this comment.
✅ No longer an issue: String Heavy Function Arguments
The ratio of strings in function arguments is no longer above the threshold
| @@ -1,7 +1,6 @@ | |||
| import { | |||
| CREATE_FEATURE_STRATEGY, | |||
| EnvironmentVariantEvent, | |||
| FEATURE_UPDATED, | |||
There was a problem hiding this comment.
✅ Getting better: Lines of Code in a Single File
The lines of code decreases from 1984 to 1941, improve code health by reducing it to 1000
| @@ -1,7 +1,6 @@ | |||
| import { | |||
| CREATE_FEATURE_STRATEGY, | |||
| EnvironmentVariantEvent, | |||
| FEATURE_UPDATED, | |||
There was a problem hiding this comment.
✅ Getting better: String Heavy Function Arguments
The ratio of strings in function arguments decreases from 56.56% to 47.19%, threshold = 39.0%
| @@ -1298,8 +1283,7 @@ class FeatureToggleService { | |||
| featureName: string, | |||
| projectId: string, | |||
| newFeatureName: string, | |||
| userName: string, | |||
| userId: number, | |||
| auditUser: IAuditUser, | |||
There was a problem hiding this comment.
✅ Getting better: Large Method
FeatureToggleService.cloneFeatureToggle decreases from 78 to 75 lines of code, threshold = 70
| const preHook = ( | ||
| app, | ||
| config, | ||
| { | ||
| userService, | ||
| accessService, | ||
| }: Pick<IUnleashServices, 'userService' | 'accessService'>, | ||
| ) => { |
There was a problem hiding this comment.
❌ Getting worse: Large Method
'READ_FRONTEND_API_TOKEN should be able to see FRONTEND tokens' increases from 72 to 81 lines of code, threshold = 70
| const preHook = ( | ||
| app, | ||
| config, | ||
| { | ||
| userService, | ||
| accessService, | ||
| }: Pick<IUnleashServices, 'userService' | 'accessService'>, | ||
| ) => { |
There was a problem hiding this comment.
❌ New issue: Large Method
'READ_CLIENT_API_TOKEN should be able to see CLIENT tokens' has 78 lines, threshold = 70
| userName, | ||
| userId, |
There was a problem hiding this comment.
✅ No longer an issue: Large Method
'Roundtrip with strategies in multiple environments works' is no longer above the threshold for lines of code
| IUnleashStores, | ||
| IUser, | ||
| IUserAccessOverview, | ||
| import { |
There was a problem hiding this comment.
ℹ Getting worse: Lines of Code in a Single File
The lines of code increases from 1479 to 1505, improve code health by reducing it to 1000
| await projectService.createProject( | ||
| projectForCreate, | ||
| editorUser, | ||
| TEST_AUDIT_USER, | ||
| ); | ||
| await projectService.createProject( | ||
| projectForDelete, | ||
| editorUser, | ||
| TEST_AUDIT_USER, | ||
| ); |
There was a problem hiding this comment.
❌ Getting worse: Large Method
'Should allow user to take multiple group roles and have expected permissions on each project' increases from 73 to 81 lines of code, threshold = 70
chriswk
force-pushed
the
task/useAuditInfoInEvents
branch
from
a302760
to
7dccbf7
Compare
There was a problem hiding this comment.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 20 findings(s) 🚩
- Improving Code Health: 55 findings(s) ✅
- Affected Hotspots: 4 files(s) 🔥
View detailed results in CodeScene
Absence of Expected Change Pattern
- unleash/src/test/e2e/api/admin/event.e2e.test.ts is usually changed with: unleash/src/test/e2e/health.e2e.test.ts
chriswk
force-pushed
the
task/addUserAuditInfo
branch
from
d7123b0
to
62ec318
Compare
| } | ||
|
|
||
| async deleteFeatureDependency( | ||
| dependency: FeatureDependencyId, | ||
| projectId: string, | ||
| user: IUser, | ||
| auditUser: IAuditUser, |
There was a problem hiding this comment.
For the future, we can consider merging auditUser with user as they should always represent the same, but user has permissions and auditUser don't, but for now, let's do this as a gradual improvement.
There was a problem hiding this comment.
yes, agreed, the thing missing was having some way from audit user to UserWithPermission. so the one thing audituser has that most other user implementations do not have is the ip.
| expect(audit!.id).toBe(-1); | ||
| expect(audit!.username).toBe('unknown'); |
There was a problem hiding this comment.
Just a note: this comes from new NoAuthUser().id and new NoAuthUser().username which is injected by the noAuthentication function above
| if (!req.user) { | ||
| logger.info('Could not find user'); | ||
| } else { |
There was a problem hiding this comment.
Do we want to log everytime we don't find a user?
| if (!req.user) { | |
| logger.info('Could not find user'); | |
| } else { | |
| if (req.user) { |
There was a problem hiding this comment.
Since this middleware is only mounted on /api/admin not finding a user might actually be an error.
chriswk
force-pushed
the
task/useAuditInfoInEvents
branch
from
7dccbf7
to
61b6ceb
Compare
There was a problem hiding this comment.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 9 findings(s) 🚩
- Improving Code Health: 54 findings(s) ✅
- Affected Hotspots: 4 files(s) 🔥
View detailed results in CodeScene
Absence of Expected Change Pattern
- unleash/src/test/e2e/api/admin/event.e2e.test.ts is usually changed with: unleash/src/test/e2e/health.e2e.test.ts
| @@ -1,6 +1,6 @@ | |||
| import { subDays } from 'date-fns'; | |||
| import { ValidationError } from 'joi'; | |||
| import type { IUser } from '../../types/user'; | |||
| import type { IAuditUser, IUser } from '../../types/user'; | |||
There was a problem hiding this comment.
✅ Getting better: Lines of Code in a Single File
The lines of code decreases from 1076 to 1054, improve code health by reducing it to 1000
| @@ -1,6 +1,6 @@ | |||
| import { subDays } from 'date-fns'; | |||
| import { ValidationError } from 'joi'; | |||
| import type { IUser } from '../../types/user'; | |||
| import type { IAuditUser, IUser } from '../../types/user'; | |||
There was a problem hiding this comment.
✅ Getting better: Primitive Obsession
The ratio of primitive types in function arguments decreases from 78.99% to 64.55%, threshold = 30.0%
There was a problem hiding this comment.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 9 findings(s) 🚩
- Improving Code Health: 54 findings(s) ✅
- Affected Hotspots: 4 files(s) 🔥
View detailed results in CodeScene
Absence of Expected Change Pattern
- unleash/src/test/e2e/api/admin/event.e2e.test.ts is usually changed with: unleash/src/test/e2e/health.e2e.test.ts
| @@ -18,14 +18,17 @@ import { | |||
| createProjectService, | |||
| } from '../index'; | |||
| import { | |||
| type IAuditUser, | |||
There was a problem hiding this comment.
ℹ Getting worse: Lines of Code in a Single File
The lines of code increases from 2138 to 2164, improve code health by reducing it to 1000
There was a problem hiding this comment.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 9 findings(s) 🚩
- Improving Code Health: 54 findings(s) ✅
- Affected Hotspots: 4 files(s) 🔥
View detailed results in CodeScene
Absence of Expected Change Pattern
- unleash/src/test/e2e/api/admin/event.e2e.test.ts is usually changed with: unleash/src/test/e2e/health.e2e.test.ts
There was a problem hiding this comment.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 9 findings(s) 🚩
- Improving Code Health: 54 findings(s) ✅
- Affected Hotspots: 4 files(s) 🔥
View detailed results in CodeScene
Absence of Expected Change Pattern
- unleash/src/test/e2e/api/admin/event.e2e.test.ts is usually changed with: unleash/src/test/e2e/health.e2e.test.ts
There was a problem hiding this comment.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 9 findings(s) 🚩
- Improving Code Health: 54 findings(s) ✅
- Affected Hotspots: 4 files(s) 🔥
View detailed results in CodeScene
Absence of Expected Change Pattern
- unleash/src/test/e2e/api/admin/event.e2e.test.ts is usually changed with: unleash/src/test/e2e/health.e2e.test.ts
There was a problem hiding this comment.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 9 findings(s) 🚩
- Improving Code Health: 54 findings(s) ✅
- Affected Hotspots: 4 files(s) 🔥
View detailed results in CodeScene
Absence of Expected Change Pattern
- unleash/src/test/e2e/api/admin/event.e2e.test.ts is usually changed with: unleash/src/test/e2e/health.e2e.test.ts
So, this is the extension to the auditInfo middleware.
I've tried to use/add the audit info to all events I could see/find. This makes this PR necessarily huge, because we do store quite a few events here and there. I realise it might not be complete yet, but tests run green, and I think we now have a pattern to follow for other events. Merging this will probably break enterprise quite hard, so will prepare a PR on enterprise tomorrow which is linked against this.