Skip to content
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

Differentiate new PRs from issues in notifications #299

Merged
merged 3 commits into from
May 13, 2016

Conversation

dahlbyk
Copy link
Member

@dahlbyk dahlbyk commented May 13, 2016

Follow-up from #255

Fixes this:
slack_2016-05-13_15-03-16

@dahlbyk dahlbyk added this to the Polish milestone May 13, 2016
@dahlbyk dahlbyk temporarily deployed to huboard-rails-pr-299 May 13, 2016 20:02 Inactive
@dahlbyk dahlbyk temporarily deployed to huboard-rails-pr-299 May 13, 2016 20:41 Inactive
@dahlbyk
Copy link
Member Author

dahlbyk commented May 13, 2016

This isn't working: mash.payload.pull_request.nil?. I have once again conflated event and web hook payloads.

Rather than infer PR-ness from payload.issue properties (head, base, commits_url), it seems reasonable to explicitly mark PR payloads as such: 77b26c3.

@dahlbyk
Copy link
Member Author

dahlbyk commented May 13, 2016

Third time's a charm:
slack_2016-05-13_16-27-40

Ready for review.

@@ -6,6 +6,7 @@ class IssuesOpenIssueJob < IssueEventJob

def payload(params)
{
pull_request: params[:pull_request],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could also be written params[:issue].key?(:pull_request) if the webhook payload is similar enough to the original issue payload that is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding :pull_request to the base payload, not attaching it to the :issue hash.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I assume in discovery you found that the webhook doesn't carry a PR object ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, in WebhooksController#publish_pull_request_event we essentially "rename" the pull_request part of the payload as issue. So I added a simple flag in 77b26c3 to identify PRs again:

        message = HashWithIndifferentAccess.new(
+        :pull_request => true,
         :issue => payload[:pull_request],
         ...

This change just passed that value on through.

@discorick
Copy link
Member

Looks good to me :shipit: ing it

@discorick discorick merged commit 169cb10 into master May 13, 2016
@discorick discorick deleted the dahlbyk/new-pull-request branch May 13, 2016 21:52
@dahlbyk dahlbyk mentioned this pull request May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants