diff --git a/spec/github/models_spec.cr b/spec/github/models_spec.cr index c75cc84..e833cfa 100644 --- a/spec/github/models_spec.cr +++ b/spec/github/models_spec.cr @@ -178,12 +178,48 @@ describe Github::Notification do ) notification.reason_message.should eq "コメントがつきました" end + + # latest_comment_url が subject.url に戻る通知でも、取得済みの subject 本体の + # コメント数で初回でないと判断できる(issue #116)。 + it "switches to a follow-up message when the fetched pull request has conversation comments" do + message = pull_request_without_comment_signal.reason_message(subject_detail(comments: 2, review_comments: 0)) + message.should eq "担当している PR/Issue に動きがありました" + end + + it "switches to a follow-up message when the fetched pull request only has review comments" do + message = pull_request_without_comment_signal.reason_message(subject_detail(comments: 0, review_comments: 3)) + message.should eq "担当している PR/Issue に動きがありました" + end + + it "switches to a follow-up message when the fetched issue has comments" do + notification = notification_from( + "assign", + url: "https://api.github.com/repos/o/r/issues/1", + latest_comment_url: "https://api.github.com/repos/o/r/issues/1", + ) + notification.reason_message(subject_detail(comments: 1)).should eq "担当している PR/Issue に動きがありました" + end + + it "keeps the assign message when the fetched pull request has no comment at all" do + message = pull_request_without_comment_signal.reason_message(subject_detail(comments: 0, review_comments: 0)) + message.should eq "アサインされました" + end + + it "keeps the assign message when the fetched payload carries no comment count" do + # 本文取得に失敗した場合など、判断材料が無いときは初回向け文言のままにする + pull_request_without_comment_signal.reason_message(subject_detail).should eq "アサインされました" + end end describe "#pretext" do it "prefixes the subject type before the reason message" do notification_from("mention").pretext.should eq "[Issue] メンションされました" end + + it "passes the fetched subject detail through to the reason message" do + pretext = pull_request_without_comment_signal.pretext(subject_detail(comments: 1)) + pretext.should eq "[PullRequest] 担当している PR/Issue に動きがありました" + end end describe "#display_title" do @@ -255,6 +291,52 @@ private def notification_from(reason : String, type = "Issue", url = "", latest_ }.to_json) end +# latest_comment_url が subject.url に戻っている(=コメントの有無を判定できない) +# アサイン済み PR の通知(issue #116)。 +private def pull_request_without_comment_signal + notification_from( + "assign", + type: "PullRequest", + url: "https://api.github.com/repos/o/r/pulls/1", + latest_comment_url: "https://api.github.com/repos/o/r/pulls/1", + ) +end + +# 本文取得で返る subject 本体(PR / Issue)のレスポンス。件数を省くと +# コメントオブジェクト(件数フィールドを持たない)と同じ形になる。 +private def subject_detail(comments : Int32? = nil, review_comments : Int32? = nil) + Github::Comment.from_json({ + body: "body", + user: {login: "octocat"}, + comments: comments, + review_comments: review_comments, + }.to_json) +end + +describe Github::Comment do + describe "#commented?" do + it "is true when the pull request has conversation comments" do + subject_detail(comments: 2, review_comments: 0).commented?.should be_true + end + + it "is true when the pull request only has review comments" do + subject_detail(comments: 0, review_comments: 3).commented?.should be_true + end + + it "is false when the pull request has no comment at all" do + subject_detail(comments: 0, review_comments: 0).commented?.should be_false + end + + it "is false when the payload has no comment count (a comment object)" do + subject_detail.commented?.should be_false + end + + it "is false for a locally built comment (no comment url / fetch failure)" do + Github::Comment.new(nil).commented?.should be_false + end + end +end + private def notification_with(url = "", repo_html_url : String? = nil) Github::Notification.from_json({ reason: "subscribed", diff --git a/spec/github/usecase_spec.cr b/spec/github/usecase_spec.cr index 419d364..84b77ea 100644 --- a/spec/github/usecase_spec.cr +++ b/spec/github/usecase_spec.cr @@ -39,6 +39,18 @@ private def comment(body : String? = "body", html_url : String? = "https://examp Github::Comment.from_json({body: body, html_url: html_url, user: {login: "octocat"}}.to_json) end +# コメントが無いスレッドで本文取得先になる subject 本体(PR / Issue)のレスポンス。 +# 件数を省くとコメントオブジェクトと同じ形になる(issue #116)。 +private def subject_detail(comments : Int32? = nil, review_comments : Int32? = nil) + Github::Comment.from_json({ + body: "body", + html_url: "https://example.com/c", + user: {login: "octocat"}, + comments: comments, + review_comments: review_comments, + }.to_json) +end + private def build(notify, comment) Github::Usecase.new(StubRepo.new(comment)).build_message(notify) end @@ -57,6 +69,18 @@ describe Github::Usecase do build(notify, comment).pretext.should eq "[Issue] レビュー依頼中の PR に動きがありました" end + # latest_comment_url からはコメントの有無が分からない通知でも、本文取得で + # 得た subject 本体のコメント数で文言を切り替える(issue #116)。 + it "reflects the follow-up wording when the fetched pull request has comments" do + message = build(pull_request(reason: "assign"), subject_detail(comments: 1, review_comments: 0)) + message.pretext.should eq "[PullRequest] 担当している PR/Issue に動きがありました" + end + + it "keeps the initial wording when the fetched pull request has no comment" do + message = build(pull_request(reason: "assign"), subject_detail(comments: 0, review_comments: 0)) + message.pretext.should eq "[PullRequest] アサインされました" + end + it "formats the title as owner/repo#number title" do build(notification, comment).title.should eq "octocat/Hello-World#42 Spurious failure" end diff --git a/src/github/models.cr b/src/github/models.cr index 712c1af..7c85228 100644 --- a/src/github/models.cr +++ b/src/github/models.cr @@ -72,6 +72,9 @@ module Github # 何が起きたか(コメントか push か状態変更か)は通知 payload から判別できない # ため、文言はコメントに限定せず「動きがありました」に留める。 # ここに reason を足せば他の reason にも同じ切り替えを適用できる。 + # + # 切り替えの判定は Subject#commented? だけでは足りず、本文取得で得た subject + # 本体のコメント数も併用する(issue #116。詳細は followup? のコメント)。 FOLLOWUP_MESSAGES = { "review_requested" => "レビュー依頼中の PR に動きがありました", "assign" => "担当している PR/Issue に動きがありました", @@ -91,8 +94,11 @@ module Github reason.in?(MENTION_REASONS) end - def reason_message : String - if subject.commented? + # reason に対応する文言。detail には本文取得で得た subject 本体(PR / Issue)を + # 渡す。コメントの有無の判定に使うだけなので、渡さなければ従来どおり + # Subject#commented? のみで判定する。 + def reason_message(detail : Comment? = nil) : String + if followup?(detail) followup = FOLLOWUP_MESSAGES[reason]? return followup if followup end @@ -100,6 +106,29 @@ module Github REASON_MESSAGES[reason]? || GENERIC_MESSAGE end + # 「初回ではない=その後の動き」とみなせるか。 + # + # Subject#commented?(latest_comment_url が subject.url と異なる)だけでは + # 取りこぼす。latest_comment_url は通知を発生させたイベント側を反映することが + # あり、コメント済みのスレッドでも push・レビュー・アサイン変更が起点の通知では + # subject.url に戻る。またレビューコメントは latest_comment_url に現れない + # ことがあるため、レビュー上でだけ議論されている PR は常に初回扱いになる。 + # 結果、コメントの付いた PR でも「アサインされました」のままになる(issue #116)。 + # + # そこで本文表示のためにどのみち取得している subject 本体のレスポンスを使う。 + # Subject#commented? が false のとき comment_url は PR / Issue 自体を指し、 + # そのレスポンスにはコメント数(PR は会話とレビューの 2 種)が含まれるので、 + # 追加の API 呼び出し無しでスレッドにコメントがあるかを判定できる。 + # + # 逆に Subject#commented? が true のときは取得先が実コメントでコメント数を + # 持たないが、その場合は先に true が確定するので影響しない。 + private def followup?(detail : Comment?) : Bool + return true if subject.commented? + return false unless detail + + detail.commented? + end + # CI・自動チェックの状態でメンションを抑止する対象か(issue #105)。 # レビューできる状態になっていない PR で `@channel` / `@everyone` を撃たない # ことが目的なので、PR の通知はすべて対象にする。 @@ -118,8 +147,9 @@ module Github end # 通知の pretext(botのセリフ)。`[] ` 形式。 - def pretext : String - "[#{subject.type}] #{reason_message}" + # detail は reason_message にそのまま渡す(issue #116)。 + def pretext(detail : Comment? = nil) : String + "[#{subject.type}] #{reason_message(detail)}" end # 一目で対象が分かるよう `owner/repo#番号 タイトル` 形式にする。 @@ -211,12 +241,15 @@ module Github # 値が入る。よって url と異なる値のときだけコメントありと判断できる。 # # あくまで「コメントが存在するか」であって「今回の通知の起点がコメントか」では - # ない点に注意。latest_comment_url は通知を発生させたイベントではなくスレッドの - # 現在の最新コメントを指すため、コメント済みスレッドに push や状態変更が来た - # 通知でも真になる。通知 payload にイベント種別が無く、追加の API 呼び出し - # 無しでは区別できないので、これを「初回ではない=その後の動き」の目安として - # 使い、文言側はコメントに限定しない表現にしている(issue #104 / PR #106 - # レビュー指摘)。 + # ない点に注意。通知 payload にイベント種別が無く、追加の API 呼び出し無しでは + # 区別できないので、これを「初回ではない=その後の動き」の目安として使い、 + # 文言側はコメントに限定しない表現にしている(issue #104 / PR #106 レビュー指摘)。 + # + # ただし真になるのは片道で、これが偽でもコメントが無いとは限らない。 + # latest_comment_url は通知を発生させたイベント側を反映することがあり、 + # コメント済みでも push などが起点の通知では subject.url に戻る。よって + # 「コメントが無い」側の確定には使えず、Notification#followup? では subject + # 本体のコメント数と併用する(issue #116)。 def commented? : Bool return false unless comment = latest_comment_url.presence comment != url @@ -240,16 +273,31 @@ module Github getter owner : User end + # 通知本文の取得結果。コメントが無いスレッドでは subject 本体(PR / Issue)を + # 取得するため、コメントと subject 本体の両方をこの 1 クラスで受ける。 class Comment include JSON::Serializable getter user : User getter html_url : String? getter body : String? + # スレッドのコメント数。subject 本体を取得したときだけ入り、コメント + # オブジェクトのレスポンスには無いため nilable(issue #116)。 + # PR は会話タブ(comments)とレビュー(review_comments)で別カウントになる。 + getter comments : Int32? + getter review_comments : Int32? def initialize(@body) @user = User.new end + + # スレッドにコメントが 1 件以上付いているか(issue #116)。 + # 件数が取れない場合(コメントオブジェクト・本文取得失敗・本文なし通知)は + # 判断材料が無いので false を返し、呼び出し側で初回向け文言に倒す。 + def commented? : Bool + total = (comments || 0) + (review_comments || 0) + total.positive? + end end class User diff --git a/src/github/usecase.cr b/src/github/usecase.cr index 78052a9..b8d4983 100644 --- a/src/github/usecase.cr +++ b/src/github/usecase.cr @@ -18,7 +18,9 @@ module Github author_name: comment.user.login, author_icon: comment.user.avatar_url, author_link: comment.user.html_url, - pretext: notify.pretext, + # コメントが無いスレッドでは comment は subject 本体(PR / Issue)になる。 + # その中のコメント数を文言の切り替え判定に使う(issue #116)。 + pretext: notify.pretext(comment), color: notify.subject.color, title: notify.display_title, title_link: notify.link(comment),