-
Notifications
You must be signed in to change notification settings - Fork 72
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
活動時間カレンダーの実装 #8263
base: main
Are you sure you want to change the base?
活動時間カレンダーの実装 #8263
Conversation
c74d36a
to
baa6fb9
Compare
お疲れ様です! |
@Judeeeee |
@machida |
@machida |
お疲れ様です! |
@Judeeeee |
@hagiya0121 |
db/fixtures/learning_time_frames.yml
Outdated
@@ -0,0 +1,11 @@ | |||
# Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html |
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.
このコメントは不要だと思います。テストの方のfixtureにも書いてありました。
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.
@hagiya0121
コメントありがとうございます!
5330637でコメントを削除しました🙆♀️
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.
@Judeeeee
動作に問題はありませんでした。
気になったところをコメントしたので確認お願いします🙏
db/fixtures/learning_time_frames.yml
Outdated
<% week_days = { '日' => 'sun', '月' => 'mon', '火' => 'tue', '水' => 'wed', '木' => 'thu', '金' => 'fri', '土' => 'sat' } %> | ||
<% week_days.each_with_index do |(day_name, day_prefix), day_index| %> | ||
<% (0..23).each_with_index do |hour, hour_index| %> | ||
<%= "#{day_prefix}_#{hour}:" %> | ||
id: <%= day_index * 24 + hour_index + 1 %> | ||
week_day: <%= day_name.inspect %> | ||
activity_time: <%= hour %> | ||
<% end %> |
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.
week_days
をハッシュにしてるのはなぜでしょうか?ハッシュのキーか値のどちらかを配列にしても同じことができそうですが🤔day_prefix
で分かりやすいフィクスチャ名を付けるためでしょうか?day_name.inspect
としていますがinspect
はいらないと思いました。
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.
day_prefixで分かりやすいフィクスチャ名を付けるためでしょうか?
はい、おっしゃる通りです〜
他のfixtureファイルに倣ってfixture名を作成する意図でした。
(厳密には他ファイルで名詞+数字の形なので、このdiscussion後に修正します。)
私が期待しているfixtureのイメージは以下です。
sun1:
id: 1
week_day: 日
activity_time: 5
話は変わって、カレンダーを表示させる際に上記week_dayで定義されている日付を使わずにビュー側で日付の配列をeachで回しているからweek_day自体が不要では?という問いに対しては、今後の実装を考慮した結果になります。
現段階では使用していないのですが、今後ペアプロマッチング機能実装を予定しており必要なことが想定されているため、現在のカラム構成になっています。
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.
inspectの削除はこちらで行いました〜
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.
なるほど理解しました🙏
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.
(厳密には他ファイルで名詞+数字の形なので、このdiscussion後に修正します。)
こちら、daac3afで対応しました〜
public/images/background/people.png
Outdated
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.
このファイルは何のために追加されたのでしょうか?
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.
@machida
お疲れ様です!
この画像ファイルはデザイン入れる際に誤ってcommitしてしまったものですかね?👀
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.
誤ってcommitしてしまったものとのことだったので、32ee275で削除しました!
def update_learning_time_frames | ||
learning_time_frame_ids = params[:user][:learning_time_frame_ids] | ||
learning_time_frames_users = @user.learning_time_frames_users | ||
learning_time_frames_users.delete_all | ||
|
||
return if learning_time_frame_ids.blank? | ||
|
||
learning_time_frame_ids.each do |learning_time_frame_id| | ||
learning_time_frames_users.create!(user_id: @user.id, learning_time_frame_id:) | ||
end | ||
end |
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.
中間テーブルの更新は、このメゾッドを使わなくてもuser_attribute
に{ learning_time_frame_ids: [] }
を追加するだけでいいと思います。
注意点としてこの方法だとチェックボックスを全て外した際にlearning_time_frame_ids
がnil
になってしまって更新されないので、対策したほうがいいです。
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.
こちらのcommitで修正しました🙆♀️
全てチェックを外した状態で更新させるために、hidden_field_tag
を用いてデフォルト値を送る形にしました。
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.
= hidden_field_tag 'user[learning_time_frame_ids][]', '', id: nil
引数のid: nill
は省略していいと思います
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.
41149cbで削除しました👌
app/controllers/home_controller.rb
Outdated
@@ -33,7 +33,8 @@ def set_required_fields | |||
discord_account_name: current_user.discord_profile.account_name, | |||
github_account: current_user.github_account, | |||
blog_url: current_user.blog_url, | |||
graduated: current_user.graduated? | |||
graduated: current_user.graduated?, | |||
learning_time_frames: current_user.graduated? || current_user.learning_time_frames.present? |
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.
レコードの存在を確認するだけならpresent?
よりもexists?
の方がいいと思います。
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.
こちらで修正しました🙆
他ファイルでも同様の箇所があったため、合わせて修正しました。
app/views/users/show.html.slim
Outdated
@@ -31,10 +31,10 @@ | |||
p | |||
| このページは他のユーザーから見た、あなたのプロフィールページです。 | |||
| ( #{link_to '登録情報変更', edit_current_user_path} ) | |||
.container.is-xl | |||
.container(class="#{!@user.graduated? && @user.learning_time_frames.present? ? 'is-xxl' : 'is-xl'}") |
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.
!@user.graduated? && @user.learning_time_frames.present?
が繰り返し使われているのが気になります。ヘルパーメゾッドにした方がいいと思います。
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.
こちらでヘルパーメソッドにしました!
test/system/current_user_test.rb
Outdated
@@ -247,4 +247,48 @@ class CurrentUserTest < ApplicationSystemTestCase | |||
assert_no_text 'Rubyの経験あり' | |||
assert_text 'JavaScriptの経験あり' | |||
end | |||
|
|||
test 'visible learning time framestable for non advisors and grad users' do |
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.
テスト名がframestable
となっています。frames table
ではないでしょうか。
他にもframetable
になっている箇所がありました。
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.
こちらで修正しました!
visit_with_auth '/current_user/edit', 'kimura' | ||
assert_selector 'h1.auth-form__title', text: '登録情報変更' | ||
assert_selector 'label.a-form-label', text: '主な活動予定時間' | ||
|
||
visit_with_auth '/current_user/edit', 'mentormentaro' | ||
assert_selector 'h1.auth-form__title', text: '登録情報変更' | ||
assert_selector 'label.a-form-label', text: '主な活動予定時間' | ||
|
||
visit_with_auth '/current_user/edit', 'kensyu' | ||
assert_selector 'h1.auth-form__title', text: '登録情報変更' | ||
assert_selector 'label.a-form-label', text: '主な活動予定時間' | ||
|
||
visit_with_auth '/current_user/edit', 'advijirou' | ||
assert_selector 'h1.auth-form__title', text: '登録情報変更' | ||
assert_no_selector 'label.a-form-label', text: '主な活動予定時間' | ||
|
||
visit_with_auth '/current_user/edit', 'sotugyou' | ||
assert_selector 'h1.auth-form__title', text: '登録情報変更' | ||
assert_no_selector 'label.a-form-label', text: '主な活動予定時間' | ||
end |
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.
主な活動予定時間
が表示される人とされない人でテストを分けてもいいんじゃないかなと思いました。
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.
システムテストは極めて重いので、あまり追加したくないという背景があります。
そのため、「存在する」ことのみを確認するシステムテストとしました🙆
test/system/current_user_test.rb
Outdated
assert_selector 'h1.page-main-header__title', text: 'プロフィール' | ||
assert_selector 'h2.card-header__title', text: '主な活動予定時間' | ||
|
||
within('tbody#learning_time_frame') do |
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.
なぜwithin
でスコープを限定しているのでしょうか?
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.
なぜwithinでスコープを限定しているのでしょうか?
これはデザインを当てていただいた後の修正で、試行錯誤した跡が残ってしまっているだけです💦
セレクタ自体が一意になっており、スコープを絞る必要がないので削除しました。
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.
セレクタ自体が一意になっており、スコープを絞る必要がないので削除しました。
削除されていないようです🤔
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.
すみません、83f1198で削除しました💦
5330637
to
e8b5852
Compare
c752144
to
736ee0f
Compare
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.
丁寧なレビューありがとうございました!勉強になります🙏
いただいたコメントに返信しているので、ご確認ください🙇♀️
引き続きよろしくお願いいたします〜
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.
@Judeeeee
確認しました!
気になったところをコメントしたので確認お願いします🙏
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.
@hagiya0121
早速の確認ありがとうございます!助かります🙏
いただいたコメントに返信しているので、確認お願いいたします〜
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.
@Judeeeee
確認OKです🙏 Approveします!
@hagiya0121 |
@komagata |
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.
conflictの修正をお願いします。
issue
概要
以下の目的から、「活動時間」を登録してプロフィール画面で表示する新機能を実装しました。
仕様
1️⃣活動時間の入力
ユーザー登録情報編集ページに活動時間のチェックボックスを設置しました。
2️⃣入力を促すアナウンス
活動時間を登録していない場合、ダッシュボードで登録を促します。
3️⃣活動時間の表示
活動時間が登録してあると、プロフィールから確認できます。
![スクリーンショット 2025-01-12 17 37 31](https://private-user-images.githubusercontent.com/43412616/402326407-eca93bc0-1f8f-43d8-8ecd-b4eefb67758d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwNjkxNzYsIm5iZiI6MTczOTA2ODg3NiwicGF0aCI6Ii80MzQxMjYxNi80MDIzMjY0MDctZWNhOTNiYzAtMWY4Zi00M2Q4LThlY2QtYjRlZWZiNjc3NThkLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjA5VDAyNDExNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQ0MTEzMjU2ODdiODJjZGE2NjdkNDBjYTYxMmJhMjNiNjg3Y2E2NGRlMmU3MGQwMGI3YmYzOGRjMmQ2ODExMTkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.Wb1vjn-s9ntR_0_OG0vsJ8-ATPhAF9qohgqZGs4aYrM)
変更確認方法
1. このブランチをローカルに取り込む
2. migrationの実行とシードデータの投入
3. 活動時間カレンダーの表示確認
以下1~5を順に実施すると、余計な手戻りがなくてスムーズです🙆♀️
1. 新規登録時に「活動時間」のチェックテーブルが表示されないこと
2. 活動時間がプロフィールページに反映されること
ログインユーザーを現役生(
kimura
)、メンター(mentor
)、研修生(kensyu
)に変えて以下を実施してください。kimura
ormentor
orkensyu
でログインする3. プロフィールページに「活動時間」が反映されていること
4. ユーザー登録情報編集ページで「活動時間」が非表示なこと
卒業生、アドバイザーは活動時間が入力できません。
sotugyou
) or アドバイザー(advijirou
)でログインし、ユーザー登録情報編集ページに「活動時間」がないことを確認する5. 現役生は卒業したら非表示になる。
komagata
でログインし、1.kimura
のプロフィールページで「活動時間」が登録されていることを確認する