-
Notifications
You must be signed in to change notification settings - Fork 0
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
the get image endpoint and cardPut with the image #45
Conversation
…llyPro into cardApi
…kBellyPro into cardRESTfulApi
For: non existent tagId, tagId of a tag that belong to the wrong user, and a request without an email
To test this on postman, you need a dataURL in your image field in the body. To do that, pick an image, go online and find a webside that do image to dataURL conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost done. Minor changes needed, but remove server/package-lock.json
. I suspect it got there because you ran npm install
inside server
.
package.json
Outdated
@@ -23,5 +23,9 @@ | |||
"server", | |||
"shared", | |||
"web" | |||
] | |||
], | |||
"dependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependencies are already added to server/package.json. They do not need to be here.
server/jest.config.js
Outdated
@@ -6,6 +6,7 @@ module.exports = { | |||
preset: 'ts-jest', | |||
testEnvironment: 'node', | |||
testPathIgnorePatterns: [ | |||
'src/__tests__/(.*\.)?helpers.[jt]sx?$' | |||
'src/__tests__/(.*\.)?helpers.[jt]sx?$', | |||
'src/__tests__/api/authenticated/imageUri.ts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helpers rule above exists so that test-exclusive helper files do not trigger test failures. Rename imageUri.ts to image.helpers.ts instead.
import { HttpStatusError } from '../../../api/HttpStatusError'; | ||
import { imageUri } from './imageUri'; | ||
|
||
describe('cardPut unit tests', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe the tests with its route name: PUT /api/card unit tests
import { User } from './auth'; | ||
import { mockRequest } from './helpers'; | ||
|
||
describe('image unit tests', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe the unit tests with its route name: /api/image unit tests
throw new HttpStatusError(404); | ||
} | ||
const imageData = cardDoc.image; | ||
if (imageData === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use == null
to relax the test so that it catches both undefined
and null
.
throw new HttpStatusError(400); | ||
} | ||
try { | ||
image = await Jimp.read(Buffer.from(image.substr(dataURIPrefix.length), 'base64')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not reuse the image variable. Store the results of the Jimp buffer in a new variable.
} catch (e) { | ||
throw new HttpStatusError(400); | ||
} | ||
image = await image.getBufferAsync(Jimp.MIME_JPEG); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, do not reuse the image variable, store the buffer in a new variable.
email: cardDoc.email, | ||
jobTitle: cardDoc.jobTitle, | ||
company: cardDoc.company, | ||
hasImage: !!cardDoc.image, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use the double negation pattern to get a boolean. Compare it against null instead: cardDoc.image != null
.
…rule in jest.config.js
Revert e5925d1 because requestLimiter is useless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one unsanitized input usage, fix that and it will be good to go.
const { cardId } = params; | ||
|
||
try { | ||
Types.ObjectId(cardId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant also saving the ObjectId into a local variable then passing it into findById
later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait but it essentially does the same thing no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mongoose will perform the conversion by itself one more time if the argument type does not match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Turns out a certain member of our team is right again, image turns out to be very easy.
Closes #23 and #30
This suffer from the same problem as #43
Note that this pull request does not address #25 and #26, which I'm hoping for someone else to do.