プレスキットページ内に最新のプレスリリース記事を6つまで表示し、プレスリリース記事の一覧表示ページを作成#8350
Conversation
|
@machida プレスキットプレスキットのページ内に、下記動画のようにプレスリリース記事を6つまで表示しております。プレスリリースの表示に関してのデザインのご確認、修正お願いいたします。 2025-02-19.11.28.20.movプレスリリース一覧ページプレスキット内の「全てのプレスリリース」のリンクからプレスリリース一覧ページにアクセス出来ます。 2025-02-19.11.35.08.mov |
|
@Ryooo-k 対応ありがとうございます!デザインをいじってコミットを追加しますー |
|
@Ryooo-k すいません、遅くなりました🙇♂️デザインを入れましたのでご確認お願いします🙏 |
|
@Ryooo-k rebase してメンバーレビュー、駒形さんレビューをお願いします🙏 |
ca70092 to
0fd561d
Compare
|
デザインありがとうございます! @kitarou888 |
|
@Ryooo-k |
|
@Ryooo-k
http://localhost:3000/press_releases に直接アクセスしたところ真っ白でした。(下記)
ブログデータのセットなど必要な手順があれば「変更確認方法」に追記くださいー |
Gemfile.lock
Outdated
| zeitwerk (2.6.18) | ||
|
|
||
| PLATFORMS | ||
| arm64-darwin-23 |
There was a problem hiding this comment.
こちらは本イシューで必要な修正でしょうか?
それともたまたま紛れてしまったものでしょうか?
There was a problem hiding this comment.
ごめんなさい。こちらの日報など拝見すると、Gemfile.lockの差分は解消されそうでした。
git restoreで差分を戻そうとしても勝手にarm64-darwin-23が追加されるので、困っておりましたがもう少しちゃんと調べるべきでした😅
こちらのコミットはgit revertかpush --forceで差分がなくなりようにします。
test/system/press_kit_test.rb
Outdated
| require 'application_system_test_case' | ||
|
|
||
| class PressKitTest < ApplicationSystemTestCase | ||
| test 'show press releases' do |
There was a problem hiding this comment.
プレスリリース一覧ページへのリンクボタンがあることのテストもあるとよさそうです。
There was a problem hiding this comment.
systemテストをどこまでやるかはかなり悩んだんですよねー😓
他のsystemテストも確認してるのですが、例えばwelcomeページにある「コース内容を確認する」のリンクボタンも、リンクボタンが存在することを確認するテストは無さそうでした。
ただし「コース内容を確認する」のリンク先であるページにアクセスして表示が適切であるかのテストはあるようでした。
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はメモをとっておらず😭)
プレスリリース一覧ページへのリンクボタンがあることのテストを入れるかどうかは、他のテストファイルも入念に確認して、もう少し考えてみます。
There was a problem hiding this comment.
@Ryooo-k
ご事情、ありがとうございます!!
なるほど・・・おっしゃるとおり「システムテストをどこまでやるか」問題はDocsなどに書かれていないと今後も混乱を招きそうですね💦
ちょっと私の方からスクラムマスター(駒形さん)に問題提起してみることにしますので、Ryooo-kさんのご判断の上で書かれていないのであれば、システムテストに関するレビューについては修正していただかなくて大丈夫です🙆♂️
以下は個人的な考えとして、残しておきます↓
システムテストはそれ自体仕様になるものなので、たとえば今回のイシューであれば
- プレスキットページへのパスにアクセスしたら
- プレスリリースのページにちゃんと飛んでいて、
- ブログが6件だけ存在し、それらは
- WIPは含まれず、
- すべてにプレスリリースタグがついていて、
- created_atでなくpublished_atで降順になっていて、
- プレスリリース一覧への導線が置いてあり、
- プレスリリース一覧ページへのパスにアクセスしたら
- プレスリリース一覧ページにちゃんと飛んでいて、
- ブログが最大30件だけ表示されていて、それらは
- WIPは含まれず、
- すべてにプレスリリースタグがついていて、
- created_atでなくpublished_atで降順になっていて、
はシステムテストで最低限カバーすべきかなと思いました。
test/system/press_releases_test.rb
Outdated
|
|
||
| test 'show only published press releases' do | ||
| visit press_releases_path | ||
| assert_selector '.thumbnail-card.a-card', count: 7 |
There was a problem hiding this comment.
このテストの目的は「表示されるブログ記事がすべてpublishedであること」でしょうか?
であればカウント数でassertするのではなく、表示されているすべてのブログのpublished属性がtrueであることをテストしたほうがよさそうです。
There was a problem hiding this comment.
systemテストでpublished属性がtrueであることを確認するのは難しいかと...
There was a problem hiding this comment.
「難しい」とおっしゃる内容をもう少し詳しく教えていただいてもよいでしょうか?🙏
というのも読む側が7という数字を見ても、一体なにをテストしているのかがすぐに理解できないためです(いわゆるマジックナンバー)。
教えていただいた上で、私のほうでもよりよい書き方がないかいっしょに調査したいと思いますので!
There was a problem hiding this comment.
「難しい」とおっしゃる内容をもう少し詳しく教えていただいてもよいでしょうか?🙏
published_atはboolean型ではなくdate型なのでtrueかどうかの判定ができない点と、もし仮にpublished_atで確認するとして場合、その確認方法が思いつかなかったです。
下の写真は、実際のアプリケーションのブログ一覧ページですが、赤枠で囲ってる部分がpublished_atの値となります。HTML内では<div class="thumbnail-card__date">2025年02月25日(火) 02:41</div>となっており、published_atとしてのテストが難しいかなーと思いました。
というのも読む側が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テストって難しい😅)
There was a problem hiding this comment.
HTML内では
<div class="thumbnail-card__date">2025年02月25日(火) 02:41</div>となっており、published_atとしてのテストが難しいかなーと思いました。
なるほど!であればこのテキストの値を取り出して、Time型に変換すればassertできるかなと思いました。
ちなみに「publishedであること」というのは具体的にどのような状態を指すのでしょうか??
このテストの必要性を私が理解できていないためすみませんが教えていただけるとうれしいです🙏
There was a problem hiding this comment.
「publishedであること」というのは具体的にどのような状態を指すのでしょうか??
このテストの必要性を私が理解できていないためすみませんが教えていただけるとうれしいです🙏
publishedは初めて公開された日付ですね!
wip状態のプレスリリースは表示されないことを確認したいという目的です!
There was a problem hiding this comment.
publishedは初めて公開された日付ですね!
wip状態のプレスリリースは表示されないことを確認したいという目的です!
なるほど、ということはWIPの記事は上のスクショにある赤枠の日付は書かれないということでしょうか?
もしそうであればWIPかどうかはブラウザでthumbnail-card__dateクラスのtext属性がemptyかどうかを見るだけでassertできるかなと思ったのですが、いかがでしょうか?
test/system/press_releases_test.rb
Outdated
| class PressReleasesTest < ApplicationSystemTestCase | ||
| test 'show listing press releases' do | ||
| visit press_releases_path | ||
| assert_text 'プレスリリース' |
There was a problem hiding this comment.
「プレスリリース」のテキストの存在確認ではなく、titleが「プレスリリース」であるページに正しく遷移したかをテストするとよさそうです。
There was a problem hiding this comment.
下記のように変更します。
| assert_text 'プレスリリース' | |
| assert_equal 'プレスリリース', title |
| require 'application_system_test_case' | ||
|
|
||
| class PressReleasesTest < ApplicationSystemTestCase | ||
| test 'show listing press releases' do |
There was a problem hiding this comment.
1ページあたり最大30記事であることをテストしてもよいかと感じました。
There was a problem hiding this comment.
これも非常に悩みました。
articles_test.rbやuser/reports_test.rbなど表示数に制限があるページはいくつかあるのですが、私が確認する限り、最大の出力数はテストしていないようでした。
こちらももう少し調べてみます🫡
There was a problem hiding this comment.
最大の出力数をテストすることはわりと簡単かなと思っていて、Capybaraのallメソッドで各記事がもつクラスを指定してあげると配列を返すので、それをsizeやlengthで数えればよいかと思いましたが、いかがでしょうか??
There was a problem hiding this comment.
教えていただきありがとうございます!allメソッドを使用してテスト書いてみます!
There was a problem hiding this comment.
最大30件の件ですが、masyukoさんが日報にコメントをくださいましたので、参考にできそうです👍
- 30件はKaminariが担保している数値なのでシステムテストは不要
- ページネーションの機能自体もシステムテストでは書かない
There was a problem hiding this comment.
共有ありがとうございます!
システムテストをどこまで行うかは個人やチームの方針などで色々変わってきそうですね👀
There was a problem hiding this comment.
個人で決まるのはよくないと思うので、チームの方針を皆が見えるようにしてほしいですよね😅
test/system/press_kit_test.rb
Outdated
| require 'application_system_test_case' | ||
|
|
||
| class PressKitTest < ApplicationSystemTestCase | ||
| test 'show press releases' do |
There was a problem hiding this comment.
以下はテストすべきかなと思いました。
created_atの降順でなくpublished_atの降順であること- WIPは表示されないこと ←たとえばWIP記事のpublished_atを
Time.nowにするとか?
There was a problem hiding this comment.
確かにこれはテストするべきですね!ご指摘ありがとうございます。
あと、ページネーションに関してもテストしておいた方が良さそうなので、テストに関しては改めて考えてみます。
There was a problem hiding this comment.
ページネーションについてはやるべきかを先ほどQ&Aでスクラムマスターの駒形さんに質問をしましたので、そちらの回答を待つのでもよいかと思いますー🙆♂️
There was a problem hiding this comment.
他のファイルではページネーションのテストを書いているのもありましたので、並び順、ページネーションのテストを追加いたしました。
|
|
||
| def press_kit; end | ||
| def press_kit | ||
| @press_releases = Article.press_releases.includes(%i[user thumbnail_attachment]).order(published_at: :desc) |
There was a problem hiding this comment.
limitを使用してコントローラー側で6件に絞り込んだ方が良いですね。
ご指摘ありがとうございます!
| class PressReleasesController < ApplicationController | ||
| skip_before_action :require_active_user_login, raise: false, only: %i[index] | ||
|
|
||
| layout 'lp' |
There was a problem hiding this comment.
すみません、こちら何のためにあるコードなのか教えてもらえるとうれしいです🙏
There was a problem hiding this comment.
layout 'lp'の有無で何が変わるかは動画で見ると分かりやすいので下記ご確認ください。
2025-03-13.12.56.49.mov
lpはおそらく「ランディングページ (Landing Page)」のことを指していると思います。
未登録の状態でwelcomeページからアクセスできるサイトはlayout 'lp'で統一していそうです。
layout 'lp'を指定していない場合、views/layouts/application.html.slimが参照されます。
There was a problem hiding this comment.
動画とてもわかりやすかったです🙏
layout 'lp'とするとlp.html.slimに遷移するような情報も見かけたのですが、「そんなファイルはないな・・・」と疑問に思ったため質問させていただきました!
今後の開発の参考にさせていただきます😊
test/models/article_test.rb
Outdated
| articles = Article.press_releases | ||
|
|
||
| articles.each do |article| | ||
| assert_equal 'プレスリリース', *article.tag_list |
There was a problem hiding this comment.
こちら気になったので確認させてください!
*article.tag_listの配列展開ですが、リストの要素数が2つ以上('プレスリリース'とそれ以外)であっても問題なくテストは通るのでしょうか?
There was a problem hiding this comment.
確かにタグが2つ以上ある記事だとテスト通らないですね!ご指摘ありがとうございます!
assert_includesに変更いたします。
db/fixtures/articles.yml
Outdated
| <% end %> | ||
|
|
||
| <% date = Date.parse("2023-01-01") %> | ||
| <% (56..85).each do |id| %> |
There was a problem hiding this comment.
この範囲にidを指定している理由は、これより上に55(56?)のテストデータが存在するためでしょうか?(数字の意図がわからなかったため確認です)
ほか(5..12など)についても同様です。
There was a problem hiding this comment.
そうですね!どちらもidが被らないようにデータを作成するようにしています。
データ数に関してですが、(56..85)は30個のプレスリリースタグを持つブログ記事のデータが開発環境のDBに作成されます。
(5..12)は、8つ分のデータがある状態でテストが実行されるようになります。8つの理由は、7個は公開済みの記事で、残り1つはwipの記事です。
7個公開済みのデータに対してプレスキットのページでは6つしか表示されないことを確認するためのデータです。
8つ目のwipデータは、modelのテストでwipがないことを確認するために作成しています。
There was a problem hiding this comment.
@Ryooo-k
なるほど、ありがとうございます!
以下がまだ気になっていますので、教えていただいてもよいでしょうか🙏
- 必要な数についてはコメントを残すことは
.ymlファイルでは難しいものでしょうか?読む側として上で書かれたような情報はとても重要だと感じました! - 開始番号が
56や5になっている理由について。101..130や101..108の方が30件であること、8件であることがわかりやすいなと思ったのと、開始番号を大きい数にすることで今後テストデータが増えた場合のidかぶり(じつは考えなくてもよかったりする??)などを気にしなくてすむのかなと思いました。
There was a problem hiding this comment.
fixtureに関してですが、こちらのkomagataさんのコメントですが参考になったので共有します。
現状は、テストデータをfixtureで作成するようにしていますが、今回のケースのように特定のテストだけに使用するのであれば、テストメソッド内でテストデータを作成した方が良さそうでした。
今回のプレスリリースタグを持つブログ記事のテストは、「他の多くのテストでも使いそうなやつ」には該当しないと思うので、fixtureの追加は削除するようにしようかと考えています!
読む側として上で書かれたような情報はとても重要だと感じました!
101..130や101..108の方が30件であること、8件であることがわかりやすいなと思った...
こちら大変参考になりました✨読む側が分かりやすいようにするように気をつけます🫡
There was a problem hiding this comment.
fixtureに関してですが、こちらのkomagataさんのコメントですが参考になったので共有します。
これは勉強になります!
共有ありがとうございます🙏
であれば30件、8件である理由などもテストメソッド内にコメントとして自然に残せそうですね!
kitarou888
left a comment
There was a problem hiding this comment.
取り急ぎ気になった点をレビューしました!
Active Recordまわりは私が勉強不足で正しいかを判断できなかったため、これから勉強したのちに、改めてレビューさせていただければと思います〜🙏
|
@kitarou888 |
|
@Ryooo-k |
|
@kitarou888 修正完了しましたら、ご連絡いたします。 |
|
@Ryooo-k 私のアクションとしまして、以下引き続き進めていきます!
|
|
@Ryooo-k ※ちなみに教えていただいた |
test/system/press_kit_test.rb
Outdated
| assert_no_text wip_press_release.title | ||
| end | ||
|
|
||
| test 'press releases are displayed from the most recent publication date' do |
There was a problem hiding this comment.
この辺りはArticleモデルのメソッドとして用意してあればそちらのメソッドをテストすることでほとんど担保できるので実行速度の面からもそちらがいいかもです。
There was a problem hiding this comment.
承知しました!
モデルテストで、公開済みのブログのみを取得するテストと降順になっているかのテストを追加します。
下記システムテストは削除いたします。
- 公開順に画面表示されているかのテスト
- wip状態のプレスリリースは含まれていないかを確認するテスト
|
@komagata |
db/schema.rb
Outdated
| t.string "name" | ||
| t.string "slug" |
There was a problem hiding this comment.
意図的な変更でなければ変更しない状態がいいです。
(コマンドとかで勝手になったのだとは思いますが)
There was a problem hiding this comment.
承知しました。git revertで本コミットを取り消します。
test/system/press_kit_test.rb
Outdated
| assert_equal 'プレスキット | FJORD BOOT CAMP(フィヨルドブートキャンプ)', title | ||
| end | ||
|
|
||
| test 'show recent 6 press releases' do |
There was a problem hiding this comment.
こちらもmodelの方で確認できる(もしくはそういうメソッドがrailsの方にある)のであればsystem testでなくてもほとんど担保できると思うので、system testは不要かなと思います。
There was a problem hiding this comment.
scopeの引数でデータ取得数を指定できるようにし、modelテストでその挙動を確認するようにいたします。
|
@komagata |
test/system/press_releases_test.rb
Outdated
| assert_equal 'プレスリリース | FJORD BOOT CAMP(フィヨルドブートキャンプ)', title | ||
| end | ||
|
|
||
| test 'displays up to 24 articles' do |
There was a problem hiding this comment.
show listing press releasesのテスト以外はsystem testは不要かなと思います。
|
@komagata |
|
@Ryooo-k conflictの修正をお願いします〜 |
6788c1a to
ec078da
Compare
|
@Ryooo-k conflictの修正をお願いします。またrevertのコミットもあるので修正して下さい。 |
b466574 to
71cce18
Compare
|
@komagata |
|
@Ryooo-k Revertのコミットが含まれているので修正して下さい。Revertは基本的に緊急時以外使わないです。 |
71cce18 to
b158766
Compare
|
@komagata |
|
@Ryooo-k conflictの修正をお願いします〜 |
b158766 to
d84d702
Compare
|
@komagata |
|
ご確認ありがとうございます! |



Issue
概要
プレスキットのページ内に、プレスリリースタグを持つブログ記事を最大6つまで表示するようにしました。
また「全てのプレスリリース」ボタンを押すと、プレスリリースタグを持つブログ記事の一覧表示されるページにアクセスするようにしました。
変更確認方法
feature/show_latest_press_release_articles_in_presskitをローカルに取り込むi.
git fetch origin pull/8350/head:feature/show_latest_press_release_articles_in_presskitii.
git checkout feature/show_latest_press_release_articles_in_presskitbin/rails db:migrate RAILS_ENV=testでfixtureのデータをDBに取り入れるforeman start -f Procfile.devでローカルサーバーを立ち上げるScreenshot
変更前
変更後