machida
commented
2 months ago @MikotoMakizuru class名を変えてpushしましたー レビューに進めてくださいー
Open MikotoMakizuru opened 2 months ago
machida
commented
2 months ago @MikotoMakizuru class名を変えてpushしましたー レビューに進めてくださいー
MikotoMakizuru
commented
2 months ago @reckyy はじめまして、PR のレビューをお願いしたいのですが可能でしょうか。 よろしくお願いします。
reckyy
commented
2 months ago @MikotoMakizuru お疲れ様です。 レビューは可能です！ が、https://github.com/fjordllc/bootcamp/pull/8066#issuecomment-2356064644 にて @Shrimprin さんにも依頼されてますが、そちらは気にしなくても大丈夫ですか？ ご返信お待ちしております。
MikotoMakizuru
commented
2 months ago @reckyy 返信なかったので別の方を探しておりました。 https://github.com/fjordllc/bootcamp/pull/8066#issuecomment-2356064644 は削除しておきます。
レビューお願いいたします！🙇♂️
reckyy
commented
2 months ago @MikotoMakizuru なるほどです。承知しました！ 3日以内にはレビューさせていただきます〜。
GitHubの通知に気づかない方もいるので、急ぎで返信が欲しい場合はDiscordで連絡してる方もいます！（GitHub上でコメントした後に）
MikotoMakizuru
commented
2 months ago @reckyy
3日以内にはレビューさせていただきます〜。
ありがとうございます、よろしくお願いします。
GitHubの通知に気づかない方もいるので、急ぎで返信が欲しい場合はDiscordで連絡してる方もいます！（GitHub上でコメントした後に）
承知です、今後は Discord でも連絡してみたいと思います。
MikotoMakizuru
commented
2 months ago @reckyy
画面のサイズをディスプレイの半分にした時に、カードの幅が異なっていましたがこちら意図された変更でしょうか？
私の環境でも確認できました。ユーザ名が長いとカードの幅が広がってしまうみたいです。
ウィンドウ幅が広いと「...」で省略され、ある一定のサイズまで狭めると全て表示するみたいです。あまりよろしくない仕様かと思うので「ウィンドウ幅を狭めるとユーザ名が省略されなくなる」といった別 issue で登録するか相談してみます。（デザインの部分なので町田さんが対応されるかもしれません）
https://github.com/user-attachments/assets/27c2d2ab-41a1-4726-84e8-c5c195560d57
そもそも今回追加されたテストが必ず落ちてしまいます。。（ファイル単体でテストすると） ちょっと原因については調べれていないですが、まず
@MikotoMakizuruさんの手元では必ず通るかどうかお伺いしたいです。
私の環境では、以下の通りパスしますね。。。
$ bin/rails test test/system/home_test.rb
Running via Spring preloader in process 83815
Run options: --seed 62766........................................................ [Minitest::CI] Generating test report in JUnit XML format...
Finished in 34.673798s, 1.6151 runs/s, 3.9223 assertions/s. 56 runs, 136 assertions, 0 failures, 0 errors, 0 skips
- 今回追加したテストを指定したとき$ bin/rails test test/system/home_test.rb:325 Running via Spring preloader in process 86298 Run options: --seed 56859
. [Minitest::CI] Generating test report in JUnit XML format...
Finished in 4.294147s, 0.2329 runs/s, 2.5616 assertions/s. 1 runs, 11 assertions, 0 failures, 0 errors, 0 skips
CI も通っているので、ちょっと原因がわからないですね。reckyy さんの実行結果などを載せていただければ、なにかわかるかもしれないです。🙇♂️@reckyy お疲れです🍵
記事も拝見しましたが、自分もユニークなクラスやIDを付与するのが最善だと思います。ただ、このPR内では現状のテストがベストだと思いました。一応、komagataさんのレビューの際にもご相談されると良いかと思います。
細かく見ていただいてありがとうございます。komagata さんレビュー時にも相談してみたいと思います。
- カードの幅問題
私の環境でも確認できました。ユーザ名が長いとカードの幅が広がってしまうみたいです。 ウィンドウ幅が広いと「...」で省略され、ある一定のサイズまで狭めると全て表示するみたいです。あまりよろしくない仕様かと思うので「ウィンドウ幅を狭めるとユーザ名が省略されなくなる」といった別 issue で登録するか相談してみます。（デザインの部分なので町田さんが対応されるかもしれません）
承知しました！お任せします。
相談してみたいと思います！🙇♂️
- testが落ちる件 こちらごめんなさい、今日回してみると問題なくパスしました。。PCの調子が悪かったのかもしれません。 この件はご放念ください。
稀によくありますね😄 全然大丈夫です！🙆♂️
reckyy
commented
2 months ago @MikotoMakizuru ご対応ありがとうございます！
自分としてはApproveなのですが、以下のIssueが進行しているようです。 もしご存知でしたら申し訳ないです。
今回のPR内ではproduct.vueを使用していますが、上記PRではproduct.vueやproducts.vueなどのVueファイルをViewComponentに移行しているようなので、上記PRがmergeされてからの方がいいのかなと思いました。
（komagataさん、もしくは @Shrimprin さんにご相談されるのがいいかと思います。）
ご返信お待ちしております。
MikotoMakizuru
commented
2 months ago @reckyy komagata さん、machida さんに確認してみます〜
komagata
commented
1 month ago @MikotoMakizuru
しかし、このテストは .card-list-item の 1 つ目と 2 つ目に必ず特定の提出物が表示されることを前提としているため、新たに .card-list-item を使用したカードが表示されると、テストが失敗するリスクがあります。これは、変更に対して脆いテストコードとなってしまいます。伊藤さんの記事の解法4のように ID を指定して各提出物を特定できればテストの安定性を高められますが、今回は View の修正までは行っていません。もし他に良い改善案があれば、ご指摘いただけると幸いです。
「研修終了まで7日」などのブロックで絞り込んでいる（within）のであれば問題ないです。
MikotoMakizuru
commented
1 month ago @reckyy
今回のPR内ではproduct.vueを使用していますが、上記PRではproduct.vueやproducts.vueなどのVueファイルをViewComponentに移行しているようなので、上記PRがmergeされてからの方がいいのかなと思いました。 （komagataさん、もしくは
@Shrimprinさんにご相談されるのがいいかと思います。） ご返信お待ちしております。
今週のスクラム MTG で相談した結果、@Shrimprin さんの実装が非 vue 化の対応であり、私が今回 vue で追加実装した部分はマージできないため、非 vue 化のプルリクがマージされたら rebase して rails の view で作り直す形となりました。（ポイントは別で付与していただくそうです。）
Issue
7912
概要
研修生には必ずではありませんが、研修終了日が設定されています。今回の実装では、研修終了日が 7 日以内の研修生の提出物一覧をメンターのダッシュボードに表示するようにしました。これにより、メンターは研修終了が近い研修生の提出物を優先的にレビューすることが可能になります。
変更確認方法
feature/show-products-trainees-whose-training-end-date-within-7daysをローカルに取り込むrails db:seedを実行して初期データ投入（この操作はローカルのDBを初期化するので、初期化しても問題ないタイミングで行ってください。）foreman start -f Procfile.devでローカル環境を立ち上げるScreenshot
変更前
変更後
補足事項
今回の実装により、研修終了日が 7 日以内の提出物一覧が表示されることで、「研修終了日が 7 日以内」かつ「提出から 5 日経過」の提出物が以下の画像のように 2 件表示される仕様となります。この仕様についてはこちらのコメントで相談したうえで実装を行いました。
気になっていること（不安要素）
今回の実装では、「研修終了日が 7 日以内の提出物一覧が表示されること」を確認するテストを作成しました。
今回変更を行ったことにより、(補足事項に記載したように)提出物が 2 件表示されるため、上記のテストだと研修終了日が 7 日以内のカードに表示されていなくても、他のカードに表示されていればテストがパスしてしまいます。そこで、対象の提出物と絞るため、こちらの記事を参考に、
withinでスコープを絞りました。繰り返し要素の各枠には.card-list-itemが使われていたため、1つ目と2つ目の要素をそれぞれ指定しています。しかし、このテストは
.card-list-itemの 1 つ目と 2 つ目に必ず特定の提出物が表示されることを前提としているため、新たに.card-list-itemを使用したカードが表示されると、テストが失敗するリスクがあります。これは、変更に対して脆いテストコードとなってしまいます。伊藤さんの記事の解法4のように ID を指定して各提出物を特定できればテストの安定性を高められますが、今回は View の修正までは行っていません。もし他に良い改善案があれば、ご指摘いただけると幸いです。