Regular Event のテストが失敗していたバグを修正 #8256
Conversation
app/models/regular_event.rb
Outdated
| to: Date.new(Time.current.year, 12, 31) | ||
| def scheduled_dates_within_period( | ||
| from: Time.current.to_date, | ||
| to: Time.current.to_date.next_year |
There was a problem hiding this comment.
kyokuchoさんが仰っていたように、取得する日付が"年始〜年末"で固定されているのが問題だと思ったので、取得する日付を"今日〜来年の今日"までとしました。
There was a problem hiding this comment.
@mousu-a 色々とメソッド名が変わっているのはなぜでしょうか？
toを来年にするだけで、メソッドの意味は合っているように思います。
他のすべてのメソッドについても同じです。
There was a problem hiding this comment.
@komagata
all_scheduled_datesを変えたのは、元々のメソッドが、"今年の全てのスケジュールの日付"を取得するメソッドだったたところを、"今日〜来年の今日"（指定した期間のスケジュールの日付を取得する）メソッドに変えたのでメソッド名を変えました。
feature_scheduled_datesを変えたのは、既にbootcamp内にUpcomingEventクラスがあり文脈がより伝わりやすいかなと考えたためです。
next_holding_dateを変えたのは、メソッド名を見てdateオブジェクトを返すメソッドに勘違いしそうだったため変更しました。
すみません、このissueに限った話ではないのですが、"自分が書いた部分・それ以外という分け方で考えず、コード全体としてあるべき姿に"、ということと、"そのissue内で修正していい範囲"のラインを測りかねています💦
今回のkomagataさんの質問の意図としては、既存コードで十分に読みやすい、且つこのissue内で必要なこと(バグの修正)以外するべきではない、ということでしょうか？
There was a problem hiding this comment.
"自分が書いた部分・それ以外という分け方で考えず、コード全体としてあるべき姿に"
こちらはその通りです。
今回のkomagataさんの質問の意図としては、既存コードで十分に読みやすい、且つこのissue内で必要なこと(バグの修正)以外するべきではない、ということでしょうか？
そういうわけではないです。ただ、一部は単純に変更前の方が良いと思うからでです。
all_scheduled_datesを変えたのは、元々のメソッドが、"今年の全てのスケジュールの日付"を取得するメソッドだったたところを、"今日〜来年の今日"（指定した期間のスケジュールの日付を取得する）メソッドに変えたのでメソッド名を変えました。
all_scheduled_datesには”今年”という意味は含まれていないので名前を変える必要はないと思います。名前が間違っているのではなく、実装が間違っていただけだと思います。また、in, with, within, and, orなどがメソッド名に付く場合も冗長な名前、処理になっていることが多いので注意が必要です。
There was a problem hiding this comment.
all_scheduled_datesには”今年”という意味は含まれていないので名前を変える必要はないと思います。
よく考えてみると仰る通りfrom to が変わっているだけで実装自体は変わっていませんでした🙇♂️
変数名はそのままに、from to を"今日〜来年の今日"としました。
from to をこのようにした理由としては、動作も問題なく、既存のコードであるself.fetch_participated_regular_eventsでfrom to を指定する二度手間を防げるためこのfrom to にしました。
0a00905
|
@komagata |
| @@ -148,9 +145,9 @@ def end_at_be_greater_than_start_at | |||
| errors.add(:end_at, ': イベント終了時刻はイベント開始時刻よりも後の時刻にしてください。') | |||
| end | |||
|
|
|||
| def feature_scheduled_dates | |||
| def upcoming_scheduled_dates | |||
There was a problem hiding this comment.
実装にも変更があるならいいかもですが、名前だけ変更というのは強力な理由がないとやらないかもです。
ただ、こちらは確かに元の名前より少しいいと思うのでこちらで大丈夫です。
There was a problem hiding this comment.
ありがとうございます！
こちらの変更はそのまま残しています。
88e5552
| @@ -9,7 +9,7 @@ def holding_cycles | |||
| end.join('、') | |||
| end | |||
|
|
|||
| def next_holding_date | |||
| def holding_status | |||
There was a problem hiding this comment.
ステータスが出る場合も日付が出る場合もあるので、どちらがより良いとも言えないかもです。
また、急ぎのバグ修正の場合はあまり関係ないところは弄らず、変更するとしてもリリース後にリファクタリングのPRとして作るのがいいかもです。
There was a problem hiding this comment.
すみません！こちら元に戻しました🙇♂️
88e5552
（リベースしたため履歴には残っていません）
mousu-a
force-pushed
the
bug/regular-event-test-fails
branch
from
6b3d779 to
88e5552
Compare
|
@komagata 元に戻した履歴をhistoryに残したくなかったためリベースしました。 |
app/models/regular_event.rb
Outdated
| from: Time.current.to_date, | ||
| to: Time.current.to_date.next_year |
There was a problem hiding this comment.
| from: Time.current.to_date, | |
| to: Time.current.to_date.next_year | |
| from: Date.current, | |
| to: Date.current.next_year |
でいいかもです。
There was a problem hiding this comment.
Date.currentでも同じ結果になるのは知りませんでした。修正しました！
1a9a8d3
|
@komagata
すみません、今回発生した以下の無駄な変更
この変更(コミット)をリモートに残したくなかったため、ローカルでコミット履歴を改竄しました。 |
すみません、 上のコメントにあるように、ローカルでコミット履歴を改善したので、ローカルのブランチを削除して再度リモートから |
|
@komagata |
Issue
概要
Regular Event の一部テストケースで、イベント開催日時を「第二水曜日」としており、年内にこれ以降第二水曜日が存在しない状況の時に、次回の開催日を参照する変数の中身がnilとなってしまいテストが落ちてしまっていたバグを修正しました。
変更確認方法
bug/regular-event-test-failsをローカルに取り込むgit fetch origin bug/regular-event-test-failsgit checkout bug/regular-event-test-failsrails test test/system/regular_events_test.rbrails test test/models/regular_event_test.rbScreenshot
テストが落ちるバグ対応のため、画面の変更はありません！