Fix issue comment unfurl

[dennissivia]

Closes #928

Problem

This commit fixes unfurling of private and public issue comments.
When the octokit/rest.js changed their routing arguments from id to <resource>_id, unfurling issue comments stopped working.

Solution

This PR adjusts the argument name and thus fixes the problem.

Example (from linked issue)

URL: https://github.com/rust-lang/rust/issues/63997#issuecomment-526118914
Error:

8:56:48.513Z ERROR http: Empty value for parameter 'comment_id': undefined (id=86b1ba8d-141f-40e9-9eef-ebc87e5f0be3)
  HttpError: Empty value for parameter 'comment_id': undefined
      at values.forEach (/Users/.../slack/node_modules/@octokit/rest/plugins/validate/validate.js:66:15)
      at Array.forEach (<anonymous>)
      at Object.keys.forEach.parameterName (/Users/.../slack/node_modules/@octokit/rest/plugins/validate/validate.js:38:12)
      at Array.forEach (<anonymous>)
      at validate (/Users/...s/slack/node_modules/@octokit/rest/plugins/validate/validate.js:15:23)
      at process._tickCallback (internal/process/next_tick.js:68:7)

Offending code:
We are passing id: id (shorthand version) instead of comment_id: id.

slack/lib/unfurls/comment.js

Lines 8 to 13 in 8aa8e7b

	const comment = (await github.issues.getComment({
	owner,
	repo,
	id,
	headers: { accept: 'application/vnd.github.html+json' },
	})).data;

Additional fixtures

In addition, I added new fixtures to allow tests that have less context.
This should make it clear, that for public unfurls, the user of the repo
and the comment does not matter and it decouples the fixture and
subscription setup.
This was mainly helpful to me to make sure the tests only require, what they should require.

ℹ️ These fixtures are very similar to the existing ones and can be ignored during the review if that helps.

[codecov]

Codecov Report

Merging #929 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
+ Coverage   97.04%   97.04%   +<.01%     
==========================================
  Files         157      157              
  Lines        2639     2640       +1     
  Branches      361      361              
==========================================
+ Hits         2561     2562       +1     
  Misses         74       74              
  Partials        4        4

[codecov]

Codecov Report

Merging #929 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
+ Coverage   97.04%   97.04%   +<.01%     
==========================================
  Files         157      157              
  Lines        2639     2640       +1     
  Branches      361      361              
==========================================
+ Hits         2561     2562       +1     
  Misses         74       74              
  Partials        4        4

[wilhelmklopp]

Great work ✨

I wonder how we can spot something like this earlier when we next upgrade Probot or Octokit. Should Octokit have thrown errors/warnings when passing old arguments that no longer work? cc @gr2m

[dennissivia]

@wilhelmklopp thanks for your review ❤️ 🙏

I wonder how we can spot something like this earlier when we next upgrade Probot or Octokit. Should Octokit have thrown errors/warnings when passing old arguments that no longer work?

Better error detection

I am not sure how to prevent this except than by adding a regression test as I did.
I made sure the unfurl test causes the exact same error we are seeing in production before fixing it.
Other than that, I have no idea.
We might also need to improve our logging/alerting because this error was visible in production and we did not notice it for quite some time. I will create and issue for that.

Better update management

With regards to the Update itself. The linked commit is part of this major version bump to 8.0. The big problem here is, that we might have just jumped over all the versions that had deprecation warnings if there were any.
I am also unsure about how to avoid this issue in the future. I hope @gr2m has an idea.

[gr2m]

I wonder how we can spot something like this earlier when we next upgrade Probot or Octokit. Should Octokit have thrown errors/warnings when passing old arguments that no longer work? cc @gr2m

Ocotkit is logging warnings. The commit you linked shows that the previous URL parameter :id was deprecated and was aliased to e.g. :thread_id, you should have seen a deprecation warnings in your logs if you still used id instead of thread_id.

For breaking versions I usually try to only remove previously deprecated changes. So make sure to update to the latest version of the previous major version, run your tests, address all deprecations, then upgrading should be simple.