ropensci-org / buffy

Editorial bots generator
https://buffy-ropensci.readthedocs.io/en/latest/
MIT License
1 stars 2 forks source link

Welcome responder with "if" conditional on issue template #23

Closed mpadge closed 3 years ago

mpadge commented 3 years ago

@xuanxu Our external check service is now up and running, and we would like to plug it in to the welcome responder, which is now very easy thanks to your previous work :rocket: :+1: The only thing I do not (yet) know how to do is to only call the service if the issue is opened with particular templates? All templates have different names, and also variables that are either present or absent, plus one for submission-type that differs between the templates. We just need to work out:

  1. How to add if logic to a welcome responder? (Maybe that's already possible?)
  2. Which data from the template is best to use to determine that?

Thanks!

mpadge commented 3 years ago

@xuanxu A gentle ping on this issue. The external service is all up and running, and can be used to check template variables independent of here, so the welcome responder can call straight to that service :rocket: ... but one question before that:

It would still be good to have a welcome responder when full review issues are opened, but not when pre-submission enquiries or others are opened. So we still need some kind of if conditional. Could you please advise how best to achieve something like this:

welcome:
  data_from_issue: submission-type
  if: submission-type == "Standard|Estándar|Stats"
    message: "Thanks for submitting!"
  external_service:
    url: <endpoint-URL>
    method: get
    data_from_issue:
      - repourl
      - repo
      - issue_id
    template_file: template.md    

That if only determines whether or not the message is issued, but it would also be okay, or maybe even better, if the external service was also only called if the condition is met. Help appreciated! Thanks!

xuanxu commented 3 years ago

Whatever conditions are set (with the if param) are used to decide if the responder should run or not. So if you want a responder to only run if the submission type is one of "Standard, Estándar, Stats" you can configure it with:

welcome:
  if:
    value_matches: 
      submission-type: "Standard|Estándar|Stats"
  external_service:
    url: ...
    method: ...
    template: ...
    ...
mpadge commented 3 years ago

Awesome! This will also then only run on if, right?

welcome:
  if:
    value_matches: 
      submission-type: "Standard|Estándar|Stats"
  message: "Message only if right submission type"
  external_service:
    url: ...
    method: ...
    template: ...
    ...
xuanxu commented 3 years ago

Exactly!

mpadge commented 3 years ago

@xuanxu This issue is pretty much ready to close, but one question in the meantime. We current have a welcome responder that looks like this:

https://github.com/ropensci-org/buffy/blob/957fdc4d2a32c338e42310d815fffb05d39d30dd/config/settings-production.yml#L18-L31

I've been trying to test it in another repo, but it does not respond. What looks like a response was generated by my giving an external nudge to the external service, but the service is not queries at all by the welcome responder as configured. (Here is a clean example showing failure to respond, and no call was made to our external service.)

Is the responder as specified above hard-coded for repo: ropensci/software-review, or will it accept other values passed from buffy? The standard external service works perfectly (via @ropensci-review-bot check package), suggesting that the substitution of repo works perfectly there, but the automatic call from the welcome responder fails. Any ideas?

xuanxu commented 3 years ago

@mpadge It looks like the config for the external_service is missing a mandatory param: name. If that is the case in the server logs there should be a message like: Configuration Error in WelcomeResponder: No value for name

mpadge commented 3 years ago

Like this? Please merge if that's what you mean. (Only @maelle has access to those logs, so i can't check there, sorry.)

xuanxu commented 3 years ago

Merged!

mpadge commented 3 years ago

@maelle Will the Heroku server be automatically updated to match this change, or does something else have to be done?

mpadge commented 3 years ago

@xuanxu Now we get this response, and again here, clearly from here: https://github.com/ropensci-org/buffy/blob/f83771b9f6d4f1ecacf473bb131995ac4b1cb438/app/workers/external_service_worker.rb#L36-L38

The 500 was also logged at our external service (which is good!), so we now have the service itself working: https://github.com/ropensci-org/buffy/blob/f83771b9f6d4f1ecacf473bb131995ac4b1cb438/config/settings-production.yml#L97-L111

... yet not when part of the welcome responder: https://github.com/ropensci-org/buffy/blob/f83771b9f6d4f1ecacf473bb131995ac4b1cb438/config/settings-production.yml#L23-L32

Still need a bit more help debugging this, please!

xuanxu commented 3 years ago

🤔 If the service is called in both cases, maybe there's some diference in the query or params sent? Can you check that in the external service server?

BTW as it's configured now, both repo and issue_id values are overwritten with the real ones

mpadge commented 3 years ago

Yeah, it must be some difference in the query or params, but I can't intercept the parameters in the external service, unfortunately. Heroku also doesn't provide any insight into the queries that are actually constructed. Any other ideas?

xuanxu commented 3 years ago

Maybe taking a look at the payloads for a 'issue open' vs a 'comment' in the latest deliveries of the webhook and check in both cases the info is correct (issue:body includes repourl, etc) is worth a try. Other than that I'd try to log somewhere the query being sent. I'll try to reproduce it locally.

mpadge commented 3 years ago

The webhook bodies are identical, so it's not that.

mpadge commented 3 years ago

I rebuilt the logger and redeployed the service so we get more insight, including the full query string (after ? in the following). The external_service responder does this:

INFO [2021-08-31 12:42:29] 3.234.206.146 "Faraday v1.5.0" 138.68.123.59:8000 GET /editorcheck ?bot_name=ropensci-review-bot&issue_author=mpadge&issue_id=16&repo=ropenscilabs%2Fstatistical-software-review&repourl=https%3A%2F%2Fgithub.com%2Fhypertidy%2Fgeodist&sender=mpadge 200 0.766

while the welcome responder only does this:

INFO [2021-08-31 12:45:33] 3.234.206.146 "Faraday v1.5.0" 138.68.123.59:8000 GET /editorcheck ?bot_name=ropensci-review-bot&issue_author=mpadge&issue_id=19&repo=ropenscilabs%2Fstatistical-software-review&sender=mpadge 500 0.003

The query string for welcome responder isn't properly formed - it is only this (re-formatted for easy reading):

bot_name=ropensci-review-bot&
  issue_author=mpadge&
  issue_id=19&
  repo=ropenscilabs%2Fstatistical-software-review&
  sender=mpadge

but it should be this

bot_name=ropensci-review-bot&
  issue_author=mpadge&
  issue_id=16&
  repo=ropenscilabs%2Fstatistical-software-review&
  repourl=https%3A%2F%2Fgithub.com%2Fhypertidy%2Fgeodist&
  sender=mpadge

The all-important repourl parameter from the HTML variable array is not getting sent by the welcome responder. Further ideas @xuanxu ?

mpadge commented 3 years ago

@xuanxu after your buffy commit here deploying on Heroku we still get the welcome responder delivering this:

?bot_name=ropensci-review-bot&
    issue_author=mpadge&
    issue_id=20&
    repo=ropenscilabs%2Fstatistical-software-review&
    sender=mpadge

so still missing the critical repourl parameter.

mpadge commented 3 years ago

I had a bit of a buffy code check to see if i could spot anything. The problem seems to be that the parameter array from the issue template is not getting passed through by the welcome responder. This is likely not the cause, but nevertheless may be good to address just for the sake of consistency. You use params throughout to reference the HTML variables which populate the params array, but in these lines you have service_params:

https://github.com/ropensci-org/buffy/blob/1736e4d09cc377ff65785518d0f8ee78bc162d15/app/responders/welcome_responder.rb#L25-L36

mpadge commented 3 years ago

This can now also be closed. Thank you @xuanxu for all your great work here - addressing all the things in this issue is a key part of the whole automation process. Great work!!