Skip to content

Conversation

@Embotic-Wayne
Copy link
Collaborator

  • added mongodb schema for user id, type, and when its deleted (excluding duplicates)
  • new api to create, read, and delete permission requests sent by members
  • unit tests to validate their behavior

Copy link
Contributor

@charred-70 charred-70 left a comment

Choose a reason for hiding this comment

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

lgtm w tree

Copy link
Contributor

@adarshm11 adarshm11 left a comment

Choose a reason for hiding this comment

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

is that a banana

type: Schema.Types.ObjectId,
ref: 'User',
required: true,
index: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can remove this, we have the PermissionRequestSchema.index to take care of this

},
type: {
type: String,
enum: Object.keys(PermissionRequestTypes),
Copy link
Collaborator

Choose a reason for hiding this comment

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

i know they will be a 1 to 1 mapping, we will use the values when we create new data though

Suggested change
enum: Object.keys(PermissionRequestTypes),
enum: Object.values(PermissionRequestTypes),

);

// Compound unique index prevents duplicate active requests per user+type
PermissionRequestSchema.index({ userId: 1, type: 1 }, { unique: true });
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of { unique: true } can it be

{ 
    unique: true, 
    partialFilterExpression: { deletedAt: null } 
}

that way we dont index deleted records

Comment on lines 24 to 26
const populated = await PermissionRequest.findById(permissionRequest._id)
.populate('userId', 'firstName lastName email');
res.status(OK).send(populated);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can also return an empty 200, unless we want to do something with this data in the frontend after submitting

.populate('userId', 'firstName lastName email');
res.status(OK).send(populated);
} catch (error) {
if (error.code === 11000) return res.status(BAD_REQUEST).send({ error: 'Request already exists' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

we have CONFLICT instead of BAD_REQUEST

we can send that and no text back, since conflict implies something exists

}
});

router.post('/delete', async (req, res) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should admins be able to delete records, and whatever record they want?

}
});

router.get('/getAll', async (req, res) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about / returns everything, up to you

}
});

router.get('/get', async (req, res) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we consolidate both endpoints into one /

the endpoint can either take a parameter like userId, or nothing

if we didnt pass any filters in, return everything if the user is an officer or higher

if we passed in a user id, return the data if

  • the user is less than an officer and its their own user id.
  • the user is an officer or higher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants