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

[refactor]:signup_router #237

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

takuabonn
Copy link
Contributor

issue

# 230

content

This PR enhances the code structure by utilizing res.locals.reqClientData as the source for client data, removing direct access to req.body, req.query, andreq.params.

Copy link
Owner

@prasenjeet-symon prasenjeet-symon left a comment

Choose a reason for hiding this comment

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

Hey , please do not run any code formatter on the codebase , and submit the PR . It changes lots of lines and make it very difficult to review the code . Just make changes to the required lines and format only that lines of code . By hand or select only that lines and format.

@@ -153,9 +153,11 @@ router.post('/login', apiRequestAuthLoginValidator, async (req, res) => {
*/
router.post('/signup', apiRequestAuthSignupValidator, async (req, res) => {
try {
const parsedBody = emailPasswordObjectValidator.parse(req.body);
const reqClientData: IRequestAuthSignup = res.locals.reqClientData;
const parsedBody = emailPasswordObjectValidator.parse(reqClientData.body);
Copy link
Owner

Choose a reason for hiding this comment

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

Why you are validating again if already validated , remove this line

@@ -153,9 +153,11 @@ router.post('/login', apiRequestAuthLoginValidator, async (req, res) => {
*/
router.post('/signup', apiRequestAuthSignupValidator, async (req, res) => {
try {
const parsedBody = emailPasswordObjectValidator.parse(req.body);
const reqClientData: IRequestAuthSignup = res.locals.reqClientData;
const parsedBody = emailPasswordObjectValidator.parse(reqClientData.body);
const email = parsedBody.email;
Copy link
Owner

Choose a reason for hiding this comment

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

use reqClientData

@@ -153,9 +153,11 @@ router.post('/login', apiRequestAuthLoginValidator, async (req, res) => {
*/
router.post('/signup', apiRequestAuthSignupValidator, async (req, res) => {
try {
const parsedBody = emailPasswordObjectValidator.parse(req.body);
const reqClientData: IRequestAuthSignup = res.locals.reqClientData;
const parsedBody = emailPasswordObjectValidator.parse(reqClientData.body);
const email = parsedBody.email;
const password = parsedBody.password;
Copy link
Owner

Choose a reason for hiding this comment

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

use reqClientData to get password

error:'A account already exists with this email.'
}
error: 'A account already exists with this email.',
};
res.status(500).json(response);
Copy link
Owner

Choose a reason for hiding this comment

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

Status should be 409 same as response.

error: error.issues[0]?.message
}
error: error.issues[0]?.message,
};
return res.status(400).send(response);
Copy link
Owner

Choose a reason for hiding this comment

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

Put return below

}
const response: ApiResponse<null> = {
success: false,
status: 400,
Copy link
Owner

Choose a reason for hiding this comment

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

It should be 500

const response: ApiResponse<null> = {
success: false,
status: 400,
error: error,
Copy link
Owner

Choose a reason for hiding this comment

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

Do not return error object . Only "Something unexpected happened". like that

success: false,
status: 400,
error: error,
};
return res.status(400).send(response);
Copy link
Owner

Choose a reason for hiding this comment

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

Change here too ( 500 )

Copy link
Owner

@prasenjeet-symon prasenjeet-symon left a comment

Choose a reason for hiding this comment

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

Fix the required changes and submit again . Thanks

@takuabonn
Copy link
Contributor Author

必要な変更を修正して、再度送信します。ありがとう

i'm sorry.
i didn't understanded task at all.

@prasenjeet-symon
Copy link
Owner

必要な変更を修正して、再度送信します。ありがとう

i'm sorry. i didn't understanded task at all.

Okay. Is it related to English language or something else.

「わかりました。それは英語の言語に関連していますか、それとも別の何かですか。」

@takuabonn
Copy link
Contributor Author

必要な変更を修正して、再度送信します。ありがとう

ごめんなさい。タスクをまったく理解していませんでした。

わかった。それは英語か何か他のものに関係していますか。

「わかりました。それは英語の言語に関連していますか、問題別の何かですか。」

Not only is my English not good, but
I thought the try chatch block was already implemented and did not look at the status inside.
Thank you for your concern for the message in Japanese.

@takuabonn
Copy link
Contributor Author

@prasenjeet-symon

遅くなりすみません。
私が、理解していなかったことは
今回のタスクの経緯です
apiRequestAuthSignupValidatorでバリデーションをすでにかけてそのバリデーション を通った値でアクセスしたいのでres.locals.reqClientDataで書き換える必要がある、
バリデーション エラー(400)と500エラーでの分岐が必要
もしまだチャンスがあればご確認お願い致します。

238

@prasenjeet-symon
Copy link
Owner

@prasenjeet-symon

遅くなりすみません。 私が、理解していなかったことは 今回のタスクの経緯です apiRequestAuthSignupValidatorでバリデーションをすでにかけてそのバリデーション を通った値でアクセスしたいのでres.locals.reqClientDataで書き換える必要がある、 バリデーション エラー(400)と500エラーでの分岐が必要 もしまだチャンスがあればご確認お願い致します。

238

「確認してお知らせします。ありがとうございます。」

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.

2 participants