個別ダウンロード用のAPIを作成 #146
個別ダウンロード用のAPIを作成 #146
Conversation
| emoji = Emoji.find_by(params[:emoji_id]) | ||
| emoji.download_logs.create(user: current_user) | ||
| image = emoji.image.slack | ||
| send_file image.file.path |
There was a problem hiding this comment.
[Brakeman]
Model attribute used in file name
| @@ -8,6 +8,13 @@ class Api::V1::DownloadController < Api::V1::BaseController | |||
| ZIP_FILENAME = "emojis.zip".freeze | |||
|
|
|||
| def index | |||
| emoji = Emoji.find_by(params[:emoji_id]) | |||
Codecov Report
@@ Coverage Diff @@
## master #146 +/- ##
==========================================
+ Coverage 99.88% 99.88% +<.01%
==========================================
Files 35 35
Lines 843 873 +30
==========================================
+ Hits 842 872 +30
Misses 1 1
Continue to review full report at Codecov.
|
| @@ -8,6 +8,13 @@ class Api::V1::DownloadController < Api::V1::BaseController | |||
| ZIP_FILENAME = "emojis.zip".freeze | |||
|
|
|||
| def index | |||
| emoji = Emoji.find(params[:emoji_id]) | |||
| emoji.download_logs.create(user: current_user) | |||
There was a problem hiding this comment.
download#indexでdownloadがcreateされるのは違和感があります
There was a problem hiding this comment.
createされてるのはdownloadじゃなくてdownload_logsだけど同様？
donwloadされた副作用としてdonwlod logのcreateなんだけど
There was a problem hiding this comment.
emojis#showとdownload#indexって何が違うんだっけ？
There was a problem hiding this comment.
emoji#showはemoji情報の取得、jsonが返ってくる
download#indexは画像のダウンロード、image fileが返ってくる
There was a problem hiding this comment.
emojis#showで返ってくるemoji情報に画像のURL入ってない？
There was a problem hiding this comment.
emojiを参照すればいつもダウンロードログを作るわけじゃないし、今のほうがスッキリしてると思うんだけど。
There was a problem hiding this comment.
ログを残したいエンドポイントを作るときに毎回controller側に
DownloadLog.create(hogehoge)って書かないといけないのイケてなくない？って思ってしまう
There was a problem hiding this comment.
ログを残したいのはダウンロードコントローラだけだし、emoji.download_logs.create ってしてるので、emoji modelのメソッド作るのと対して変わらんと思うんだが
There was a problem hiding this comment.
あぁ、
emoji = Emoji.find(params[:emoji_id])
emoji.download_logs.create(user: current_user)を同時にしたいわけか。
There was a problem hiding this comment.
Rails(というかMVCアーキテクチャ)はcontrollerをどこまで薄くできるかが腕の見せどころ
| DownloadLog.create!(emoji_id: emoji_id, user: current_user) | ||
| emojis = DownloadLog.transaction do | ||
| emoji_ids.map do |emoji_id| | ||
| Emoji.find_with_logging!(emoji_id, current_user) |
There was a problem hiding this comment.
N+1を回避するメソッドも生やしたくなってこない？
There was a problem hiding this comment.
もう疲れたんだが…ｗ
結局createは一括でできないからmodelでN+1するかcontrollerでN+1するかの違い？
There was a problem hiding this comment.
createは一括でできないけどfindは一括でできるよねって話
(createもbulk insertできるgem入れれば一括でできるんだけど)
https://qiita.com/amidara/items/2d42e7cd518e6820e083
# models/emoji.rb
def self.where_with_logging!(condition, user = nil, limit = 100) # 同時ダウンロードできる最大数って指定してるんだっけ？
emojis = Emoji.where(condition).limit(limit)
DownloadLog.transaction do
emojis.each {|emoji| download_logs.create!(emoji: emoji, user: user) }
end
emojis
end
def self.find_with_logging!(id, user = nil)
self.where_with_logging!(id: id, user, 1).first
endThere was a problem hiding this comment.
話はわかるが計算コスト変わらない気がするんよね
There was a problem hiding this comment.
早めにリリースしたいので別Issue対応でお願いシャス。 #168
close #125
なぜ実装する必要があるのか、
どんな機能を実装した(したかった)のか
どうやって実装した(したかった)のか