feat: remove unnecessary saved object get call #214
Conversation
Signed-off-by: Lin Wang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## workspace #214 +/- ##
==========================================
Coverage 66.21% 66.22%
==========================================
Files 3420 3420
Lines 65755 65755
Branches 10589 10589
==========================================
+ Hits 43541 43543 +2
+ Misses 19577 19570 -7
- Partials 2637 2642 +5
Flags with carried forward coverage won't be shown. Click here to find out more. see 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| const hasAllPermission = savedObjectsGet.every((savedObject) => { | ||
| const hasPermission = this.inMemoryValidate({ | ||
| savedObject, | ||
| permissionModes, | ||
| principals, | ||
| shouldLogNotPermitted: false, | ||
| }); |
There was a problem hiding this comment.
| const hasAllPermission = savedObjectsGet.every((savedObject) => { | |
| const hasPermission = this.inMemoryValidate({ | |
| savedObject, | |
| permissionModes, | |
| principals, | |
| shouldLogNotPermitted: false, | |
| }); | |
| const hasAllPermission = savedObjectsGet.every((savedObject) => { | |
| const hasPermission = this.inMemoryValidate({ | |
| savedObject, | |
| permissionModes, | |
| principals, | |
| }); | |
| if (!hasPermission) { | |
| // do log here | |
| this.logNotPermitted(...) | |
| notPermittedSavedObjects.push(savedObject); | |
| } |
I tend to make inMemoryValidate a pure util function so it doesn't care about logging, what do you think?
There was a problem hiding this comment.
I think we need to log the not permitted validation when call inMemoryValidate directly. Is that means we need to call the logNotPermitted manually after inMemoryValidate call?
There was a problem hiding this comment.
Is that means we need to call the logNotPermitted manually after inMemoryValidate call?
Yes, just feel passingshouldLogNotPermittedtoinMemoryValidatemakes the function a bit overwhelming.
There was a problem hiding this comment.
What do you think about change inMemoryValidate accept multi saved objects? So we don't need add shouldLogNotPermitted parameter.
| ); | ||
| } | ||
|
|
||
| public inMemoryValidate({ |
There was a problem hiding this comment.
Can you put some comment on this function?
There was a problem hiding this comment.
To be more specific, can we call it validateObjectACL?
| savedObjectsOrSavedObject: | ||
| | Array<Pick<SavedObject<unknown>, 'id' | 'type' | 'workspaces' | 'permissions'>> | ||
| | Pick<SavedObject<unknown>, 'id' | 'type' | 'workspaces' | 'permissions'>, |
There was a problem hiding this comment.
| savedObjectsOrSavedObject: | |
| | Array<Pick<SavedObject<unknown>, 'id' | 'type' | 'workspaces' | 'permissions'>> | |
| | Pick<SavedObject<unknown>, 'id' | 'type' | 'workspaces' | 'permissions'>, | |
| savedObjects: Array<Pick<SavedObject<unknown>, 'id' | 'type' | 'workspaces' | 'permissions'>>, |
Can we simplify it like this?
There was a problem hiding this comment.
Is that means we only accept array as paramenter?
There was a problem hiding this comment.
Yeah, but not a big issue, just think this might make it looks simpler.
Signed-off-by: Lin Wang <[email protected]>
wanglam
changed the title
Description
Issues Resolved
Screenshot
Testing the changes
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr