Check for approved access request #1069
Conversation
| request.metadata.overview.entities.some((entity) => entities.includes(entity)), | ||
| ) | ||
| async hasApprovedAccessRequest(user: UserDoc, model: ModelDoc) { | ||
| const accessRequests = await getModelAccessRequestsForUser(user, model.id) |
There was a problem hiding this comment.
I would expect this to be a service function, not in a connector.
Also, the implementation is a little odd. I wouldn't tend to handle the edge case of no access requests. The speed increase likely isn't worth the complexity?
const accessRequests = await getModelAccessRequestsForUser(user, model.id)
return await hasApprovedAccessRequestReview(accessRequests.map((accessRequest) => accessRequest.id))If you did want to keep the check, I'd expect something like:
const accessRequests = await getModelAccessRequestsForUser(user, model.id)
if (accessRequests.length === 0) {
return false
}
const ids = accessRequests.map(request => request.id)
return hasApprovedAccessRequestReview(ids)There was a problem hiding this comment.
"I would expect this to be a service function, not in a connector." - This was mainly done because the previous implementation made the same decision to make this function in the connector rather than a service. Additionally, it's unclear to me which service this function would belong to as uses both access requests and reviews to make the decision.
"The speed increase likely isn't worth the complexity?" - The complexity to me seems very minimal considering it's a single if statement to avoid an unnecessary database query, I'd happy to reverse the if statement.
| @@ -80,6 +80,9 @@ export async function getFilesByModel(user: UserDoc, modelId: string) { | |||
|
|
|||
| export async function getFilesByIds(user: UserDoc, modelId: string, fileIds: string[]) { | |||
| const model = await getModelById(user, modelId) | |||
| if (fileIds.length === 0) { | |||
There was a problem hiding this comment.
If you're going to add edge case checks like this, they should probably be at the top of the function. Did you put it down here so that we're still checking model authorisation?
backend/src/services/v2/review.ts
Outdated
| @@ -188,6 +188,18 @@ export async function sendReviewResponseNotification(review: ReviewDoc, user: Us | |||
| } | |||
| } | |||
|
|
|||
| export async function getApprovedAccessRequestReviews(accessRequestIds: string[]) { | |||
There was a problem hiding this comment.
This isn't really getApprovedAccessRequestReviews. get implies you're returning something. Instead I'd name this checkAccessRequestsApproved.
JR40159
deleted the
feature/BAI-1132-access-requests-should-be-approved-before-providing-access-to-model-binaries
branch
No description provided.