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

記事ごとの登録日時を持たない場合も例外が発生しないよう修正 #6941

Merged
merged 5 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 49 additions & 18 deletions app/models/external_entry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ class << self
def fetch_and_save_rss_feeds(users)
threads = Concurrent::Promises.future do
users.each do |user|
rss_items = ExternalEntry.parse_rss_feed(user.feed_url)
feed = ExternalEntry.parse_rss_feed(user.feed_url)

next unless rss_items
next unless feed

rss_items.each do |item|
feed.items.each do |item|
case item.class.name
when 'RSS::Atom::Feed::Entry' then ExternalEntry.save_atom_feed(user, item)
when 'RSS::Rss::Channel::Item' then ExternalEntry.save_rss_feed(user, item)
when 'RSS::RDF::Item' then ExternalEntry.save_rdf_feed(user, item)
when 'RSS::Atom::Feed::Entry' then ExternalEntry.save_atom_feed(user, item, feed.updated&.content)
Copy link
Contributor

@sochi419 sochi419 Oct 18, 2023

Choose a reason for hiding this comment

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

&.を使った記述がありますが、これはどういった役割なのでしょうか?(初めて見ました)

Copy link
Contributor Author

@siso25 siso25 Oct 18, 2023

Choose a reason for hiding this comment

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

レシーバーがnilのとき(今回であればfeed.updatedの戻り値がnilのとき)、メソッドを呼び出してもNoMethodErrorが発生しなくなります。
RSSやAtomの仕様上必須でない項目については、フィードに含まれていないことがあります。今回の場合はエラーを出すよりもスキップしたい(nilを返してほしい)ので、こちらの書き方にしています。

https://docs.ruby-lang.org/ja/latest/doc/news=2f2_3_0.html

Copy link
Contributor

Choose a reason for hiding this comment

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

なるほどですね、ぼっち演算子というものなんですね~
レシーバがnilの時にエラーが出ないようにするためのものなんですね👀

ご教示ありがとうございます🙏

when 'RSS::Rss::Channel::Item' then ExternalEntry.save_rss_feed(user, item, feed.channel&.lastBuildDate)
when 'RSS::RDF::Item' then ExternalEntry.save_rdf_feed(user, item, feed.channel&.dc_date)
end
end
end
Expand All @@ -38,50 +38,81 @@ def parse_rss_feed(feed_url)
return nil if feed_url.blank?

begin
RSS::Parser.parse(feed_url).items
RSS::Parser.parse(feed_url)
rescue StandardError => e
logger.warn("[RSS Feed] #{feed_url}: #{e.message}")
nil
end
end

def save_rdf_feed(user, rdf_item)
def save_rdf_feed(user, rdf_item, feed_updated)
return if ExternalEntry.find_by(url: rdf_item.link)

published_at = rdf_publish_date(rdf_item, feed_updated)
ExternalEntry.create(
title: rdf_item.title,
url: rdf_item.link,
summary: rdf_item.description,
thumbnail_image_url: nil,
published_at: rdf_item.dc_date,
published_at: published_at.utc? ? Time.zone.parse(published_at.to_s) : published_at,
user:
)
end

def save_rss_feed(user, rss_item)
def save_rss_feed(user, rss_item, feed_updated)
return if ExternalEntry.find_by(url: rss_item.link)

published_at = rss_publish_date(rss_item, feed_updated)
ExternalEntry.create(
title: rss_item.title,
url: rss_item.link,
summary: rss_item.description,
thumbnail_image_url: rss_item.enclosure&.url,
published_at: rss_item.pubDate,
published_at: published_at.utc? ? Time.zone.parse(published_at.to_s) : published_at,
user:
)
end

def save_atom_feed(user, atom_item)
return if ExternalEntry.find_by(url: atom_item.link.href)
def save_atom_feed(user, atom_item, feed_updated)
return if ExternalEntry.find_by(url: atom_item.link&.href)

published_at = atom_publish_date(atom_item, feed_updated)
ExternalEntry.create(
title: atom_item.title.content,
url: atom_item.link.href,
summary: atom_item.content.content,
thumbnail_image_url: atom_item.links.find { |link| !link.type.nil? && link.type.include?('image') }&.href,
published_at: atom_item.published.content,
title: atom_item.title&.content,
url: atom_item.link&.href,
summary: atom_item.content&.content,
thumbnail_image_url: atom_image_url(atom_item),
published_at: published_at.utc? ? Time.zone.parse(published_at.to_s) : published_at,
user:
)
end

private

def rdf_publish_date(rdf_item, feed_updated)
return rdf_item.dc_date if rdf_item.dc_date
return feed_updated if feed_updated

Time.zone.now
end

def rss_publish_date(rss_item, feed_updated)
return rss_item.pubDate if rss_item.pubDate
return feed_updated if feed_updated

