-
Notifications
You must be signed in to change notification settings - Fork 71
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
ブログ記事を published_at 順に並べる #7955
Conversation
@@ -62,7 +62,7 @@ def set_article | |||
end | |||
|
|||
def list_articles | |||
articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }).order(created_at: :desc).page(params[:page]) | |||
articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }).order(published_at: :desc).page(params[:page]) |
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 messageにも書いたのですが、PostgreSQLではNULL値は非NULL値より大きいとみなされます。
WIP記事はpublised_at
が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.
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 |
@masyuko0222 引き続きよろしくお願いいたします🙇 |
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.
問題ありません!〇
一点細かすぎる点で恐縮ですがコメントさせて頂いたのでご確認お願いいたします~。
@@ -62,7 +62,7 @@ def set_article | |||
end | |||
|
|||
def list_articles | |||
articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }).order(created_at: :desc).page(params[:page]) | |||
articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }).order(published_at: :desc).page(params[:page]) |
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.
[NITS]
動作確認の
ブログ一覧ページで作成したWIP記事がブログ記事の先頭に存在されていること
についてですが、既存の「仮想マシンにDebianをインストールしよう」が常に先頭に来るようになっています。(環境依存だったらすみません)
ただ開発環境だけの問題かもしれませんし、WIPが沢山あってどれが最新か分かりづらい、という状況も実際はほぼ起こらないのかなぁとも思います。
(仕様次第だとも思いますし、)修正すべきかのご判断はお任せします!
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.
以下2点、コメントの修正を行いましたのでご確認ください🙏 |
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
承知しました!そうしましたらレビューとしては問題ありません!
Approveいたします~。
Approveありがとうございます〜! |
PostgreSQLでは、NULL値は非NULL値より大きいとみなされる https://www.postgresql.jp/document/15/html/queries-order.html
f536457
to
004670c
Compare
お疲れ様ですー! |
@@ -67,7 +67,7 @@ def set_article | |||
end | |||
|
|||
def list_articles | |||
articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }).order(created_at: :desc).page(params[:page]) | |||
articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }).order(published_at: :desc, created_at: :desc).page(params[:page]) |
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.
1行が横に長過ぎるので適度に改行するのが良さそうです。
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.
495accfで改行を加えました🙆
test/system/articles_test.rb
Outdated
visit_with_auth articles_path, 'komagata' | ||
titles = all('h2.thumbnail-card__title').map(&:text) | ||
|
||
assert_includes titles[0], @article3.title |
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.
配列と配列を比べるのが1回で済むので良いかなと思います。
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.
e59f94dで配列同士を比較する形にしました〜
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.
@komagata
お疲れ様です〜確認ありがとうございました!
コメントを踏まえて修正したので、ご確認ください🙏
@@ -67,7 +67,7 @@ def set_article | |||
end | |||
|
|||
def list_articles | |||
articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }).order(created_at: :desc).page(params[:page]) | |||
articles = Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }).order(published_at: :desc, created_at: :desc).page(params[:page]) |
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.
495accfで改行を加えました🙆
test/system/articles_test.rb
Outdated
visit_with_auth articles_path, 'komagata' | ||
titles = all('h2.thumbnail-card__title').map(&:text) | ||
|
||
assert_includes titles[0], @article3.title |
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.
e59f94dで配列同士を比較する形にしました〜
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.
確認させて頂きました。OKです〜🙆♂️
@Judeeeee 本番環境で確認してOKでした〜。
といった内容もとっても重要な情報ですので、GitHub上(まさにこのスレッド)で行うようにお願いします〜。(他でやった場合もメモとしてこちらに残しておいていただければありがたいです。) |
@komagata |
issue
概要
現在ブログ記事は公開日時の降順で並んでおらず、2021年の記事が先頭に表示されています。
この問題を解決するために、本PRではブログ記事を公開日時の降順で並ぶ変更を加えました。
また、メンター・管理者のみ表示されるWIP記事は仕様についての認識擦り合わせから以下の条件で表示させました。
修正前
修正後
変更確認方法
feature/order-articles-by-published_at-desc
をローカルに取り込むforeman start -f Procfile.dev
でローカルサーバーを立ち上げる確認事項