facebook のイベント履歴収集＋登録処理のエラーを解消する

[chicaco]

背景

#371 & #375 マージ＆デプロイ後、本番環境で統計スクリプトを実行したが、facebook のイベント履歴収集＋登録処理でエラーになる。

やりたいこと

1. facebook のイベント履歴収集＋登録処理のエラーを解消する

Fix #395

このPRでやること

• facebook のイベント履歴収集＋登録処理のエラーの原因を調査する
• エラーを解消する
• RSpec 追加

cf. #396 (comment)

やらなかったこと

特になし

レビューポイント

• find_by の一意条件を dojo_id, name, group_id に限定し、url を除外すること
• すでに多重登録が起きている DB に対して再実行したとき、多重登録を解消する処理

困ってること

特になし

[chicaco]

原因

本番 DB の dojo_event_services T に group_id が同じで URL が異なる facebook イベントのレコードが存在することが、facebook イベントを yaml から読み込むよう修正した処理で悪影響を及ぼすようになったためでした。

対処

rake dojo_event_services:upsert で二重登録が起きないように、以下の方針で修正します。

1. find_by の一意条件を dojo_id, name, group_id に限定し、url は除外
2. 既に多重登録となっているレコードは、これを解消する

動確状況

(1) 準備 : 本番DBをローカル環境にリストア
(2) rake dojo_event_services:upsert 実行 → 多重レコード解消
(3) rake statistics:aggregation[201801,201902,facebook] 実行 → 正常終了

[chicaco]

RSpec 追加したので、WIP を外しました。

[nalabjp]

destroy_allしている処理は既知の問題となっている多重登録を解消するための処理でしょうか？
そして、それを解消した後は多重登録は発生しない想定でしょうか？
もしそうであるなら、destroy_allの処理は今回のみ必要な実装だと思うので、
書き捨てのスクリプトかrake taskにして既存の実装には追加しない方が良いのではと思いました。

多重登録になった直接的原因がIssueを見てもちょっとよく分からなかったので、
外した事を言っているような気もしています。😅

[chicaco]

コメントありがとうございます！
順序を入れ替えてコメント追記します。

多重登録になった直接的原因がIssueを見てもちょっとよく分からなかったので、
外した事を言っているような気もしています。😅

多重登録は、2018/12/ 2 の a917540 のコミットで facebook の URL から ?ref=page_internal を削除しようとしたときに起こったようです。
find_or_initialize_by の一意条件が dojo_id, name, group_id,url だったので、異なる url を指定したために別レコードが作成されてしまいました。
url を変更(≒訂正)したかったのだと思いますが...。

destroy_allしている処理は既知の問題となっている多重登録を解消するための処理でしょうか？
そして、それを解消した後は多重登録は発生しない想定でしょうか？

前者は YES、後者もたぶん YES です。

もしそうであるなら、destroy_allの処理は今回のみ必要な実装だと思うので、
書き捨てのスクリプトかrake taskにして既存の実装には追加しない方が良いのではと思いました。

これは迷ったところです。
こちらの rake task にも修正が必要なことがわかったので、押し込んでしまいました。
書捨てのスクリプトか rake task でも大丈夫だと思います。

少し脱線しますが、
書捨てなスクリプトか rake task ですと、一度コミット＆マージして、本番環境で実行後、別 PR で削除する、という手順になるでしょうか？
(ちょっと面倒かしら → 害のないコードなのでここに入れといてもいっか、というちょっと浅はかな思考でした)

※ 追記
多重登録を解消するための一番簡単なやり方は、dojo_event_services T のレコードを全削除して、rake dojo_event_services:upsert を再実行すること、です。
ローカル環境では多重登録が起きない yaml を用いての rake dojo_event_services:upsert だったので、本番環境でしか起きないという現象になっていました。

[nalabjp]

@chicaco

なるほど、詳細な説明ありがとうございます。

書捨てなスクリプトか rake task ですと、一度コミット＆マージして、本番環境で実行後、別 PR で削除する、という手順になるでしょうか？

いくつかのやり方があると思います。
例を挙げると、

• rails consoleで実行する
  • 特定のデータに対して決め打ちで実行したい場合
  • Issueに手順を書いてレビュー後にrails consoleで実行、実行結果をIssueに追記して残す
• 書捨てのスクリプトにする
  • rails consoleで実行するのと同じ条件ですが、こちらはコミットに残る
  • 適当なディレクトリを掘ってそこにスクリプトを置く（実行後に捨てる捨てないはどちらでも）
• rake taskにする
  • 決め打ちじゃなく汎用的なツールになりそうなら
  • 条件に合わせてお掃除するようなものは今後も使えるかもしれない

みたいな感じで使い分けたりしています。
参考になりますでしょうか。

[chicaco]

@nalabjp コメントありがとうございます。
この開発スタイルに不慣れなので、とても参考になりました。 🙇‍♀️

今回の場合、テーブル内のレコード全削除でも対処可能なので、rails console でよさそうな気がします。
スクリプトに残すほど複雑なことも行いませんし、汎用化することも再利用することもなさそうなので。

以下の手順で対応いたします。

1. こちらの destroy_all 処理を削除してレビュー通過した後で
2. Issue 化して削除の手順を記載してレビューいただき、
3. 本 PR をマージ → rails console で削除実行 → rake dojo_event_services:upsert も実行
4. 実行結果を Issue に追記

修正コードを PUSH 後に(RSpec も修正しますので、早くても今夜です)、改めてレビュー依頼いたします。
よろしくお願いします。

[chicaco]

yaml に書かれていない(＝削除したい) dojo_event_service を削除していない、という考慮漏れに気付いてしまいました...。
基本的には単純追加で、稀に URL 修正がある、という想定でしたが。
書かれていない dojo_event_service を削除するようにすれば、一旦全削除の手順も不要です。

[chicaco]

@nalabjp お手隙のときに再レビューお願いします。
1 つ上のコメントに記載した通り考慮漏れがあり、これに対応して昨日より作戦変更しました。

多重登録を解消するだけでなく、yaml から削除された(レアケースだとは思いますが)レコードは削除する、つまり、yaml で指定されているレコードのみにするように見直しました。
結果、rails console による削除実行は不要になりました。

[nalabjp]

LGTM 👍