Skip to content

プレスキットページ内に最新のプレスリリース記事を6つまで表示し、プレスリリース記事の一覧表示ページを作成#8350

Merged
komagata merged 3 commits intomainfrom
feature/show_latest_press_release_articles_in_presskit
May 13, 2025
Merged

プレスキットページ内に最新のプレスリリース記事を6つまで表示し、プレスリリース記事の一覧表示ページを作成#8350
komagata merged 3 commits intomainfrom
feature/show_latest_press_release_articles_in_presskit

Conversation

@Ryooo-k
Copy link
Copy Markdown
Contributor

@Ryooo-k Ryooo-k commented Feb 19, 2025

Issue

概要

プレスキットのページ内に、プレスリリースタグを持つブログ記事を最大6つまで表示するようにしました。
また「全てのプレスリリース」ボタンを押すと、プレスリリースタグを持つブログ記事の一覧表示されるページにアクセスするようにしました。

変更確認方法

  1. feature/show_latest_press_release_articles_in_presskit をローカルに取り込む
    i. git fetch origin pull/8350/head:feature/show_latest_press_release_articles_in_presskit
    ii. git checkout feature/show_latest_press_release_articles_in_presskit
  2. bin/rails db:migrate RAILS_ENV=test でfixtureのデータをDBに取り入れる
  3. foreman start -f Procfile.dev でローカルサーバーを立ち上げる
  4. プレスキットにアクセスする
  5. 最新のプレスリリースとして6つブログ記事が表示されていることを確認する
スクリーンショット 2025-03-12 18 59 40
  1. 上記画像の下部の「全てのプレスリリース」ボタンを押し、プレスリリース一覧ページにアクセスできることを確認する
  2. 全てのプレスリリース記事(30個)が表示されていることを確認する
スクリーンショット 2025-03-12 19 02 27

Screenshot

変更前

スクリーンショット 2025-03-12 19 04 19

変更後

スクリーンショット 2025-03-12 18 59 40 スクリーンショット 2025-03-12 19 02 27

@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented Feb 19, 2025

@machida
お疲れ様です!
下記2点のデザインのご確認・修正をお願いいたします。

プレスキット

プレスキットのページ内に、下記動画のようにプレスリリース記事を6つまで表示しております。プレスリリースの表示に関してのデザインのご確認、修正お願いいたします。
現状は、ブログのページを参考にしております。

2025-02-19.11.28.20.mov

プレスリリース一覧ページ

プレスキット内の「全てのプレスリリース」のリンクからプレスリリース一覧ページにアクセス出来ます。
現状は、ブログのページを参考にしております。
こちらもデザインのご確認をお願いいたします。

2025-02-19.11.35.08.mov

@Ryooo-k Ryooo-k requested a review from machida February 19, 2025 09:54
@machida
Copy link
Copy Markdown
Member

machida commented Feb 19, 2025

@Ryooo-k 対応ありがとうございます!デザインをいじってコミットを追加しますー

@machida machida self-assigned this Feb 19, 2025
@machida
Copy link
Copy Markdown
Member

machida commented Mar 10, 2025

@Ryooo-k すいません、遅くなりました🙇‍♂️デザインを入れましたのでご確認お願いします🙏

@machida
Copy link
Copy Markdown
Member

machida commented Mar 11, 2025

@Ryooo-k rebase してメンバーレビュー、駒形さんレビューをお願いします🙏

@Ryooo-k Ryooo-k force-pushed the feature/show_latest_press_release_articles_in_presskit branch from ca70092 to 0fd561d Compare March 12, 2025 07:40
@Ryooo-k Ryooo-k self-assigned this Mar 12, 2025
@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented Mar 12, 2025

@machida

デザインありがとうございます!

@kitarou888
お疲れ様です!
こちらのレビューをお願いしたいのですが可能でしょうか?

@Ryooo-k Ryooo-k requested a review from kitarou888 March 12, 2025 08:06
@Ryooo-k Ryooo-k marked this pull request as ready for review March 12, 2025 08:06
@kitarou888
Copy link
Copy Markdown
Contributor

@Ryooo-k
了解しました!
週末までにとりかかりたいと思いますので、(内容の理解に時間を要するなど)状況に応じてご連絡・ご相談させてください!

@kitarou888
Copy link
Copy Markdown
Contributor

kitarou888 commented Mar 12, 2025

@Ryooo-k
私の環境でbin/setupforeman start -f Procfile.devしましたが、プレスリリース欄にブログ記事は1件も表示されず、「全てのプレスリリース」ボタンも表示されませんでした。(下記)

image

http://localhost:3000/press_releases に直接アクセスしたところ真っ白でした。(下記)

image

ブログデータのセットなど必要な手順があれば「変更確認方法」に追記くださいー

Gemfile.lock Outdated
zeitwerk (2.6.18)

PLATFORMS
arm64-darwin-23
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

こちらは本イシューで必要な修正でしょうか?
それともたまたま紛れてしまったものでしょうか?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ごめんなさい。こちらの日報など拝見すると、Gemfile.lockの差分は解消されそうでした。
git restoreで差分を戻そうとしても勝手にarm64-darwin-23が追加されるので、困っておりましたがもう少しちゃんと調べるべきでした😅
こちらのコミットはgit revertpush --forceで差分がなくなりようにします。

require 'application_system_test_case'

class PressKitTest < ApplicationSystemTestCase
test 'show press releases' do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

プレスリリース一覧ページへのリンクボタンがあることのテストもあるとよさそうです。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

systemテストをどこまでやるかはかなり悩んだんですよねー😓
他のsystemテストも確認してるのですが、例えばwelcomeページにある「コース内容を確認する」のリンクボタンも、リンクボタンが存在することを確認するテストは無さそうでした。

スクリーンショット 2025-03-12 23 36 05

ただし「コース内容を確認する」のリンク先であるページにアクセスして表示が適切であるかのテストはあるようでした。

class WelcomeTest < ApplicationSystemTestCase
  test 'GET /practices' do
    visit '/practices'
    assert_equal '学習内容 | FJORD BOOT CAMP(フィヨルドブートキャンプ)', title
    assert_selector "meta[property='og:title'][content='学習内容']", visible: false
    assert_selector "meta[name='twitter:title'][content='学習内容']", visible: false
  end

おそらくsystemテストはかなり重い処理なので、重要度が低いものはテストをしないようにしているかなと。(どこかのPRでkomagataさんがそのようなレビューをしているのを見かけました。そのPRはメモをとっておらず😭)

プレスリリース一覧ページへのリンクボタンがあることのテストを入れるかどうかは、他のテストファイルも入念に確認して、もう少し考えてみます。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Ryooo-k
ご事情、ありがとうございます!!
なるほど・・・おっしゃるとおり「システムテストをどこまでやるか」問題はDocsなどに書かれていないと今後も混乱を招きそうですね💦
ちょっと私の方からスクラムマスター(駒形さん)に問題提起してみることにしますので、Ryooo-kさんのご判断の上で書かれていないのであれば、システムテストに関するレビューについては修正していただかなくて大丈夫です🙆‍♂️

以下は個人的な考えとして、残しておきます↓

システムテストはそれ自体仕様になるものなので、たとえば今回のイシューであれば

  • プレスキットページへのパスにアクセスしたら
    • プレスリリースのページにちゃんと飛んでいて、
    • ブログが6件だけ存在し、それらは
      • WIPは含まれず、
      • すべてにプレスリリースタグがついていて、
      • created_atでなくpublished_atで降順になっていて、
    • プレスリリース一覧への導線が置いてあり、
  • プレスリリース一覧ページへのパスにアクセスしたら
    • プレスリリース一覧ページにちゃんと飛んでいて、
    • ブログが最大30件だけ表示されていて、それらは
      • WIPは含まれず、
      • すべてにプレスリリースタグがついていて、
      • created_atでなくpublished_atで降順になっていて、

はシステムテストで最低限カバーすべきかなと思いました。


test 'show only published press releases' do
visit press_releases_path
assert_selector '.thumbnail-card.a-card', count: 7
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

このテストの目的は「表示されるブログ記事がすべてpublishedであること」でしょうか?
であればカウント数でassertするのではなく、表示されているすべてのブログのpublished属性がtrueであることをテストしたほうがよさそうです。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

systemテストでpublished属性がtrueであることを確認するのは難しいかと...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

「難しい」とおっしゃる内容をもう少し詳しく教えていただいてもよいでしょうか?🙏