Time.zone.now
end

def atom_image_url(atom_item)
atom_item.links&.find { |link| !link.type.nil? && link.type.include?('image') }&.href
end

def atom_publish_date(atom_item, feed_updated)
return atom_item.published.content if atom_item.published
return atom_item.updated.content if atom_item.updated
return feed_updated if feed_updated

Time.zone.now
end
end
end
101 changes: 95 additions & 6 deletions test/models/external_entry_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,29 +32,84 @@ class ExternalEntryTest < ActiveSupport::TestCase
rdf_item.expect(:link, 'https://test.com/index.rdf')
rdf_item.expect(:description, 'article description')
rdf_item.expect(:dc_date, Time.zone.local(2022, 1, 1, 0, 0, 0))
rdf_item.expect(:dc_date, Time.zone.local(2022, 1, 1, 0, 0, 0))

assert ExternalEntry.save_rdf_feed(user, rdf_item, nil)
end

test '.save_rdf_feed with no article publish date' do
user = users(:hatsuno)
rdf_item = Minitest::Mock.new
rdf_item.expect(:link, 'https://test.com/index.rdf')
rdf_item.expect(:title, 'test title')
rdf_item.expect(:link, 'https://test.com/index.rdf')
rdf_item.expect(:description, 'article description')
rdf_item.expect(:dc_date, nil)

assert ExternalEntry.save_rdf_feed(user, rdf_item, nil)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

記事の公開日が存在しない場合に正しく動作するかを検証するためのtestとは思うのですが、すいません、私が初学者ということもあり、Minitest::Mockを使ったテスト方法が難しくて良く分かっておらず、勉強のためにこの箇所のtestだけ何をやっているか1行ずつ解説していただけるとありがたいです🙏

Minitest::Mockを使ったtestは大体似たような記述に見えるので、1つだけ理解できれば、残りのmocktestも理解できると思うので、よろしくお願いします🙏

Copy link
Contributor Author

@siso25 siso25 Oct 18, 2023

Choose a reason for hiding this comment

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

メソッド内で引数で渡したオブジェクトのインスタンス変数を参照したり、インスタンスメソッドを実行したりする際に、想定通りの値を返すようにすることがMockを使用している目的です。
以下のようなメソッドをテストする際にmock.expect(:link, 'https://test.com/index.rdf')を設定して引数として渡すと、メソッド内でrdf_item.linkを実行した際にhttps://test.com/index.rdfという文字列が返ってくる、という動きになります。

def save_rdf_feed(rdf_item)
  return if ExternalEntry.find_by(url: rdf_item.link) # rdf_item.link → 'https://test.com/index.rdf'が返される

  ExternalEntry.create(
    title: rdf_item.title,
    url: rdf_item.link
  )
end

また、mock.expectの内容はメソッド内で呼び出す順番に設定する必要があり、上記のメソッドの場合はrdf_item.linkを2回呼んでいるため、以下のようにmock.expected(:link, 'xxx')を2回設定する必要があります。

mock = Minitest::Mock.new
mock.expected(:link, 'https://test.com/index.rdf')
mock.expected(:title, 'test title')
mock.expected(:link, 'https://test.com/index.rdf') # もう一度設定しないとエラーになる

別のところに出てくると思うのですが、値を取得する際にitem.link.hrefのようにネストする場合は、以下のように記述しています。

mock = Minitest::Mock.new
mock.expected(:link, mock) # mock自身を返すようにする
mock.expected(:href, 'https://test.com/index.rdf')

Copy link
Contributor

Choose a reason for hiding this comment

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

解説ありがとうございます🙏
mockの使い方について、どういう使い方をするのか理解できました!


test '.save_rdf_feed no exception even if all rdf elements are nil' do
user = users(:hatsuno)
rdf_item = Minitest::Mock.new
rdf_item.expect(:link, nil)
rdf_item.expect(:title, nil)
rdf_item.expect(:link, nil)
rdf_item.expect(:description, nil)
rdf_item.expect(:dc_date, nil)

assert ExternalEntry.save_rdf_feed(user, rdf_item)
assert ExternalEntry.save_rdf_feed(user, rdf_item, Time.zone.local(2023, 1, 1, 0, 0, 0).utc)
end

test '.save_rss_feed' do
Copy link
Contributor

Choose a reason for hiding this comment

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

test名の最初に.をつける場合とそうでない場合があるのですが、どういった場合に.をつけるのでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sochi419
クラスメソッドに対するテストには.を付けているのですが、今回追加したのもクラスメソッドに対するテストなので、テスト名称を修正します。

Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど、クラスメソッドに対するテストには.をつけるのですね👀
勉強になりました✍️

クラスメソッドに対するテストには.を付けているのですが、今回追加したのもクラスメソッドに対するテストなので、テスト名称を修正します。

PR上ではまだ修正されていないので、これから修正する感じですかね?よろしくお願いします🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

すみませんが、メソッド名を修正しましたので、念のためこちらだけ確認をお願いできればと思います。

Copy link
Contributor

Choose a reason for hiding this comment

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

修正ありがとうございます!
命名を修正したこと、確認できました👀

user = users(:hatsuno)
rss_item = Minitest::Mock.new
rss_item.expect(:link, 'https://test.com/rss')
rss_item.expect(:pubDate, Time.zone.local(2022, 1, 1, 0, 0, 0))
rss_item.expect(:pubDate, Time.zone.local(2022, 1, 1, 0, 0, 0))
rss_item.expect(:title, 'test title')
rss_item.expect(:link, 'https://test.com/rss')
rss_item.expect(:description, 'article description')
rss_item.expect(:enclosure, rss_item)
rss_item.expect(:url, 'https://test.com/image.png')
rss_item.expect(:pubDate, Time.zone.local(2022, 1, 1, 0, 0, 0))

assert ExternalEntry.save_rss_feed(user, rss_item)
assert ExternalEntry.save_rss_feed(user, rss_item, nil)
end

test '.save_rss_feed with no article publish date' do
user = users(:hatsuno)
rss_item = Minitest::Mock.new
rss_item.expect(:link, 'https://test.com/rss')
rss_item.expect(:pubDate, nil)
rss_item.expect(:title, 'test title')
rss_item.expect(:link, 'https://test.com/rss')
rss_item.expect(:description, 'article description')
rss_item.expect(:enclosure, nil)

assert ExternalEntry.save_rss_feed(user, rss_item, Time.zone.local(2023, 1, 1, 0, 0, 0).utc)
end

test '.save_rss_feed no exception even if all rss elements are nil' do
user = users(:hatsuno)
rss_item = Minitest::Mock.new
rss_item.expect(:link, nil)
rss_item.expect(:pubDate, nil)
rss_item.expect(:title, nil)
rss_item.expect(:link, nil)
rss_item.expect(:description, nil)
rss_item.expect(:enclosure, nil)

assert ExternalEntry.save_rss_feed(user, rss_item, nil)
end

test '.save_atom_feed' do
user = users(:hatsuno)
atom_item = Minitest::Mock.new
atom_item.expect(:link, atom_item)
atom_item.expect(:href, 'https://test.com/feed')
atom_item.expect(:published, atom_item)
atom_item.expect(:published, atom_item)
atom_item.expect(:content, Time.zone.local(2022, 1, 1, 0, 0, 0))
atom_item.expect(:title, atom_item)
atom_item.expect(:content, 'test title')
atom_item.expect(:link, atom_item)
Expand All @@ -65,10 +120,44 @@ class ExternalEntryTest < ActiveSupport::TestCase
atom_item.expect(:find, atom_item)
atom_item.expect(:nil?, atom_item)
atom_item.expect(:href, 'https://test.com/image.png')
atom_item.expect(:published, atom_item)
atom_item.expect(:content, Time.zone.local(2022, 1, 1, 0, 0, 0))

assert ExternalEntry.save_atom_feed(user, atom_item)
assert ExternalEntry.save_atom_feed(user, atom_item, nil)
end

test '.save_atom_feed with no article publish date' do
user = users(:hatsuno)
atom_item = Minitest::Mock.new
atom_item.expect(:link, atom_item)
atom_item.expect(:href, 'https://test.com/feed')
atom_item.expect(:published, nil)
atom_item.expect(:updated, nil)
atom_item.expect(:title, atom_item)
atom_item.expect(:content, 'test title')
atom_item.expect(:link, atom_item)
atom_item.expect(:href, 'https://test.com/feed')
atom_item.expect(:content, atom_item)
atom_item.expect(:content, 'article description')
atom_item.expect(:links, atom_item)
atom_item.expect(:find, atom_item)
atom_item.expect(:nil?, atom_item)
atom_item.expect(:href, 'https://test.com/image.png')

assert ExternalEntry.save_atom_feed(user, atom_item, Time.zone.local(2023, 1, 1, 0, 0, 0).utc)
end

test '.save_atom_feed no exception even if all atom elements are nil' do
user = users(:hatsuno)
atom_item = Minitest::Mock.new
atom_item.expect(:link, nil)
atom_item.expect(:published, nil)
atom_item.expect(:updated, nil)
atom_item.expect(:title, nil)
atom_item.expect(:content, nil)
atom_item.expect(:link, nil)
atom_item.expect(:content, nil)
atom_item.expect(:links, nil)

assert ExternalEntry.save_atom_feed(user, atom_item, nil)
end

test '.fetch_and_save_rss_feeds' do
Expand Down