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

Feature/add signup signin #15

Merged
merged 7 commits into from Feb 19, 2019

Conversation

@t4kvmi
Copy link
Owner

t4kvmi commented Feb 18, 2019

サインアップ、サインインを追加しました。
現時点では認証機能を実装しておらずパスワードは平文保存となっています。
レビューよろしくお願いします。

);
res.status(200).json(user);
} catch (err) {
const errorCode = '23505';

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Feb 19, 2019

こちらの 23505 は何かの記事やドキュメントを参考にした感じでしょうか?

This comment has been minimized.

Copy link
@t4kvmi

t4kvmi Feb 19, 2019

Author Owner

@tsuyopon-xyz

こちらの 23505 は何かの記事やドキュメントを参考にした感じでしょうか?

はい、postgresqlのドキュメントを確認しました。
23505unique_violationだったのでそれを適用しました。
https://www.postgresql.org/docs/current/errcodes-appendix.html

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Feb 19, 2019

23505unique_violationだったのでそれを適用しました。

それであれば変数名を errorCode という汎用的な名前にするより uniqueViolationのように具体的な名前にしたほうがより、この値の内容が伝わるので良いかなと思いますmm

This comment has been minimized.

Copy link
@t4kvmi

t4kvmi Feb 19, 2019

Author Owner

@tsuyopon-xyz

それであれば変数名を errorCode という汎用的な名前にするより uniqueViolationのように具体的な名前にしたほうがより、この値の内容が伝わるので良いかなと思いますmm

おっしゃる通りです。
修正してマージします。

},
});
// compare user password and requested password
const isMatch = user.password === req.body.password;

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Feb 19, 2019

User.findOne の條件で password も一緒に含めずに、ここで isMatch として別で比較した理由は、後ほど、パスワードをハッシュ化することを想定して分けた感じでしょうか?

This comment has been minimized.

Copy link
@t4kvmi

t4kvmi Feb 19, 2019

Author Owner

@tsuyopon-xyz

User.findOne の條件で password も一緒に含めずに、ここで isMatch として別で比較した理由は、後ほど、パスワードをハッシュ化することを想定して分けた感じでしょうか?

はい、お考えいただいた通りの実装を考えていました。

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Feb 19, 2019

2点質問コメントをしました。
実装自体は問題ないので、ご確認&ご回答いただけたらそのままマージして頂いても問題ありません^^

@t4kvmi t4kvmi merged commit 22babed into develop Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.