というのも読む側が7という数字を見ても、一体なにをテストしているのかがすぐに理解できないためです(いわゆるマジックナンバー)。

教えていただいた上で、私のほうでもよりよい書き方がないかいっしょに調査したいと思いますので!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

「難しい」とおっしゃる内容をもう少し詳しく教えていただいてもよいでしょうか?🙏

published_atはboolean型ではなくdate型なのでtrueかどうかの判定ができない点と、もし仮にpublished_atで確認するとして場合、その確認方法が思いつかなかったです。

下の写真は、実際のアプリケーションのブログ一覧ページですが、赤枠で囲ってる部分がpublished_atの値となります。HTML内では<div class="thumbnail-card__date">2025年02月25日(火) 02:41</div>となっており、published_atとしてのテストが難しいかなーと思いました。

スクリーンショット 2025-03-13 9 30 12

というのも読む側が7という数字を見ても、一体なにをテストしているのかがすぐに理解できないためです(いわゆるマジックナンバー)。

確かに分かりにくいですね!調べてみたのですがarticles_test.rbで参考になるテストがあったので共有します。

  test 'display recent 10 articles on article page' do
    Article.delete_all
    11.times do |i|
      Article.create(
        title: "test title #{i}",
        body: 'test body',
        user: users(:komagata),
        wip: false,
        published_at: "2021-12-31 #{i}:00:00"
      )
    end

    visit article_path Article.last.id
    titles = all('.card-list-item-title').map(&:text)

    within '.card-list' do
      assert_equal 'test title 10', titles.first
      assert_equal 'test title 1', titles.last
      assert_no_text 'test title 0'
      assert_equal all('.card-list-item').count, 10
    end
  end

このテストでは、テストメソッド内でArticle.createで記事を作成しています。こうすることで、Articleが何個データがあるか分かるようになっていました。(Article.delete_allしても次のテストでは、データが復元されるようです)
最後にassert_equal all('.card-list-item').count, 10で数量の確認をしていますので、これを参考にテスト修正してみますね👍(テストメソッドの中でwipも作成して、それが表示されないことを確認しようと思います。)

ご指摘ありがとうございますー!(systemテストって難しい😅)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

HTML内では<div class="thumbnail-card__date">2025年02月25日(火) 02:41</div>となっており、published_atとしてのテストが難しいかなーと思いました。

なるほど!であればこのテキストの値を取り出して、Time型に変換すればassertできるかなと思いました。

ちなみに「publishedであること」というのは具体的にどのような状態を指すのでしょうか??
このテストの必要性を私が理解できていないためすみませんが教えていただけるとうれしいです🙏

Copy link
Copy Markdown
Contributor Author

@Ryooo-k Ryooo-k Mar 13, 2025

Choose a reason for hiding this comment

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

「publishedであること」というのは具体的にどのような状態を指すのでしょうか??
このテストの必要性を私が理解できていないためすみませんが教えていただけるとうれしいです🙏

publishedは初めて公開された日付ですね!
wip状態のプレスリリースは表示されないことを確認したいという目的です!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

publishedは初めて公開された日付ですね!
wip状態のプレスリリースは表示されないことを確認したいという目的です!

なるほど、ということはWIPの記事は上のスクショにある赤枠の日付は書かれないということでしょうか?
もしそうであればWIPかどうかはブラウザでthumbnail-card__dateクラスのtext属性がemptyかどうかを見るだけでassertできるかなと思ったのですが、いかがでしょうか?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

be6b104 にて修正しました。

class PressReleasesTest < ApplicationSystemTestCase
test 'show listing press releases' do
visit press_releases_path
assert_text 'プレスリリース'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

「プレスリリース」のテキストの存在確認ではなく、titleが「プレスリリース」であるページに正しく遷移したかをテストするとよさそうです。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

下記のように変更します。

Suggested change
assert_text 'プレスリリース'
assert_equal 'プレスリリース', title

require 'application_system_test_case'

class PressReleasesTest < ApplicationSystemTestCase
test 'show listing press releases' do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1ページあたり最大30記事であることをテストしてもよいかと感じました。

Copy link
Copy Markdown
Contributor Author

@Ryooo-k Ryooo-k Mar 13, 2025

Choose a reason for hiding this comment

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

これも非常に悩みました。
articles_test.rbuser/reports_test.rbなど表示数に制限があるページはいくつかあるのですが、私が確認する限り、最大の出力数はテストしていないようでした。
こちらももう少し調べてみます🫡

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

最大の出力数をテストすることはわりと簡単かなと思っていて、Capybaraのallメソッドで各記事がもつクラスを指定してあげると配列を返すので、それをsizelengthで数えればよいかと思いましたが、いかがでしょうか??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

教えていただきありがとうございます!allメソッドを使用してテスト書いてみます!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

最大30件の件ですが、masyukoさんが日報にコメントをくださいましたので、参考にできそうです👍

  • 30件はKaminariが担保している数値なのでシステムテストは不要
  • ページネーションの機能自体もシステムテストでは書かない

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

共有ありがとうございます!
システムテストをどこまで行うかは個人やチームの方針などで色々変わってきそうですね👀

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

個人で決まるのはよくないと思うので、チームの方針を皆が見えるようにしてほしいですよね😅

require 'application_system_test_case'

class PressKitTest < ApplicationSystemTestCase
test 'show press releases' do
Copy link
Copy Markdown
Contributor

@kitarou888 kitarou888 Mar 12, 2025

Choose a reason for hiding this comment

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

以下はテストすべきかなと思いました。

  • created_atの降順でなくpublished_atの降順であること
  • WIPは表示されないこと ←たとえばWIP記事のpublished_atをTime.nowにするとか?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

確かにこれはテストするべきですね!ご指摘ありがとうございます。
あと、ページネーションに関してもテストしておいた方が良さそうなので、テストに関しては改めて考えてみます。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ページネーションについてはやるべきかを先ほどQ&Aでスクラムマスターの駒形さんに質問をしましたので、そちらの回答を待つのでもよいかと思いますー🙆‍♂️

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

他のファイルではページネーションのテストを書いているのもありましたので、並び順、ページネーションのテストを追加いたしました。


def press_kit; end
def press_kit
@press_releases = Article.press_releases.includes(%i[user thumbnail_attachment]).order(published_at: :desc)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ここで6件に絞らないのには理由はありますか?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

limitを使用してコントローラー側で6件に絞り込んだ方が良いですね。
ご指摘ありがとうございます!

class PressReleasesController < ApplicationController
skip_before_action :require_active_user_login, raise: false, only: %i[index]

layout 'lp'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

すみません、こちら何のためにあるコードなのか教えてもらえるとうれしいです🙏

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

layout 'lp'の有無で何が変わるかは動画で見ると分かりやすいので下記ご確認ください。

2025-03-13.12.56.49.mov

lpはおそらく「ランディングページ (Landing Page)」のことを指していると思います。
未登録の状態でwelcomeページからアクセスできるサイトはlayout 'lp'で統一していそうです。
layout 'lp'を指定していない場合、views/layouts/application.html.slimが参照されます。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

動画とてもわかりやすかったです🙏

layout 'lp'とするとlp.html.slimに遷移するような情報も見かけたのですが、「そんなファイルはないな・・・」と疑問に思ったため質問させていただきました!

今後の開発の参考にさせていただきます😊

articles = Article.press_releases

articles.each do |article|
assert_equal 'プレスリリース', *article.tag_list
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

こちら気になったので確認させてください!
*article.tag_listの配列展開ですが、リストの要素数が2つ以上('プレスリリース'とそれ以外)であっても問題なくテストは通るのでしょうか?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

確かにタグが2つ以上ある記事だとテスト通らないですね!ご指摘ありがとうございます!
assert_includesに変更いたします。

<% end %>

<% date = Date.parse("2023-01-01") %>
<% (56..85).each do |id| %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

この範囲にidを指定している理由は、これより上に55(56?)のテストデータが存在するためでしょうか?(数字の意図がわからなかったため確認です)

ほか(5..12など)についても同様です。

Copy link
Copy Markdown
Contributor Author

@Ryooo-k Ryooo-k Mar 12, 2025

Choose a reason for hiding this comment

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

そうですね!どちらもidが被らないようにデータを作成するようにしています。
データ数に関してですが、(56..85)は30個のプレスリリースタグを持つブログ記事のデータが開発環境のDBに作成されます。

