/sponsorshipsページの新規作成 #8338
/sponsorshipsページの新規作成 #8338
Conversation
|
@harada-webdev |
|
@SuzukiShuntarou
また、ログイン後の/sponsorshipsにバグがあることが確認できました。(WIPボタンを押すと、/articles/wipsに遷移してしまう) Clipchamp.mp4以上、問題が解決されましたらレビューをいたしますので、お手数ですが再度ご確認のほどお願いいたします！ |
|
@harada-webdev |
SuzukiShuntarou
force-pushed
the
feature/create-sponsorships-page
branch
from
05602f2 to
a6ac8b4
Compare
|
@harada-webdev
|
|
@SuzukiShuntarou |
There was a problem hiding this comment.
@SuzukiShuntarou
二点ほど修正したほうがいい箇所があったのでコメントしました！
ご確認のほどお願いいたします🙏
|
|
||
| class Articles::SponsorshipsController < ApplicationController | ||
| skip_before_action :require_active_user_login, raise: false, only: %i[index] | ||
| MAX_PAGE_COUNT = 12 |
There was a problem hiding this comment.
他のControllerで定義されている1ページあたりの要素数を格納している定数名のほとんどがPAGER_NUMBERなので、その定数名を踏襲した方が良いのではないかと考えました。(grep -r "PAGER_NUMBER" app/controllersで確認できます)
There was a problem hiding this comment.
ありがとうございます！他コントローラーでも使っているのを確認いたしました。PAGER_NUMBERに変更しました。
|
|
||
| def sponsorships_articles | ||
| Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }) | ||
| .where(thumbnail_type: :sponsorship, wip: false).order(created_at: :desc).page(params[:page]) |
There was a problem hiding this comment.
ブログ記事を並べる順番について、
ArticlesControllerで記事を並べるためのメソッドsorted_articlesがpublished_atの順で並べているのと、ブログ記事を編集する際に記事公開日時を任意で変更できるのですが、その変更された新しい日時情報を格納しているカラムがpublished_atなので、created_atの順よりもpublished_atの順でブログ記事を並べたほうがよいのではないかと思いました。
There was a problem hiding this comment.
ありがとうございます！こちらも動作確認、該当のメソッドも確認いたしました。
ご指摘の通りで created_atで並び替えを行うと/articlesと異なる並び順となっておりました。
published_atで並び替えを行うように変更いたしました。
|
@SuzukiShuntarou |
|
@harada-webdev |
|
@harada-webdev @komagata |
SuzukiShuntarou
force-pushed
the
feature/create-sponsorships-page
branch
from
2c35559 to
214dafb
Compare
|
@okuramasafumi よろしければこちらのレビューを願いできればありがたいです〜！ |
| visit_with_auth sponsorships_path, 'komagata' | ||
| assert_selector '.thumbnail-card__inner', count: 1 | ||
| click_link_or_button 'ブログ記事のブランクアイキャッチ画像' | ||
| assert_text 'sponsorshipページに表示される記事です。' |
There was a problem hiding this comment.
どうせテストを書くのなら、データが正しく表示されることもテストしたい気がします。
ブログ記事をsponsorshipなものとそうではないものを2つ作って、正しいものが表示されるようなテストを書くと効果的そうです。
There was a problem hiding this comment.
@okuramasafumi
レビューありがとうございます！対応が遅くなり申し訳ございません。
元々作成されていたブログ記事のテストデータを用いて、/sponsorshipsページではsponsorshipの記事だけが表示され、それ以外のブログ記事（タイトル１とタイトル２）は表示されていないことを確認するassertを追加いたしました。伴って、テストの命名の見直しを行いました。
再度ご確認をどうぞよろしくお願いいたします。
SuzukiShuntarou
force-pushed
the
feature/create-sponsorships-page
branch
from
214dafb to
539c06f
Compare
Issue
概要
/sponsorshipsページを新規に追加し、sponsorshipタグの付いたブログ記事を1ページ当たり最新12個表示。ログインしていない状態でも該当のページを閲覧可能。
変更確認方法
feature/create-sponsorships-pageをローカルに取り込む。git fetch origin feature/create-sponsorships-pagegit checkout feature/create-sponsorships-pagerails db:seedを実行してsponsorshipタグのブログ記事の初期データを追加する。foreman start -f Procfile.devでローカル環境を立ち上げる。
未ログイン状態でヘッダー部のブログをクリックする。
未ログイン状態で
/sponsorshipsページへアクセスするSponsorshipsサムネイル画像の記事のみが表示されることを確認する。ユーザー名
komagataパスワードtesttestでログインする。ログイン状態で
/sponsorshipsページへアクセスするSponsorshipsサムネイル画像の記事のみが表示されることを確認する。Screenshot
変更前
/sponsorshipsページへのアクセスフッター（ログイン前）
変更後
/sponsorshipsページへのアクセスフッター（ログイン前）
