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

サインアップ機能 #1

Merged
merged 12 commits into from
Nov 13, 2024
Merged

サインアップ機能 #1

merged 12 commits into from
Nov 13, 2024

Conversation

Kotaro0722
Copy link
Collaborator

@Kotaro0722 Kotaro0722 commented Nov 9, 2024

受講生の確認事項

  • 画面をブラウザで実際に開いてテスト要件の画面と機能の動作確認をした(動作が分からない場合講師からスクリーンショットの提出を求めることがあります)
  • 作成したモデルを全てDjango管理画面に登録した
  • テスト要件のテストを全て実装した
  • CI が全て通った

1次レビュアーの確認事項

Copy link

github-actions bot commented Nov 9, 2024

black(フォーマッタ)のチェックに失敗しました。CI実行のログを確認して修正し,再度コミット・プッシュしてください。

@Kotaro0722 Kotaro0722 marked this pull request as ready for review November 10, 2024 14:37
Copy link

@harune-pg harune-pg left a comment

Choose a reason for hiding this comment

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

最終課題の最初のステップ、画面と機能が要件通り実装できていてよかったです👍
いくつか修正点がありますので、ご確認お願いします🙏


def form_valid(self, form):
response = super().form_valid(form)
print(self.object)

Choose a reason for hiding this comment

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

使っていないprint文は削除しておきましょう🗑️

tweets/views.py Outdated
@@ -1 +1,7 @@
# from django.shortcuts import render

Choose a reason for hiding this comment

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

使わなかったモジュールは消して大丈夫です!

form = response.context["form"]

self.assertEqual(response.status_code, 200)
self.assertFalse(User.objects.filter(email=invalid_data["username"]).exists())

Choose a reason for hiding this comment

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

63行目のfilteremailusernameを比較しているため、意図したチェックになっていないかもしれません。
他にも同様の箇所があるので、このファイル全体を見直してみてください!

具体的には、usernameフィールドがユニークなので、usernameでフィルターするといいです!

詳しい説明は、Wikiのサインアップ機能のページのSTEP5の04.の3. postのテストdef test_success_post(self)の詳しい説明3. self.assertTrue … の説明にあります。

Copy link

@harune-pg harune-pg left a comment

Choose a reason for hiding this comment

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

前回の3つの修正点については問題なく対応されていて、OKでした!🙌
その上で、新たに修正点が見つかったので確認をお願いします🙏

Comment on lines 95 to 97
self.assertEqual(response.status_code, 200)
self.assertFalse(form.is_valid())
self.assertIn("同じユーザー名が既に登録済みです。", form.errors["username"])

Choose a reason for hiding this comment

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

test_failure_post_with_duplicated_userメソッドで、期待する効果「DBにレコードが追加されていないこと」のチェックが抜けているようです。

self.assertFalse(form.is_valid())
self.assertIn("このフィールドは必須です。", form.errors["password1"])
self.assertIn("このフィールドは必須です。", form.errors["password2"])

Choose a reason for hiding this comment

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

【提案(必須じゃない)】
password2に変えるなら、password1でも同じformエラーが出てるので、どっちも書いちゃってもいいかもです!

Copy link

@harune-pg harune-pg left a comment

Choose a reason for hiding this comment

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

LGTM!
参考程度に、1件だけコメントをつけておきました。


self.assertEqual(response.status_code, 200)
self.assertFalse(form.is_valid())
self.assertEqual(User.objects.filter(username=duplicated_data["username"]).count(), 1)

Choose a reason for hiding this comment

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

【提案(必須じゃない)】
やっていることは変わりませんが、valid_data["username"]を使った方が意味的に自然かと思います。こんな感じで修正してみてはいかがでしょうか?

Suggested change
self.assertEqual(User.objects.filter(username=duplicated_data["username"]).count(), 1)
self.assertEqual(User.objects.filter(username=valid_data["username"]).count(), 1)

Copy link
Member

@TAK848 TAK848 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Kotaro0722 Kotaro0722 merged commit 5cce3f2 into main Nov 13, 2024
2 checks passed
@Kotaro0722 Kotaro0722 deleted the feature/signup branch November 13, 2024 06:27
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