Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: migrate get encrypt metadata endpoint controller to TypeScript #711

Merged
merged 6 commits into from
Nov 25, 2020

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Nov 23, 2020

Problem

Building upon #601, this PR migrates the rest of the controller into Typescript.

Solution

Features:

  • add EncryptSubSvc#getSubmissionMetadata service fn to use model static method
  • add EncryptSubSvc#getSubmissionMetadataList service fn to use model static method
  • add EncryptSubCtl#handleGetMetadata handler fn for route endpoint

Improvements:

  • Tighten (or relax depending on your POV) Joi validation on metadata retrieval endpoint
    • query.page is now optional iff query.submissionId exists, since singular submission metadata search does not require the page parameter.

Tests

Add tests for all new methods.
Note that route integration tests are not done in this PR, and will be done when route also gets refactored.

@karrui karrui requested review from mantariksh and tshuli November 23, 2020 09:21
Copy link
Contributor

@tshuli tshuli left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

1 question

): ResultAsync<SubmissionMetadata | null, DatabaseError> => {
// Early return, do not even retrieve from database.
if (!mongoose.Types.ObjectId.isValid(submissionId)) {
return okAsync(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we actually want to return 200 with an empty list if the submission ID in the query parameter is invalid? isn't that more of a 400?

Copy link
Contributor Author

@karrui karrui Nov 25, 2020

Choose a reason for hiding this comment

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

this is due to frontend sending possible invalid submissionIds due to search, and if we return 400 an error is shown to the user resulting in confusion.

returning nothing means no result found, but it's still a valid search term i guess.

We can change it if you want, just not in this PR.

Related issue opened for the debounce thingy: #603

Copy link
Contributor

Choose a reason for hiding this comment

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

ok sounds good

@karrui karrui merged commit 29e1a94 into develop Nov 25, 2020
@karrui karrui deleted the feat/encrypt-metadata-ts branch November 25, 2020 02:26
@karrui karrui mentioned this pull request Dec 1, 2020
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.

3 participants