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

memoテーブル、memoモデルにuser_nameを追加する #106

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

kuri0616
Copy link
Collaborator

対応するissue

対応内容

  • メモテーブルにuser_nameカラムを追加
  • メモモデルにuser_nameの項目、バリデーションを追加
  • user_nameのテストケースを追加

@kuri0616 kuri0616 added the backend バックエンドのissues label Sep 16, 2024
@kuri0616 kuri0616 requested a review from kakeru-one September 16, 2024 14:10
@kuri0616 kuri0616 self-assigned this Sep 16, 2024
Copy link
Contributor

@kakeru-one kakeru-one left a comment

Choose a reason for hiding this comment

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

@kuri0616
コメントしました!

@@ -10,6 +10,7 @@ end
create_table 'memos', charset: 'utf8mb4', collation: 'utf8mb4_0900_ai_ci', force: :cascade do |t|
t.string 'title', null: false, comment: 'メモのタイトル'
t.text 'content', null: false, comment: 'メモの本文'
t.string 'user_name', limit: 21, null: false, comment: 'Slackのユーザー名'
Copy link
Contributor

Choose a reason for hiding this comment

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

[Must/Discussion]

投稿者の名前をuser_nameにしてしまうと、
単語的にusersテーブルのaccount_nameなのかなと思わせてしまいます。
contributorやposterといった命名が適切ではないでしょうか?

Copy link
Contributor

Choose a reason for hiding this comment

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

あと、50文字まで許容した方がいいかなと思います!
スクリーンショット 2024-09-17 13 29 46

@@ -10,6 +10,7 @@ end
create_table 'memos', charset: 'utf8mb4', collation: 'utf8mb4_0900_ai_ci', force: :cascade do |t|
t.string 'title', null: false, comment: 'メモのタイトル'
t.text 'content', null: false, comment: 'メモの本文'
t.string 'user_name', limit: 21, null: false, comment: 'Slackのユーザー名'
Copy link
Contributor

Choose a reason for hiding this comment

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

あと、50文字まで許容した方がいいかなと思います!
スクリーンショット 2024-09-17 13 29 46

Comment on lines 75 to 76
{ title: Faker::Lorem.sentence(word_count: 3), content: Faker::Lorem.paragraph(sentence_count: 5),
user_name: Faker::Name.name }
Copy link
Contributor

Choose a reason for hiding this comment

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

idiomatic/Nits

改行が恣意的なので、慣習的になるようにしましょう.

Suggested change
{ title: Faker::Lorem.sentence(word_count: 3), content: Faker::Lorem.paragraph(sentence_count: 5),
user_name: Faker::Name.name }
{ title: Faker::Lorem.sentence(word_count: 3),
content: Faker::Lorem.paragraph(sentence_count: 5),
user_name: Faker::Name.name }

Comment on lines 79 to 86
it 'valid?メソッドがfalseを返すこと' do
expect(memo).not_to be_valid
end

it 'errorsに「ユーザー名を入力してください」と格納されること' do
memo.valid?
expect(memo.errors.full_messages).to eq ['ユーザ名を入力してください']
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Performant/Simple

一つのitブロックにまとめられませんか?
他の部分に関しても同様なので、修正しましょう!
(過去のレビューから学ぶことも重要です!)

Copy link
Contributor

@kakeru-one kakeru-one left a comment

Choose a reason for hiding this comment

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

LGTM

@kuri0616 kuri0616 merged commit 75f5011 into main Sep 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend バックエンドのissues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

メモテーブルにSlackの投稿者名を入れるユーザ名のカラムを追加する
2 participants