(5..12)は、8つ分のデータがある状態でテストが実行されるようになります。8つの理由は、7個は公開済みの記事で、残り1つはwipの記事です。
7個公開済みのデータに対してプレスキットのページでは6つしか表示されないことを確認するためのデータです。
8つ目のwipデータは、modelのテストでwipがないことを確認するために作成しています。

Copy link
Copy Markdown
Contributor

@kitarou888 kitarou888 Mar 13, 2025

Choose a reason for hiding this comment

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

@Ryooo-k
なるほど、ありがとうございます!
以下がまだ気になっていますので、教えていただいてもよいでしょうか🙏

  • 必要な数についてはコメントを残すことは.ymlファイルでは難しいものでしょうか?読む側として上で書かれたような情報はとても重要だと感じました!
  • 開始番号が565になっている理由について。101..130101..108の方が30件であること、8件であることがわかりやすいなと思ったのと、開始番号を大きい数にすることで今後テストデータが増えた場合のidかぶり(じつは考えなくてもよかったりする??)などを気にしなくてすむのかなと思いました。

Copy link
Copy Markdown
Contributor Author

@Ryooo-k Ryooo-k Mar 13, 2025

Choose a reason for hiding this comment

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

fixtureに関してですが、こちらのkomagataさんのコメントですが参考になったので共有します。
現状は、テストデータをfixtureで作成するようにしていますが、今回のケースのように特定のテストだけに使用するのであれば、テストメソッド内でテストデータを作成した方が良さそうでした。
今回のプレスリリースタグを持つブログ記事のテストは、「他の多くのテストでも使いそうなやつ」には該当しないと思うので、fixtureの追加は削除するようにしようかと考えています!

読む側として上で書かれたような情報はとても重要だと感じました!
101..130や101..108の方が30件であること、8件であることがわかりやすいなと思った...

こちら大変参考になりました✨読む側が分かりやすいようにするように気をつけます🫡

Copy link
Copy Markdown
Contributor

@kitarou888 kitarou888 Mar 13, 2025

Choose a reason for hiding this comment

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

fixtureに関してですが、こちらのkomagataさんのコメントですが参考になったので共有します。

これは勉強になります!
共有ありがとうございます🙏

であれば30件、8件である理由などもテストメソッド内にコメントとして自然に残せそうですね!

Copy link
Copy Markdown
Contributor

@kitarou888 kitarou888 left a comment

Choose a reason for hiding this comment

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

取り急ぎ気になった点をレビューしました!
Active Recordまわりは私が勉強不足で正しいかを判断できなかったため、これから勉強したのちに、改めてレビューさせていただければと思います〜🙏

@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented Mar 12, 2025

@kitarou888
早速ご確認ありがとうございます!
DB に関しては、変更確認方法のところに記載しておくべきでした😅
変更確認方法に記載しましたが、rails db:fixtures:loadをしていただければおそらくDBに反映されると思います!
コメントいただいた件は、確認して私からもコメント返信させていただきますね👍

@kitarou888
Copy link
Copy Markdown
Contributor

@Ryooo-k
rails db:fixtures:load了解しました!
明日さっそくためしてみたいと思います〜🚀
(今日はすみません力尽きました・・・😪)

@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented Mar 13, 2025

@kitarou888
レビューありがとうございます!一通りコメントは返信いたしました。
テストに関しては、再考いたします。
また、テスト環境のDBを反映するコマンドが間違ってました。bin/rails db:migrate RAILS_ENV=test でお願いします!(Rails ガイド)

修正完了しましたら、ご連絡いたします。

@kitarou888
Copy link
Copy Markdown
Contributor

@Ryooo-k
了解しました!

私のアクションとしまして、以下引き続き進めていきます!

  • 動作確認
    本日行います
  • システムテストをどこまで書くか問題
    今日中にbootcampアプリのQ&AにRyoooさんメンション付きで投稿します
  • モデル、コントローラのレビュー
    本日よりRailsガイド他で最低限の知識をつけてからレビューしたいと思いますので、日曜(orもう少しかかるかも)までお待ちください〜🙏

@kitarou888
Copy link
Copy Markdown
Contributor

@Ryooo-k
動作確認はうまく行きましたので取り急ぎご連絡まで!

※ちなみに教えていただいたbin/rails db:fixtures:loadではうまくいかず、代わりにbin/rails db:seedでうまくいくようになりましたのでご参考まで

@Ryooo-k Ryooo-k requested a review from komagata March 18, 2025 04:24
assert_no_text wip_press_release.title
end

test 'press releases are displayed from the most recent publication date' do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

この辺りはArticleモデルのメソッドとして用意してあればそちらのメソッドをテストすることでほとんど担保できるので実行速度の面からもそちらがいいかもです。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

承知しました!
モデルテストで、公開済みのブログのみを取得するテストと降順になっているかのテストを追加します。
下記システムテストは削除いたします。

  • 公開順に画面表示されているかのテスト
  • wip状態のプレスリリースは含まれていないかを確認するテスト

@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented Mar 26, 2025

@komagata
修正いたしました!
ご確認お願いいたします。

db/schema.rb Outdated
Comment on lines +129 to +130
t.string "name"
t.string "slug"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

意図的な変更でなければ変更しない状態がいいです。
(コマンドとかで勝手になったのだとは思いますが)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

承知しました。git revertで本コミットを取り消します。

assert_equal 'プレスキット | FJORD BOOT CAMP(フィヨルドブートキャンプ)', title
end

test 'show recent 6 press releases' do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

こちらもmodelの方で確認できる(もしくはそういうメソッドがrailsの方にある)のであればsystem testでなくてもほとんど担保できると思うので、system testは不要かなと思います。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

scopeの引数でデータ取得数を指定できるようにし、modelテストでその挙動を確認するようにいたします。

@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented Mar 31, 2025

@komagata
修正いたしました。ご確認お願いいたします🙇

assert_equal 'プレスリリース | FJORD BOOT CAMP(フィヨルドブートキャンプ)', title
end

test 'displays up to 24 articles' do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

show listing press releasesのテスト以外はsystem testは不要かなと思います。

@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented Apr 7, 2025

@komagata
修正いたしました!
ご確認お願いいたします。

@komagata
Copy link
Copy Markdown
Member

@Ryooo-k conflictの修正をお願いします〜

@Ryooo-k Ryooo-k force-pushed the feature/show_latest_press_release_articles_in_presskit branch from 6788c1a to ec078da Compare April 22, 2025 04:17
@komagata
Copy link
Copy Markdown
Member

komagata commented May 4, 2025

@Ryooo-k conflictの修正をお願いします。またrevertのコミットもあるので修正して下さい。

@Ryooo-k Ryooo-k force-pushed the feature/show_latest_press_release_articles_in_presskit branch from b466574 to 71cce18 Compare May 7, 2025 04:09
@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented May 7, 2025

@komagata
修正しました!(moviesテストが通らない問題は、コンフリクトの反映が間違っていたことが原因でした。)
遅くなりましたが、ご確認お願いいたします🙇

@komagata
Copy link
Copy Markdown
Member

komagata commented May 7, 2025

@Ryooo-k Revertのコミットが含まれているので修正して下さい。Revertは基本的に緊急時以外使わないです。

@Ryooo-k Ryooo-k force-pushed the feature/show_latest_press_release_articles_in_presskit branch from 71cce18 to b158766 Compare May 8, 2025 01:03
@machida machida removed their assignment May 8, 2025
@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented May 8, 2025

@komagata
失礼いたしました🙇Revertは使用しないようにします。
コミットを変更しました。ご確認お願いします!

@komagata
Copy link
Copy Markdown
Member

komagata commented May 9, 2025

@Ryooo-k conflictの修正をお願いします〜

@Ryooo-k Ryooo-k force-pushed the feature/show_latest_press_release_articles_in_presskit branch from b158766 to d84d702 Compare May 13, 2025 03:12
@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented May 13, 2025

@komagata
修正いたしました!ご確認お願いいたします。

Copy link
Copy Markdown
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

確認させて頂きました。OKです〜🙆‍♂️

@komagata komagata merged commit 0cf357d into main May 13, 2025
2 checks passed
@komagata komagata deleted the feature/show_latest_press_release_articles_in_presskit branch May 13, 2025 19:09
@github-actions github-actions bot mentioned this pull request May 13, 2025
19 tasks
@Ryooo-k
Copy link
Copy Markdown
Contributor Author

Ryooo-k commented May 13, 2025

ご確認ありがとうございます!

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.

4 participants