ブログの公開済み記事が公開日順で並ぶようにした #8167

MikotoMakizuru · 2024-10-30T18:44:20Z

Issue

ブログの公開済み記事が公開日順に並んでない（デグレ） #8058

概要

こちらのプルリクの対応により、ブログ一覧のソート順が作成日時の降順で並んでいました。今回のバグ修正対応では、ブログの公開日順で並ぶようにしました。

変更確認方法

bug/articles-are-displayed-from-the-date-of-latest-publication をローカルに取り込む
rails db:seed を実行して初期データ投入（このコマンドはローカルの DB データを初期化するので、初期化しても問題ないタイミングで行ってください。DB のデータが初期状態の場合実行する必要はありません。）
コマンドライン環境で rails c を起動し、下記の「テストーデータ」を投入する ruby のコマンドを実行
foreman start -f Procfile.dev でサーバを起動し、http://localhost:3000/articles/ にアクセス
「1..3番目に表示されてほしいブログ」の順にブログが表示されていることを確認

Screenshot

変更前

変更後

テストデータ

Article.create(
  title: '1番目に表示されてほしいブログ',
  body: 'test',
  created_at: Date.current - 6.day,
  updated_at: Date.current - 6.day,
  user_id: 459775584,
  published_at: Date.current - 1.day
)

Article.create(
  title: '2番目に表示されてほしいブログ',
  body: 'test',
  created_at: Date.current - 5.day,
  updated_at: Date.current - 5.day,
  user_id: 459775584,
  published_at: Date.current - 2.day
)

Article.create(
  title: '3番目に表示されてほしいブログ',
  body: 'test',
  created_at: Date.current - 4.day,
  updated_at: Date.current - 4.day,
  user_id: 459775584,
  published_at: Date.current - 3.day
)

MikotoMakizuru · 2024-11-01T18:49:24Z

@kurumadaisuke
お疲れ様です、コードレビューをお願いすることは可能でしょうか。
よろしくお願いいたします。

kurumadaisuke · 2024-11-03T00:34:25Z

@MikotoMakizuru
すいません、ちょっと家のことでバタバタしていてまとまった時間が取れそうにありません🙇‍♂️

MikotoMakizuru · 2024-11-03T05:25:10Z

@Judeeeee
お疲れ様です、コードレビューをお願いすることは可能でしょうか。
よろしくお願いいたします。

Judeeeee · 2024-11-03T13:28:25Z

@MikotoMakizuru
お疲れ様です〜
承知しました！
今週ちょっと忙しいので、1週間目処でのレビューと認識いただけると幸いです🙇
もし急ぎであれば、お手数ですが他の方に依頼いただけると幸いです🙏

MikotoMakizuru · 2024-11-04T09:39:36Z

@Judeeeee

今週ちょっと忙しいので、1週間目処でのレビューと認識いただけると幸いです🙇

かしこまりました、問題ありません！
よろしくお願いいたします。

Judeeeee · 2024-11-11T11:38:55Z

@MikotoMakizuru
お疲れ様です！
ちょっと本日見れそうにないので、明日レビューさせていただきます🙇
お待たせして恐縮ですが、引き続きよろしくお願いいたします💦

MikotoMakizuru · 2024-11-11T12:51:59Z

@Judeeeee
ご連絡ありがとうございます！
かしこまりました、明日でも問題ありません。

お忙しいところ恐れ入ります、よろしくお願いします🙇‍♂️

Judeeeee

@MikotoMakizuru
お待たせしました！

ブラウザでの動作確認はOKでした🙆
レビューしている中で、気になった箇所がいくつかあるのでコメントしました🙏

Judeeeee · 2024-11-12T03:02:35Z

app/controllers/articles_controller.rb

-  def list_articles
-    articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob })
-                      .order(published_at: :desc, created_at: :desc).page(params[:page])
-    admin_or_mentor_login? ? articles : articles.where(wip: false)
-  end
-
-  def list_recent_articles(number)
-    Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob })
-           .where(wip: false).order(published_at: :desc).limit(number)


確かに未使用のメソッドですが、本PRとは直接関係ない差分なので変更しない方がレビューしやすいです🙇
(気になる気持ちは大いにわかるのですが、余分な差分が多いとコードを追いづらいです💦)

なるほど、そのような観点があるのですね！戻しておきました b5410cf

ありがとうございます！
後出しですが、公式docにも記述があるのでよかったら参考にしてみてください〜！🙏

1 つの目的を満たす、焦点を絞った小さな pull request を作成することを目的とします。 pull request を小さくすると、レビューとマージが簡単かつ迅速になり、バグを導入する余地が少なくなり、変更の履歴がより明確になります。

ref: pull request のベストプラクティス

Judeeeee · 2024-11-12T03:09:04Z

test/system/articles_test.rb

@@ -414,6 +414,40 @@ class ArticlesTest < ApplicationSystemTestCase
    assert_no_text 'WIPの記事は atom feed に表示されない'
  end

+  test 'articles are displayed from the most recent publication date' do


article4 ~ 6がぱっと見で何が違うのか分かりにくいので、変数名を工夫するとよりわかりやすくなる気がします👀

違いがわかるよう変数名再考しましたので、ご確認お願いします c270255

修正ありがとうございます！OKです！🙆

Judeeeee · 2024-11-12T03:10:38Z

test/system/articles_test.rb

+    article4 = Article.create(
+      title: 'タイトル4',
+      body: 'テスト用のWIP記事4',
+      user: users(:komagata),
+      wip: false,
+      created_at: Date.current - 4.days,
+      updated_at: Date.current - 4.days,
+      published_at: Date.current - 3.days
+    )
+    article5 = Article.create(
+      title: 'タイトル5',
+      body: 'テスト用のWIP記事5',
+      user: users(:komagata),
+      wip: false,
+      created_at: Date.current - 5.days,
+      updated_at: Date.current - 5.days,
+      published_at: Date.current - 2.days
+    )
+    article6 = Article.create(
+      title: 'タイトル6',
+      body: 'テスト用のWIP記事6',
+      user: users(:komagata),
+      wip: false,
+      created_at: Date.current - 6.days,
+      updated_at: Date.current - 6.days,
+      published_at: Date.current - 1.day
+    )


fixturesでデータを作成せずに、テストファイルでArticle.createしたのは何か理由があったりしますか？👀

このファイル内で直接Article.createしている方もいるので強く言えないのですが、可読性の観点(コードが短い方がわかりやすいと自身は思っている)からfixtureを使った方がよりよくなると思いました🙆‍♀️

fixturesでデータを作成せずに、テストファイルでArticle.createしたのは何か理由があったりしますか？👀

以前、以下の PR で「fixtures を使用すると、全テストケースで DB に INSERT/DELETE が発生し、テストが重くなるため、1 つのテストでしか使用しないデータは、そのテスト内で作成してください」というレビューをを受けたことがありました。
今回も、このテストだけで使用するデータだったため、テスト内で作成するようにしました。
#7995 (comment)

可読性の観点(コードが短い方がわかりやすいと自身は思っている)からfixtureを使った方がよりよくなると思いました🙆‍♀️

私も Judeeeee さんと同じ考えです。
過去に fixtures に書くべきタイミングについて回答したことがあるのでそちらも参考程度に読んでいただければと思います。 #8066 (comment)

丁寧なコメントありがとうございます🙇

fixtures を使用すると、全テストケースで DB に INSERT/DELETE が発生し、テストが重くなるため

そうだったのですね！こちらについて知らなかったので勉強になりました！ありがとうございます🙏
OKとさせていただきますー🙆

Judeeeee · 2024-11-12T03:27:15Z

test/system/articles_test.rb

+    titles = all('h2.thumbnail-card__title').map(&:text)
+    assert_equal [article6.title, article5.title, article4.title, @article2.title, @article.title], titles


今回テストしたい対象はarticle4~6なので、@ article2, @ articleは含まない方がテストが明確になると思いました🙆‍♀️

Suggested change

titles = all('h2.thumbnail-card__title').map(&:text)

assert_equal [article6.title, article5.title, article4.title, @article2.title, @article.title], titles

top_three_articles = all('h2.thumbnail-card__title').take(3).map(&:text)

assert_equal [article6.title, article5.title, article4.title], top_three_articles

参考: takeメソッド

確かにテストする対象が明確になりますね、修正しました 62741c7

修正ありがとうございます〜！OKです👍

issue の対応とは関係ないため

あわせてテストデータの title, body も修正

MikotoMakizuru · 2024-11-16T08:49:40Z

@Judeeeee
お忙しいところ、レビューありがとうございます！
修正および返信いたしましたので、ご確認お願いします。

Judeeeee

@MikotoMakizuru

とんでもないです！こちらこそ修正ありがとうございました！
コメントに返信いたしましたので、ご確認ください🙏

私からはapproveさせていただきます〜👍
修正お疲れ様でした！🍵

MikotoMakizuru · 2024-11-16T14:01:13Z

@komagata
メンバーレビューが完了しましたので、レビューお願いします！

MikotoMakizuru self-assigned this Oct 31, 2024

MikotoMakizuru force-pushed the bug/articles-are-displayed-from-the-date-of-latest-publication branch from b6dca70 to 2c1ae41 Compare November 1, 2024 12:14

Judeeeee self-requested a review November 3, 2024 13:28

Judeeeee reviewed Nov 12, 2024

View reviewed changes

MikotoMakizuru added 8 commits November 16, 2024 17:23

記事の公開日時が降順で表示されていることをテストするテスト作成

15639e0

記事の公開日時が降順で表示されるようにした

8d91abf

未使用のメソッド削除

a21bf1c

誤って作成してしまったskip_before_actionメソッドを削除

cbf7586

同じ処理内容を持つメソッドを使用

331f914

削除した未使用メソッドを戻す

b5410cf

issue の対応とは関係ないため

テストデータの違いをわかるようにするため変数名修正

c270255

あわせてテストデータの title, body も修正

テスト対象を明確化したテストに修正

62741c7

MikotoMakizuru force-pushed the bug/articles-are-displayed-from-the-date-of-latest-publication branch from 2c1ae41 to 62741c7 Compare November 16, 2024 08:23

Judeeeee approved these changes Nov 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ブログの公開済み記事が公開日順で並ぶようにした #8167

ブログの公開済み記事が公開日順で並ぶようにした #8167

MikotoMakizuru commented Oct 30, 2024 •

edited

Loading

MikotoMakizuru commented Nov 1, 2024

kurumadaisuke commented Nov 3, 2024

MikotoMakizuru commented Nov 3, 2024

Judeeeee commented Nov 3, 2024

MikotoMakizuru commented Nov 4, 2024

Judeeeee commented Nov 11, 2024

MikotoMakizuru commented Nov 11, 2024

Judeeeee left a comment

Judeeeee Nov 12, 2024

MikotoMakizuru Nov 16, 2024

Judeeeee Nov 16, 2024

Judeeeee Nov 12, 2024

MikotoMakizuru Nov 16, 2024

Judeeeee Nov 16, 2024

Judeeeee Nov 12, 2024

MikotoMakizuru Nov 16, 2024

Judeeeee Nov 16, 2024

Judeeeee Nov 12, 2024

MikotoMakizuru Nov 16, 2024

Judeeeee Nov 16, 2024

MikotoMakizuru commented Nov 16, 2024

Judeeeee left a comment

MikotoMakizuru commented Nov 16, 2024

		titles = all('h2.thumbnail-card__title').map(&:text)
		assert_equal [article6.title, article5.title, article4.title, @article2.title, @article.title], titles

ブログの公開済み記事が公開日順で並ぶようにした #8167

Are you sure you want to change the base?

ブログの公開済み記事が公開日順で並ぶようにした #8167

Conversation

MikotoMakizuru commented Oct 30, 2024 • edited Loading

Issue

概要

変更確認方法

Screenshot

変更前

変更後

テストデータ

MikotoMakizuru commented Nov 1, 2024

kurumadaisuke commented Nov 3, 2024

MikotoMakizuru commented Nov 3, 2024

Judeeeee commented Nov 3, 2024

MikotoMakizuru commented Nov 4, 2024

Judeeeee commented Nov 11, 2024

MikotoMakizuru commented Nov 11, 2024

Judeeeee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikotoMakizuru commented Nov 16, 2024

Judeeeee left a comment

Choose a reason for hiding this comment

MikotoMakizuru commented Nov 16, 2024

MikotoMakizuru commented Oct 30, 2024 •

edited

Loading