直近のイベント情報を集計するtaskの追加(仕切り直し) #459
Conversation
・関連見直し ・リファクタ ・不要なコメント削除
db/schema.rb
Outdated
| t.string "event_id", null: false | ||
| t.string "event_url", null: false | ||
| t.datetime "event_at", null: false | ||
| t.integer "dojo_event_service_id", default: 1, null: false |
There was a problem hiding this comment.
migrate <-> rollback したときのゴミでした...。申し訳ありません。
| remove_index :upcoming_events, :event_at | ||
|
|
||
| add_column :upcoming_events, :dojo_name, :string, null: false | ||
| add_column :upcoming_events, :service_name, :string, null: false |
There was a problem hiding this comment.
ちょっと思慮不足で見落としていたのでいまさらで申し訳ないんですが、
dojo_nameとservice_nameが非正規化されているのってどういう意図でしょうか？
正規化のほうがされている方が良さそうに思ったので、お聞きしたいです〜。
There was a problem hiding this comment.
dojo_name を持っているは直近のイベント一覧を表示するときに、dojo レコードまで辿らなくても表示できるかな、という目論見によります。
service_name は service_name + event_id でユニーク制約を付けたかったので持ちました。
後者については、関連する(親の)レコードの attribute (今回の場合は service_name or service_id)と自レコードの attribute (今回の場合は event_id) でのユニーク制約を設定する方法があれば、その方がよいと思っています。
There was a problem hiding this comment.
履歴テーブルの場合は過去の不変の情報なので非正規化、未来の予定に関するテーブルの場合は変更に強い情報の持ち方で正規化、というのが良いかなと思っています。
このテーブルで扱ってる情報としてはあまりないユースケースになるとは思いますがdojo_nameが変わってしまうケースは変更に追従できなさそうに思います。
service_name は service_name + event_id でユニーク制約を付けたかったので持ちました。
こちらはこれからユニーク制約が追加されるという事になりますか？
後者については〜
についてはちょっと理解が追いつきませんでした。
ユニーク制約がないと困るケースが想像できなかったので、例を挙げて貰えると助かります。
There was a problem hiding this comment.
dojo_name については、ご指摘の通りです。
少し楽をしようと思ってしまいましたが、正しい姿ではありませんよね。
となると、毎週 Update していく UpcomingEvent のレコードよりも、一旦収集したらそのまま残りそうな EventHistory のレコードの方が気になってしまいました...。
service_name + event_id でのユニーク制約は、ActiveRecord レベルでは設定していたのですが、SQL レベルでは設定できていないという不十分な状態でした。
対応漏れですので、追記します。
connpass や doorkeeper という service_name + event_id で UpcomingEvent が一意になるという前提で 2 ヶ月後までのイベント情報を upsert していますので、ユニーク制約は必要であると考えました。
There was a problem hiding this comment.
event_historiesは履歴なのでそのままで大丈夫です。
過去の事実を記録しているので、仮に現在のdojo_nameと差異があったとしてもそれを最新に更新すると事実と異なる事が履歴にあることになってしまいます。
あ、event_idだけだと、重複しないと言い切れないからってことですか。
event_urlでも事足りませんかね、ユニーク制約🤔
まあ、service_nameが変わることはほぼ無いと思うので、問題があるわけではないですが。
There was a problem hiding this comment.
「event_historiesは履歴」なのですが、イベントが開催されたときの Dojo名 ではなく、統計情報を収集したときの Dojo 名ですよね。イベント開催時でも現在でもないので、気になりました。
connpass, doorkeeper のみでしたら event_url でユニーク制約でよいのですが、
static_yaml や現 event_histories でやむなく採用している facebook の yaml になりますと URL を持たないので断念しました。
URL は「直近のイベント一覧」で表示したいので、そのまま URL として有効な値を設定しておきたいと考え、yaml 用にダミーでユニークになりそうな文字列を生成するような対応も採りませんでした。
yaml からの更新の場合は、event_id も独自採番になるのですが... (ここ、あまり考慮できていなかったです)
There was a problem hiding this comment.
イベントが開催されたときの Dojo名 ではなく、統計情報を収集したときの Dojo 名ですよね。イベント開催時でも現在でもないので、気になりました。
そこまでハンドリングするのはさすがに難しそうな印象ですねぇ🤔
facebook の yaml になりますと URL を持たないので断念しました。
なるほど、そういう事情なんですね、理解しました。
ありがとうございます〜。
There was a problem hiding this comment.
ひとつ訂正です。
facebook イベントの yaml からの読み込みの場合、event_id は facebook 側のイベントから拾えますし、イベントの URL もあります。
static_yaml の場合だけが URL も event_id もありません。直近のイベント情報収集で static_yaml に対応するときに注意が必要になります。
| @client.fetch_events(@params.merge(series_id: dojo_event_service.group_id)).each do |e| | ||
| next unless e.dig('series', 'id').to_s == dojo_event_service.group_id | ||
|
|
||
| record = dojo_event_service.upcoming_events.find_by(event_id: e['event_id']) |
There was a problem hiding this comment.
[nits]
find_or_initialize_byじゃなくなったのってどういう意図でしょうか？
そんなに強い主張もないんですがあえて冗長になったのが気になったので〜。
There was a problem hiding this comment.
has_many で追加される関連のメソッドに find_or_initialize_by はないと思い込んでいたのですが、試してはいませんでした。
使えるのであれば find_or_initialize_by の方がよいですね。
確認してみます。
There was a problem hiding this comment.
関連に対しても find_or_initialize_by が使用でき、RSpec もコマンド実動も問題なかったので修正しました。
・schema.rb の差し替え ・関連に対して find_or_initialize_by が使えたのでリファクタ
・UpcomingEvent から dojo_name カラムを削除 ・UpcomingEvent に service_name + event_id でユニーク制約を追加
背景
直近の CoderDojo 開催情報を表示したい
やりたいこと
cf #258
※ 使用する EventService モジュールの I/F を変更したため、PR #297 から引き継いで仕切り直す
このPRでやること
やらなかったこと
この PR では、対象 issue (#258) の以下は行わない (別 PR とする)
レビューポイント
困ってること
特になし