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

[WIP]直近のイベント情報を集計するtaskの追加 #297

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@AnaTofuZ
Copy link
Member

AnaTofuZ commented Mar 20, 2018

関連issue

#258

行うこと

  • 保存する用のschemeを定義 ( #287 )
  • 実際に保存するタスクの追加

方針

statisticsのagressionのコードを読みながら、使わない機能(月ごとの集計)などを排除していてコードを書いていく

ユースケース例

そういえば明日明後日明々後日って東京行くんだった

CoderDojoあるかな?

— 3Dダンボー (@3Ddanbo) August 15, 2018

AnaTofuZ added some commits Mar 20, 2018

コピーとメソッド等の修正
Statisticsの資産を使いたかった為にコピーして大まかなメソッドをリネームし
た
Statisticsからコピー
そのままrun系のメソッドが使えそうな気持ちだった為コピーしてきた
@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Mar 22, 2018

あ、一点確認なんですが、「直近のイベント情報」って統計情報にページに影響ありますか? 🤔
https://coderdojo.jp/stats

統計情報ではなるべく 事実のみ を反映させたいので、まだ開催された事実のないイベント (直近のイベント) については、統計情報とは別にできるといいなと考えています >< 💦 例えば、直近のイベントが統計情報にカウントされてしまうと、何らかの理由でキャンセルされたイベントも統計情報にカウントされてしまう (事実ではない情報が紛れ込んでしまう) ことを懸念しています 💭

@AnaTofuZ

This comment has been minimized.

Copy link
Member Author

AnaTofuZ commented Mar 22, 2018

統計情報ページには関係が無いです!
今のところ統計情報のページで行っている処理に近い形 (各イベントサービスからAPIを経由して日時を取得)で情報を入手し、別のスキーマに保存させようとしています!
なので直近のイベントが統計情報に加算される事はない感じです

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Mar 22, 2018

統計情報ページには関係が無いです!

おぉ! さすがですね 😸 ちょっと気になったので、確認できてよかったです ;)

AnaTofuZ added some commits Mar 22, 2018

UpcomingEventを保存するように修正した
connpassで作成したフォーマットに合わせてfacebookを修正した
UpcomingEvent modelを定義した
schemeを定義していたが利用するmodelを定義していなかった為
event_historyを参考にmodelを定義した。

@aokabin aokabin self-assigned this Apr 4, 2018

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 4, 2018

@AnaTofuZ 僕の方で引き継ぎます、よろしくお願いしますー!

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Apr 4, 2018

お願いします!😆 何か気になることがあれば気軽に聞いてくださ〜い 😼👌

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

思考パターンメモ(ログコピペ)

やりたいことは/eventsページにて、イベント情報が見たいとのこと

何を知ればいい??

  • event情報はどこから持ってくるべきなのか?
  • どうやって持ってくるべきなのか?
  • そのイベント情報を/eventに表示するにあたって、現状どこまでできているのか?

本当にこの順番で理解すればいいのか???

現状作られている部分を徐々に紐解いていって「どういう設計にしようとしているか」を理解する必要があるかな
というのも、残されている文章だけを読んでも何をしたいのかは把握しづらい
かつ、新しく作るわけではなく、作っている途中のものを作り上げていくので、外形を把握する方が早そう

というわけで、まずはどんな感じのものができているのかを確認してみる

/eventsにアクセスしてみる

Docker、毎回データ消えるので、Documents/DockerData/CoderDojoPGにてvolumeをマウントした

その上で、 bin/setup を実行
DBを用意できたので、rails sしてサーバー起動してみるも、scrivitoのエラー
前に安川さんからもらったやつでexportを実行してみる
これで動くなら、.envとかで管理したいよね
.envはdotenvのgemを使っているのではなく、.envファイルをignoreしているだけだった
でも.envrcってあったなー、何だろうこれ、調べたい added

.envにexport書いて、source .env して読み込んでrails sしてみた
再度 /events を確認してみる

Scrivito::ResourceNotFound in CmsController#index
Could not find Obj with permalink 'events'

エラー、ぬーん
確実にScrivitoに関するエラーである...
ScrivitoのResourceNotFoundである。。。
Scrivitoが何かわからないと何のリソースなのかも対応できなさそう...?
というわけでScrivitoを調べてみるか?

いや、まずはroutesからどのController#Actionが呼ばれているのかを確認すべきかも
そこから解決策を見いだせるかも
というわけでroutes.rb読んでみる...
e。。。eventsが...ない...?
やっぱScrivitoちょっと特殊かも、調べてみよう
Qiita::Team見てみるか

https://yasslab.qiita.com/nanophate/items/dac9a44da00603e31e0a
https://yasslab.qiita.com/nanophate/items/6ed6273dfa749dd8ef6b
nanophateくんのいい感じの記事が2つ!
クラウドCMS、RailsアプリをCMS化できるよ、とのこと、ほえー
じゃあやっぱCMSみたいなページが現れてくるのかな

何者かはわかったけど、原因は不明やなー
そういえばアナグラくんが /events の作成、みたいなPR作ってた気が
それを確認してみようか
#270
→ mergeじゃなくてcloseだった
一応最後のコメント
一旦名前空間の切り出しとしてPRを作る方が粒度的に良いと思われるのでcloseします
どういうことだろう

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

ここからはこまめに書いて行きます

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

#270 のPRClose後もいくつかのPRが関連としてあり、mergeされていたので、そのあたりを確認してみる

#276#287

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

#276 について

イベント情報は、統計情報とは別途管理したく、そのためのPRっぽい
なるほどー、ここでEventService::Providersというネームスペースとすることを決めた感じか
最初にアナグラくんがAPI/Providersを提案していたのが気になる、このイベント情報はAPIなのかもしや??
という疑問を持ちつつ、コードを読む

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

lib以下にevent_service.rbを配置している
ここで require_relative に関する知識がないので調査
ほー、ファイルの参照方法が違う感じなんかー、知見を得た
cf. require と require_relative って何が違うの?

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

lib/event_service/providers/*.rb では、イベント情報を取りに行くクラスが定義されていることがわかる
(moduleってこうやって書くんだ...)
そして今度は loop do 構文を知らないので調べる、無限ループすんのかなとか思う
💭 (...無限ループじゃん)
lib配下のプログラムがどうロードされるとかどう実行されるかとかを知らないので、パッと見は「いっぱい処理してヤバそう(小並感)」という感じの処理に見える

そして、先ほどのAPI/ProvidersEventService::Providers 問題の疑念が晴れる
APIを叩くためのプログラムだからAPIというネームスペースが候補として出て来たけど
実態はイベント情報を提供するサービスへの情報取得だからEventServiceが採用された感じ、と、一旦納得
そしてProviderも単なる「提供者」と言うよりは「接続事業者」的な意味合いだとするとしっくりくる

ループの疑問は一旦置いといて、どう言う処理しているかを読もう

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

connpass.rbとdoorkeeper.rbは同じsearchメソッド生やしていて、interface?(引数等々)も同じ
もし自分が実装するとしたら Providers.doorkeeper_search("イベント名") 的な実装をしてしまいそうなので、これは参考になる実装と思った
細かく学んで行くスタイル

でもそのsearchメソッド一つとっても引数の書き方が初見 def xxx(hikisuu:)
引数の末尾の:がわからない、というわけで調べる
🤔 💭 (こういうこと調べることに時間使って大丈夫かな...何か注意されたら辞めよ)
通常使わなさそうな記法だと思ったら、調べるワードは「ruby 関数 trick」とかになる

お、これでは? Rubyのメソッドの引数受け渡しまとめ#キーワード引数

ということは、search(keyword:) はデフォルトの値無しのキーワード引数ってこと?という仮説
そうっぽかった、なるほど

後から気づいた、Qiita::Teamで調べると、simanさんが既に書いてた
https://yasslab.qiita.com/siman/items/abeab654b4a8de47c4f4

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

もうloop doをパッと見しかしてないことがわかるよね、全然break文書いてあるよね、うん
早とちりするの、よくないね 🙇

そこまで来ると、EventService::Providersに関してはだいたい分かってきた
EventService::Providersはそれぞれイベント情報を取得するのに利用する

Providersがどこで使われているかは今の所わからないが
それぞれのクラスメソッドではevent情報を返していることはわかる

そしてstatisticsの統計情報のネームスペースでは、tasksディレクトリが用意されているので
もしかしてcronか何かで定期的に情報を取得している?ということが予想される

次は #287 あたりを読む

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 13, 2018

#287 はすごいシンプル
近々あるイベント(upcoming_event)のDBスキーマを用意してあげてるだけだ
UpcommingEventの使われている場所を見れば色々わかりそう

使っているところを見てみて、次は本PRのコードの変更を読んでみよう!

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Apr 13, 2018

もしかしてcronか何かで定期的に情報を取得している?ということが予想される

ですね ;) 次のような感じで毎週月曜日に統計スクリプトが走っています 🤖 💨

ss

cf. 統計情報 - CoderDojo Japan
https://coderdojo.jp/stats

集計について
毎週月曜日の午前10時頃に集計をしています。

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 17, 2018

今気づきました。。。 🙇
なるほどですねー!参考にいたします!!

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 17, 2018

#287 で定義されたDBスキーマはマージされているけど、modelとしてrails内で扱えるようにはなってなかったっぽい
それをこのPRのupcoming_event.rbで利用できるようにしてる様子

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 17, 2018

ちょっと順を追って理解していくために、commitの変更を1つ1つ読んでいこうかな
まずは lib/upcoming/aggregation.rb
どうやらstatisticsのコピーっぽいので、それぞれのdiffを取ってみる
c.f. diff コマンド見辛かった

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 17, 2018

ある程度コードは削られているっぽい、ということはわかった←
関数が読みやすくてありがたいなー、絶対 def run がどこかで走ってるじゃんー
と思って一気に検索してみるも、全然有益な情報得られず...

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Apr 17, 2018

runメソッド何してるのかなーと思って読むと
"Upcoming::Aggregation"のinner class(というのかな?)のrunメソッドを呼んでいることがわかる
どこで定義してるんだー!と思ってたら、普通に下の方に定義してあった...笑

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Jun 26, 2018

ん...?upcomingの集計にはeventページの名前とかいらないのかな...?

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Jun 26, 2018

この状態ではどうやらうまく動かない、UpcomingEventの削除の時に、なんとかDojoEventService経由で削除するようにしないといけなさそう

そのとっかかりにありそうなのは、upcoming_event.rbのfor scope

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Jul 23, 2018

こちらに戻っております

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Jul 23, 2018

upcoming_event.rbにfor scopeを入れてたけど、実際のところ、このupcoming_event自体はdojo_event_serviceに紐づくものなので、for scopeいらないかも

ただ、手元でrakeファイルに書いちゃってるので、そこを修正したい

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Jul 23, 2018

rakeファイルじゃなくて、upcoming/tasks/connpass.rbとかに書いてるっぽかった

これからforを外す、ただ、DBのスキーマ的にはちょっとめんどくさそうだぞ...

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Jul 23, 2018

DOORKEEPER_API_TOKEN とやらが必要なのか...

自分で作った

@aokabin

This comment has been minimized.

Copy link
Contributor

aokabin commented Jul 23, 2018

static_yaml、過去のものしか掲載しないのなら不要の可能性ある。。。?

@chicaco

This comment has been minimized.

Copy link

chicaco commented Dec 28, 2018

@aokabin 引き取らせていただきま〜す。

立て直す感じで。
(ここまでのコメントは一応キャッチアップしたつもり...)

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Dec 28, 2018

よろしくお願いします...!! (>人< )

僕もすべての実装をキャッチアップできているわけではないですが、
機能開発の背景・コンテキスト周りについては詳しい方だと思うので、
何か気になることや困っていることなどあればお気軽にご連絡ください☺️

@chicaco

This comment has been minimized.

Copy link

chicaco commented Dec 28, 2018

@yasulab (改めまして)
必要な情報は統計情報と同じような気がしており(細かいところで +α-β になるとは想定していますが)、似たようなモデルと実装になるのはどうかなーと。
中止情報の扱いは要注意ですけれど、『情報格納モデル(テーブル)は 1 つにして、過去分は「実績」、今日以降分は「予定」と扱う』ことにしなかった理由があれば教えていただけないでしょうか?

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Dec 28, 2018

『情報格納モデル(テーブル)は 1 つにして、過去分は「実績」、今日以降分は「予定」と扱う』ことにしなかった理由があれば

あー、そこは僕も気になるところですね 🤔 何となく想像ですが、統計情報ページで表に公開する計算方法自体ををいじると時間がかかりそうだったから、テーブルを分けたのかなと想像しています 💭 cc/ @AnaTofuZ

@nalabjp 既存の統計情報のテーブルを活用して、『過去分は「実績」として統計情報ページに表示させ、今日以降分は「予定」として扱い統計ページには反映させない』という実装だと結構工数かかりそうですかね? 🤔

cf. 統計情報 - CoderDojo Japan

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Dec 28, 2018

背景補足: プレスリリースでは統計情報が「oo回開催された」「xx人参加した」という過去形で利用されるので、開催予定や参加予定の数字は統計情報には含めたくない 🗾 📊 💦

cf. プログラミング学習のProgate、全国のCoderDojoへ法人プランの無料提供開始

現在は100ヶ国・1,900以上の道場があります。特に2016年頃から日本での増加がめざましく、現在その数は155ヶ所以上、参加者数は延べ15,000人以上に上ります。

@chicaco

This comment has been minimized.

Copy link

chicaco commented Dec 28, 2018

統計情報表示(実績)も開催予定表示も、開催日を条件にした絞り込みを追加すればイケると思っており...。
もちろん、収集用 rake タスクにも改修が必要ですけれども。

別 rake、別 module よりは将来的な保守工数も削減できるはず。←これが一番の理由デス
同じテーブルを拡張して共有するのが無理でも、外部サービスから情報を取ってくる処理だけでも切り出して共通化したいですね。

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Dec 28, 2018

別 rake、別 module よりは将来的な保守工数も削減できるはず。←これが一番の理由デス

これは僕も賛成ですね! 今のところ上記でコメントしたところ以外には懸念点が見つからないですし、おっしゃる通りこちらの方が将来的にはラクになる設計かなと思うので、僕としては @chicaco さん案を (現在持っている情報だけで判断するのであれば) 推したいなと考えています (>人< )

@chicaco

This comment has been minimized.

Copy link

chicaco commented Dec 28, 2018

今のところ上記でコメントした以外には懸念点が見つからないですし、おっしゃる通りこちらの方が将来的にはラクになる設計かなと思うので、僕としては @chicaco さん案を (現在持っている情報だけで判断するのであれば) 推したいなと考えています (>人< )

ではその方向で現モデルとコードのキャッチアップと設計に着手してみます。
その中で懸念点の確認と対処もチェックできるかと...。☝️

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Dec 28, 2018

了解です! d( ̄  ̄)

ちなみにこちらのブランチ、 分岐してから結構時間経っちゃっているので、まずは master の最新分に追いつくところから始めると良さそうです 👀 💖

@chicaco

This comment has been minimized.

Copy link

chicaco commented Dec 28, 2018

了解ですっ!

@nalabjp

This comment has been minimized.

Copy link
Contributor

nalabjp commented Dec 28, 2018

実績と予定の話って、
event_historiesテーブルのことでしょうか?
だとするとその名の通り履歴テーブルなので過去分のみを取り扱う意図でした。
使い回すなら名称は変えた方が良さそうに思います。

一つのテーブルを使い回すのは楽なケースもあると思いますが、
この場合だと使用時に必ず実績なのか予定なのかを判定する必要が出てくる事になると思います。
そして、それはレコードに状態を持たせることになるので、コードの複雑度は上がると思います。

テーブルにデータを取り込むロジックを使い回すのは記憶が確かならパラメータの調整だけで特に拡張も必要ないかと思います。

個人的にはイベント情報テーブルを使いますなら、eventsテーブル、event_historiesテーブル、upcoming_eventsテーブルなどとして(テーブル名は適当です)、event_historiesとupcoming_eventsはeventsと関連を持つような設計にすると思います。

こんなところで回答になってますでしょうか? > @yasulab @chicaco

@nalabjp

This comment has been minimized.

Copy link
Contributor

nalabjp commented Dec 28, 2018

全然質問に答えていなかった😇

実装工数だけの話なら重い作業でも無さそうには思います。

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Dec 28, 2018

おー、コメントありがとうございます! (>人< )

なるほど確かに event_histories に予定の情報が入るのは違和感がありますね 🤔💭 (テーブル名まで確認してなかったです 💦)

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Dec 28, 2018

個人的にはeventsテーブルを使いますなら、eventsテーブル、event_historiesテーブル、upcoming_eventsテーブルなどとして(テーブル名は適当です)、event_historiesとupcoming_eventsはeventsと関連を持つような設計にすると思います。

おー、コメント助かります! ☺️ 現在の実装は upcoming_events だったと記憶しているので、これは現在の設計 (PR上のブランチにあるコード設計) に近い印象ですね 👀

@nalabjp

This comment has been minimized.

Copy link
Contributor

nalabjp commented Dec 28, 2018

あ、もうひとつ補足を。
テーブルの責務を分けた際にデータ取り込み時の冪等性やら何やらは今のところ何も検討せずに話しているので、
そこの工数なんかは全く見えていません🙈

@chicaco

This comment has been minimized.

Copy link

chicaco commented Dec 28, 2018

@nalabjp コメントありがとうございます。
インプットが補強できて助かります。

レコードに状態を持たせることになるので

実績と予定を判別するために、日付以外に「状態」も必要でしょうか?
(「中止」は必要になるかも知れません...)

開催日翌日に upcoming_events から event_histories に移る(共存はしない)ようなイメージですが、個々に持つべき情報があるのかどうかはまだ判断できていません。

@nalabjp

This comment has been minimized.

Copy link
Contributor

nalabjp commented Dec 28, 2018

イベントが開催されたかどうかは現状でも判断できていなかったかと思うので、
中止を判断するのは難しいかもしれませんね。

@chicaco

This comment has been minimized.

Copy link

chicaco commented Jan 6, 2019

現状報告です。

開発環境がやっと整って、add_upcoming_tasks に master をマージして、
rails s でローカルサーバに接続もできました。

で、FACEBOOK_APP_ID と FACEBOOK_APP_SECRET と DOORKEEPER_API_TOKEN を設定し、
bin/rake statistics:aggregation[2018,2018] とやってみたのですが、

lib/event_service/client.rb:11:in `get'

からの faraday-0.15.4 の中でエラーに...。

bin/rake spec spec/lib/

52 examples, 0 failures, 17 pending

で終わったんですが。 😕

気になることが 1 つ。
テストの実行ログが

***Aggregate for 2019/01
2019/01~2018/12のイベント履歴の集計を行いました

となっていました。
期間指定(or 判定)が変??
...1/1〜7 限定で発生する不具合のようです。 😳
to が省略されたときの振る舞いで、Time.current.prev_week.end_of_week が前年になってしまうので。
lib/statistics/aggregation.rb の aggregation_to(to) ですが、どう直しましょうか...。
ちょっと考えます。

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Jan 10, 2019

Oh... 😭 2018/12/3 から統計スクリプトが動いてなかったですね... >< 💦
報告ありがとうございます! (>人< )

たぶん gem を更新したあたりからかなぁ🤔💭 (その辺りに僕が gem 更新をやった覚えがある)

image

@yasulab

This comment has been minimized.

Copy link
Member

yasulab commented Jan 10, 2019

TODO のコメントが fix されていたように見えたので消しちゃってましたが、これを revert すれば直りそう? 🤔💭
e0b6c5b#diff-8b7db4d5cc4b8f6dc8feb7030baa2478L36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment