Welcome responder with "if" conditional on issue template

[mpadge]

@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 🚀 👍 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]

@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 🚀 ... 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]

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]

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]

Exactly!

[mpadge]

@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:

buffy/config/settings-production.yml

Lines 18 to 31 in 957fdc4

	welcome:
	if:
	value_matches:
	submission-type: "Standard|Estándar|Stats"
	message: "Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type `@ropensci-review-bot help` for help."
	external_service:
	url: http://138.68.123.59:8000/editorcheck
	method: get
	query_params:
	repo: ropensci/software-review
	issue_id:
	data_from_issue:
	- repourl
	template_file: goodpractice.md

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]

@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]

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]

Merged!

[mpadge]

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

[mpadge]

@xuanxu Now we get this response, and again here, clearly from here:

buffy/app/workers/external_service_worker.rb

Lines 36 to 38 in f83771b

	elsif response.status.between?(400, 599)
	respond("Error (#{response.status}). The #{service['name']} service is currently unavailable")
	end

The 500 was also logged at our external service (which is good!), so we now have the service itself working:

buffy/config/settings-production.yml

Lines 97 to 111 in f83771b

	external_service:
	name: editorcheck
	only:
	- editors
	command: check package
	description: Various package checks
	message: Thanks, about to send the query.
	url: http://138.68.123.59:8000/editorcheck
	method: get
	query_params:
	repo: ropensci/software-review
	issue_id:
	data_from_issue:
	- repourl
	template_file: goodpractice.md

... yet not when part of the welcome responder:

buffy/config/settings-production.yml

Lines 23 to 32 in f83771b

	external_service:
	name: editorcheck
	url: http://138.68.123.59:8000/editorcheck
	method: get
	query_params:
	repo: ropensci/software-review
	issue_id:
	data_from_issue:
	- repourl
	template_file: goodpractice.md

Still need a bit more help debugging this, please!

[xuanxu]

🤔 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]

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]

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]

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

[mpadge]

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]

@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]

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:

buffy/app/responders/welcome_responder.rb

Lines 25 to 36 in 1736e4d

	def external_service(service_params)
	check_required_params(service_params)
	ExternalServiceWorker.perform_async(service_params, locals)
	end
	def check_required_params(service_params)
	[:name, :url].each do |param_name|
	if service_params[param_name].nil? || service_params[param_name].strip.empty?
	raise "Configuration Error in WelcomeResponder: No value for #{param_name}."
	end
	end
	end

[mpadge]

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!!