Conversation
|
@yuuki-tsujimura |
| end | ||
|
|
||
| def daily_report? | ||
| params["event"]["text"].match?("日報") |
There was a problem hiding this comment.
これだとどこかしらに「日報」という単語が含まれていたら日報として判定されないでしょうか？さすがにそれだとノイズが入る可能性がかなり高いです、、
There was a problem hiding this comment.
これ結構考えたんですが、利用者側でルールを決めてもらって、それを入力する形にするしかないのではないかと考えております。例えば、【日報】と毎回書いてもらうとか。ダメでしょうか？
There was a problem hiding this comment.
どのような形が良いか、ご提案いただけますでしょうか？もしくは利用者側がルールを決めるしかないのであれば、（OSSとして公開するので、OSSの）利用者側がルールを決めれる設計にしてもらえますでしょうか？
とりあえず現状ですと、本文中のどこかしらに日報という単語があればマッチするので、それはノイズが多くなってしまうのが気になりますね。
例えば、【日報】と毎回書いてもらうとか。
朝の日報、夜の日報を現状書いているのですけど、そう書きたい時はどうすればよいでしょうか？
There was a problem hiding this comment.
改行があれば日報の投稿として認識されるようにしてみました。いかがでしょうか。
def daily_report?
params["event"]["text"].match?(/日報\n|朝の日報\n|夜の日報\n|【日報】\n/)
end
There was a problem hiding this comment.
だいぶ有り難く、ベースメントこの方向性であれば大丈夫なのですけど、例えば3期生は【朝の日報】【夜の日報】と一行目に記載して投稿しているのですが、それはどうすればよいのでしょうか？
なお、そういうふうに多くのパターンに対応する必要がときは正規表現を使うと良いかもしれないです。
| end | ||
|
|
||
| def registered_user | ||
| username = fetch_username_from_slack |
There was a problem hiding this comment.
毎回わざわざSlackにusernameを取得しにいかないといけないのでしょうか？それであればSlackのuser_idとusernameを事前にDBに保存しておけば良いのではないでしょうか？
There was a problem hiding this comment.
ご指摘ありがとうございます。
こちら、「ユーザーの新規登録時に、登録されたユーザーネームを元にSlackのuser_idを取得し、データベースに保存する」という仕様に変更しようと思いますが、それで問題なさそうでしょうか？
There was a problem hiding this comment.
「ユーザーの新規登録時に登録されたユーザーネーム」というのは、本APIにおけるユーザーネームですよね？Slackのユーザーネームとは別かと思います。もし、「ユーザーの新規登録時にSlackのユーザーネームを登録し、それを元にSlackのuser_idを取得する」という意味でしたら、ユーザー登録時にSlackを使うことは必須ではないので、Slack関連は別メソッドにしていただきたいです。
There was a problem hiding this comment.
APIに登録するユーザーネームとslackに登録するユーザーネームが同じである（同じにしてもらう）という前提で考えておりましたので、上記のような提案となりました。
ご要望いただいたように、通常のユーザー登録メソッドとSlackのユーザー名登録メソッドは分けるようにします。
There was a problem hiding this comment.
というか、slackのユーザーネームを登録してもらう手間をユーザーに負わせるなら、いっそのことslackのユーザーIDを登録する手間を負ってもらおう思います（プロフィールページを見れば書いてあるので）。
There was a problem hiding this comment.
APIに登録するユーザーネームとslackに登録するユーザーネームが同じである（同じにしてもらう）という前提で考えておりました
この前提は違うケースも多いかなと思います。例えばTwitterとFacebookだと登録するユーザー名が異なると思うのですが、同じようにサービスによって登録するユーザーネームは一般的に異なることが多いですね。
なお、システムを作成される際にこういった開発者にとって都合の良い解釈はユーザーから「えっ思ってたのと違う」となる元なので、この解釈癖はなくされたほうが今後にとって良いかと思います。
slackのユーザーIDを登録する手間を負ってもらおう思います
こちらは特にそれで問題ございません。
| before_action :require_user_is_registered, :require_post_is_daily_report, :verify_signature, only: %i[create] | ||
|
|
||
| def create | ||
| post = registered_user.slack_posts.build(post_params) |
There was a problem hiding this comment.
registered_userの処理、before_action内で呼ばれて、ここでも呼ばれてないでしょうか？１リクエスト内でしたら1度処理したら2回目は処理時間が無駄にかかるだけなので実行したくないですね
There was a problem hiding this comment.
ご指摘ありがとうございます。下記のようにすることで対応いたしました。
def registered_user
username = fetch_username_from_slack
@registered_user ||= User.find_by(name: username)
end
| def require_post_is_daily_report | ||
| return if daily_report? | ||
|
|
||
| render status: 401, json: { |
There was a problem hiding this comment.
この場合って日報じゃなかったってことなので、Unauthorizedエラーではないかと思います、、
There was a problem hiding this comment.
こちら、Unauthorizedエラーではない（他のエラー）ということでしょうか？
「Unauthorizedエラーではないか」を最初「Unauthorizedエラーではないですか？」という風に読んだんですが、renderにUnauthorizedと書いていたのでこの解釈は間違っているのだろうということで確認の質問をいたしました。
There was a problem hiding this comment.
仰る通りです。
日報じゃないってことと、認証って別の話ですよね？
There was a problem hiding this comment.
Not Acceptableに変更しました。いかがでしょうか。
render status: 406, json: {
"result": false,
"status": 406,
"message": "Not Acceptable"
}
There was a problem hiding this comment.
Not Acceptableはめったに使うことがないかなと思います。詳細は下記をご覧ください。
https://developer.mozilla.org/ja/docs/Web/HTTP/Status/406
今回のエラーは「適切なデータが送られてこなかった」というもので、バリデーションエラーと同様の文脈になるので、他のAPIを見ると400(Bad Request)か422(Unprocessable Entity)が返ってくることが多いかと思います。
|
@yuuki-tsujimura |
3 participants
概要
ユーザー登録・ログイン機能とSlackの日報投稿をデータベースに保存する機能を作成しました。
加えて、本番環境構築手順書を修正しました。
これらのレビューをお願いいたします。
下記2点のコミットをご覧いただけますと幸いです。
動作確認方法
https://api.slack.com/appsにアクセスするhttps://nippo-api.herokuapp.com/slack_postsを入力し、Verifiedと表示されるまで待つ詳細
devise_token_authをインストールし、ユーザー登録・ログイン機能を作成しました。
Slackの日報投稿をデータベースに保存する機能を作成しました（以下、内訳）。