-
Notifications
You must be signed in to change notification settings - Fork 138
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
Frontend integration #25
base: main
Are you sure you want to change the base?
Frontend integration #25
Conversation
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.
my comments mostly minor. great job
env.example
Outdated
|
||
APP_URL=http://localhost:4000 | ||
|
||
GITHUB_ACCOUNT_LOGIN=yourGithubLogin |
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 suggest to name AUTH_USERNAME
and add a comment, it should be your GitHub username.
I also suggest to group APP_URL
, AUTH_USERNAME
and AUTH_PASSWORD
together with a comment, they are needed to make the script work.
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.
Done
src/auth/auth.service.ts
Outdated
function encodeUserToken(user) { | ||
const { id, name, password } = user; | ||
function encodeUserToken(user: User) { | ||
const { /*id,*/ name, password } = user; |
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.
const { /*id,*/ name, password } = user; | |
const { name, password } = user; |
lets remove
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.
Done
src/order/services/order.service.ts
Outdated
@@ -1,39 +1,50 @@ | |||
import { Injectable } from '@nestjs/common'; | |||
import { v4 } from 'uuid'; | |||
|
|||
import { randomUUID } from 'crypto'; |
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.
import { randomUUID } from 'crypto'; | |
import { randomUUID } from 'node:crypto'; |
minor but I find it's more convenient and readable to prefix node imports with node:
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.
Done
src/cart/services/cart.service.ts
Outdated
id, | ||
id: randomUUID(), | ||
user_id, | ||
created_at: getDate(), |
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 wonder, why do you need to store date as string and just actual timestamp (Date.now()) ?
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.
You are right, I forgot update that
For proper work of
orders
endpoint you need also merge this PR rolling-scopes-school/nodejs-aws-shop-react#140P.S.
I don't know what was initial logic of application. So I didn't touch commented code and other stuff that application currently do not use.