Skip to content
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

/sponsorshipsページの新規作成 #8338

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SuzukiShuntarou
Copy link
Contributor

@SuzukiShuntarou SuzukiShuntarou commented Feb 13, 2025

Issue

概要

  • /sponsorshipsページを新規に追加し、sponsorshipタグの付いたブログ記事を1ページ当たり最新12個表示。
    ログインしていない状態でも該当のページを閲覧可能。

変更確認方法

  1. feature/create-sponsorships-pageをローカルに取り込む。

    1. git fetch origin feature/create-sponsorships-page
    2. git checkout feature/create-sponsorships-page
  2. rails db:seedを実行してsponsorshipタグのブログ記事の初期データを追加する。

  3. foreman start -f Procfile.devでローカル環境を立ち上げる。

  4. 未ログイン状態でヘッダー部のブログをクリックする。

    1. 全ての記事が表示されていることを確認する。
      image
  5. 未ログイン状態で/sponsorshipsページへアクセスする

    1. 以下の画面キャプチャのようなSponsorshipsサムネイル画像の記事のみが表示されることを確認する。
      image
  6. ユーザー名 komagata パスワード testtest でログインする。

  7. ログイン状態で/sponsorshipsページへアクセスする

    1. 以下の画面キャプチャのようなSponsorshipsサムネイル画像の記事のみが表示されることを確認する。
      image

Screenshot

変更前

  • /sponsorshipsページへのアクセス
    image

  • フッター
    image

変更後

  • /sponsorshipsページへのアクセス
    image

  • フッター
    image

@SuzukiShuntarou SuzukiShuntarou self-assigned this Feb 13, 2025
@SuzukiShuntarou SuzukiShuntarou marked this pull request as ready for review February 14, 2025 00:47
@SuzukiShuntarou
Copy link
Contributor Author

SuzukiShuntarou commented Feb 14, 2025

@harada-webdev
お疲れ様です!お手すきの際にレビューをお願いできますか?急ぎではございません。
Good First Issueではなく申し訳ないのですが、よろしくお願いいたします。
難しい場合はご連絡いただければ幸いです!

@harada-webdev
Copy link
Contributor

harada-webdev commented Feb 14, 2025

@SuzukiShuntarou
レビューは行えます!
ですが、自分の環境(WSL2)では変更後のスクリーンショットの画像のようにはならなかったです。

  • /articles
    スクリーンショット (650)

  • /sponsorships
    スクリーンショット (649)

また、ログイン後の/sponsorshipsにバグがあることが確認できました。(WIPボタンを押すと、/articles/wipsに遷移してしまう)

Clipchamp.mp4

以上、問題が解決されましたらレビューをいたしますので、お手数ですが再度ご確認のほどお願いいたします!

@SuzukiShuntarou
Copy link
Contributor Author

@harada-webdev
ご確認ありがとうございます!すみません、こちらで再度確認をしてみます。ご迷惑をおかけして申し訳ないです。

@SuzukiShuntarou SuzukiShuntarou force-pushed the feature/create-sponsorships-page branch from 05602f2 to a6ac8b4 Compare February 15, 2025 11:00
@SuzukiShuntarou
Copy link
Contributor Author

SuzukiShuntarou commented Feb 15, 2025

@harada-webdev
コメント頂いておりました件、ご連絡いたします。急ぎではございませんのでよろしくお願いいたします。

ですが、自分の環境(WSL2)では変更後のスクリーンショットの画像のようにはならなかったです。

  • こちら失礼いたしました。初期データの設定ができていませんでした。確認手順の中で2.rails db:seedを実行してsponsorshipタグのブログ記事の初期データを追加する。という手順を追加いたしました。再度ご確認をお願いいたします。

また、ログイン後の/sponsorshipsにバグがあることが確認できました。(WIPボタンを押すと、/articles/wipsに遷移してしまう)

  • こちらについては町田さんに確認いたしました。公開済 | WIPボタンで/articlesへ遷移するままで良いことを確認いたしました。ご指摘ありがとうございました!

@harada-webdev
Copy link
Contributor

harada-webdev commented Feb 15, 2025

@SuzukiShuntarou
修正ありがとうございます!
自分の環境でも動作確認できました
17日までにレビューを行いますので今しばらくお待ちください🙏

Copy link
Contributor

@harada-webdev harada-webdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SuzukiShuntarou
二点ほど修正したほうがいい箇所があったのでコメントしました!
ご確認のほどお願いいたします🙏


class Articles::SponsorshipsController < ApplicationController
skip_before_action :require_active_user_login, raise: false, only: %i[index]
MAX_PAGE_COUNT = 12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

他のControllerで定義されている1ページあたりの要素数を格納している定数名のほとんどがPAGER_NUMBERなので、その定数名を踏襲した方が良いのではないかと考えました。(grep -r "PAGER_NUMBER" app/controllersで確認できます)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます!他コントローラーでも使っているのを確認いたしました。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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ブログ記事を並べる順番について、
ArticlesControllerで記事を並べるためのメソッドsorted_articlespublished_atの順で並べているのと、ブログ記事を編集する際に記事公開日時を任意で変更できるのですが、その変更された新しい日時情報を格納しているカラムがpublished_atなので、created_atの順よりもpublished_atの順でブログ記事を並べたほうがよいのではないかと思いました。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます!こちらも動作確認、該当のメソッドも確認いたしました。
ご指摘の通りで created_atで並び替えを行うと/articlesと異なる並び順となっておりました。
published_atで並び替えを行うように変更いたしました。

@harada-webdev
Copy link
Contributor

@SuzukiShuntarou
通知用です。
レビューの際に、Suzukiさんにメンションをしていなくて通知が届いていない可能性があったのでここでメンションさせていただきました。

@SuzukiShuntarou
Copy link
Contributor Author

@harada-webdev
ご確認、ご指摘ありがとうございます!修正いたしましたので再度ご確認をどうぞよろしくお願いいたします。
細かな内容は頂いたコメントにそれぞれ返信いたしました!

Copy link
Contributor

@harada-webdev harada-webdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SuzukiShuntarou
修正を確認できましたので、Approveにいたしました😊

@SuzukiShuntarou
Copy link
Contributor Author

SuzukiShuntarou commented Feb 19, 2025

@harada-webdev
レビューありがとうございました!

@komagata
メンバーレビューが完了いたしました。レビューをよろしくお願いいたします。
ふりかえり・計画ミーティングでご指摘いただいたリンクについて、ログイン前画面のフッターに追加いたしました。(2/19追記)

@SuzukiShuntarou SuzukiShuntarou force-pushed the feature/create-sponsorships-page branch from 2c35559 to 214dafb Compare February 19, 2025 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants