道場を非表示にする仕組みの追加 #340
道場を非表示にする仕組みの追加 #340
Conversation
|
#314 (comment) にて軽く仕様を確認したのですが、もう1点確認です。 https://coderdojo.jp/stats では、3つのグラフとその下に「推移テーブル」があります。
です。こちらも、道場数のみ -1 し、残り2つはそのまま。の認識でいいですか？ あと、1つ気づいたのですが |
|
@yasulab 道場数だけはグラフとテーブルでコールしているメソッドが別々になっていて、推移テーブルの方が 2013年が無い!? [11] pry(main)> Dojo.aggregatable_annual_count period
=> {"2012"=>5, "2014"=>9, "2015"=>4, "2016"=>30, "2017"=>30} # <= 2013 年が無い
[12] pry(main)> Dojo.annual_count period
=> {"2012"=>5, "2013"=>2, "2014"=>9, "2015"=>5, "2016"=>42, "2017"=>54}こちら、先程のミーティングにもあったとおり、 issue 立てますね。 推移テーブルの道場数だけ「合計数」が表示されていないのは仕様と理解していますが、もしかしたら単に表示忘れ？何か関係しているのかな 🤔 |
推移テーブルは「*集計対象のみ」という注釈が入っています。 |
ここ多分、直感的に分かりづらいんでしょうねぇ。 |
|
@nalabjp すみません、まだちゃんと理解していない 😅 のですがグラフ、テーブル、ともに去年までの 過去データ となっています。過去の記録なのでいつ集計しても数値は変わらない。の認識で良いでしょうか？ もちろん、過去分のデータがいじられると変化はしますが。。 |
はい、ご認識のとおりで大丈夫です。 |
|
なるほど！ 😸 |
This reverts commit e76e3fd. cf. #340 (comment)
統計情報への反映はさせなくて大丈夫です! 👌 なので coderdojo.jp のトップページで表示/非表示を toggle する boolean 値のカラムを Dojo モデルに追加し、それ dojos.yaml から更新できるようにしていただければ大丈夫です 🙆♂️ yaml を通してカラムの内容を更新し、カラムの内容に応じて View 側の表示/非表示が切り替えられたらと考えています🤔💭 |
|
ci がfailed になるのはDBフィールドを追加したから?.. 🤔 💭 |
こちら、統計情報に表示される数値部分への反映はしなくても良い。という事ですか？
一応、カラムを追加して View 側の表示/非表示が切り替わる部分は作成しました。 |
|
Stats ページには反映させなくて大丈夫です🙆♀️ 一方、トップページの冒頭で表示している Dojo 数については反映してもらえると嬉しいです😆✨ |
|
Description 書き直しておきました！😆 |
naopontan
force-pushed
the
hide_dojo
branch
2 times, most recently
from
41ade72 to
0c657df
Compare
|
ふむふむ。そうなのですね。勉強になります！ 📝 👀 |
🤔.oO( |
|
すみません、外し忘れでした 💦 |
|
非アクティブ化の例を一つ含めてもよさそうですね! 😸 試しに小手指の Dojo をこの仕組みで非アクティブにするコミットを積んで見てはどうでしょうか? 🤔 |
|
なるほど、このPRに含めちゃうのですね。 |
別件ですが TODO の前半 (仕様確認〜実装) の粒度が荒いですね 👀 後半と同じぐらいの粒度まで具体化したほうがよさそうです 📝 🔍 cf. #287 |
仕組みの実装と実例の追加は意味として異なるので、コミットは別々にしておいたほうがわかりやすいと思います 👍 |
|
うぅ 😓 早速バグ発見。とりあえずタイトルに |
|
了解です! 全然大丈夫です 😸 👌 |
|
@yasulab バグフィックスし、 PR の Description も更新してみました。 |
|
良さそう! 😸 @nalabjp さんも軽くレビューしていただけると嬉しいです! Description にも書いていただいておりますが、今回はワークアラウンドな対応を目指しています 🔧 💨 |
app/models/dojo.rb
Outdated
| @@ -32,7 +32,12 @@ def dump_attributes_to_yaml(attributes) | |||
| end | |||
|
|
|||
| def group_by_region | |||
| eager_load(:prefecture).default_order.group_by { |dojo| dojo.prefecture.region } | |||
| relation = block_given? ? yield : all | |||
There was a problem hiding this comment.
ブロックの結果にAR::Relationを期待していて、実質引数としているのがメソッドとして分かりづらいなと思いました。
def group_by_region(relation = all)としている方が多少は明示的かなぁと思ったのですが、
その前に呼び出し元を以下のように書けそうな気がしました。（手元に確認できる環境が今無いので未確認ですが😅）
def group_by_region_on_active
active.group_by_region
end
activeからのチェーンで期待どおりになるならgroup_by_regionの修正は不要そうに思えますがどうでしょうか？
There was a problem hiding this comment.
def group_by_region(relation = all)としている方が多少は明示的かなぁと思ったのですが、
確かにこっちがシンプルですね。変にブロックを使ってややこしくしてしまいました。
active.group_by_region
あ、動きました。...という事で、こっちの方がよさそうですね。直します 📝
勉強になります。ありがとうございます
| @@ -38,6 +38,7 @@ namespace :dojos do | |||
| d.created_at = d.new_record? ? Time.zone.now : dojo['created_at'] || d.created_at | |||
| d.updated_at = Time.zone.now | |||
| d.prefecture_id = dojo['prefecture_id'] | |||
| d.is_active = dojo['is_active'].nil? ? true : dojo['is_active'] | |||
There was a problem hiding this comment.
'false'をtypoしたらtrueが入りそうですけど、RubyのtruthyとTypeCastの仕様だし仕方ないですかね。
There was a problem hiding this comment.
こういう gem あったら使えたりするのかなぁ🤔💭
cf. https://twitter.com/darkdimius/status/1002049138366730240
It's public now: @stripe is building a typechecker for #Ruby with emphasis on scalability and user-friendliness. Currently presenting it at #RubyKaigi with
@nelhageand@ptarjan. Try it: https://sorbet.run/ . See it at Tachibana room at #RubyKaigi2018
|
@yasulab |
dojo モデルに is_active フィールドを追加しました。 具体的には非表示にしたい道場は db/dojos.yaml にて `is_active: false` を追加するとよいです。
|
@yasulab rebase しました(squash 少し慣れてきました) 少し話が変わりますがドキュメント類まで考慮した方が良いですかね？
|
|
コメントするつもりが close してしまった... orz |
こちらナイスな指摘ですね! 僕の方で上記ドキュメントに追記しておきました 📝 💨 |
|
マージします! PRの作成と実装、お疲れ様でした! 😸 ♨️ |
close #310,#314,#333
やりたいこと
休止や廃止などの道場に対応して道場一覧などからは非表示にしたい。
今回のアプローチ
dojos テーブルに状態管理のフィールドを追加することで対応する。
zen.coderdojo.com では非アクティブな情報を持っているため、API 連携ができると手動で対応する必要がなくなるが (#330)、実装に時間がかかりそうだった。
また、既にいくつかの休止依頼 (#333 ) がきていることもあり、今回はワークアラウンドで対応する。
TODO
関連情報
cf. #310
cf. #314
cf. #330
cf. #333