Skip to content
This repository has been archived by the owner on Jan 29, 2024. It is now read-only.

feat: filter out dependabot notifications #599

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

brainrepo
Copy link
Contributor

closes #587

@brainrepo brainrepo force-pushed the feat/587-filter-out-dependabot-notifications branch from 31a98d6 to 3df3ac1 Compare April 24, 2023 11:13
src/slackbot.js Outdated
Comment on lines 39 to 41
excludedAuthors.includes(node.content?.author?.login) ||
excludedAuthors.includes(node.content?.content?.author?.login) ||
excludedAuthors.includes(node.content?.creator?.login)
Copy link
Member

Choose a reason for hiding this comment

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

why are we checking in so many places? surely the dependabot PRs will carry this information consistently in a single place, don't they?

@brainrepo brainrepo requested a review from simoneb April 25, 2023 08:23
},
})

t.test('item created by dependabot', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

can we improve these tests' descriptions by including what the outcome of the test is? e.g. "doesn't do anything if it's dependabot PR".

'X-Hub-Signature-256': signature,
},
})
t.equal(slackStub.callCount, 0)
Copy link
Member

Choose a reason for hiding this comment

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

minor but can we use sinon to make these assertions? e.g. sinon.assertNotCalled(slackStub)

see https://sinonjs.org/releases/latest/assertions/

Copy link
Contributor Author

@brainrepo brainrepo Apr 25, 2023

Choose a reason for hiding this comment

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

The Sinon assertion doesn't increment the tap report assertion counter.
I reverted it, but I am happy to restore it.

src/graphql.js Outdated
@@ -106,6 +106,10 @@ export const getActivityById = async ({ graphqlClient, id }) => {
number
title
url
author {
url
Copy link
Member

Choose a reason for hiding this comment

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

do we need the author url?

@brainrepo brainrepo merged commit a3e39da into master Apr 25, 2023
@brainrepo brainrepo deleted the feat/587-filter-out-dependabot-notifications branch April 25, 2023 09:42
@github-actions github-actions bot mentioned this pull request Apr 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce Channel Noise Improvement Proposal
2 participants