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.

Sign up for GitHub

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

Jump to bottom

feat: Add author filter option for [GithubCommitActivity] #9251

Merged
merged 14 commits into from
Jun 15, 2023
Merged

feat: Add author filter option for [GithubCommitActivity] #9251

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
37dbc7a
feat: Add author filter option for CommitActivity
jNullj Jun 10, 2023
71753a2
fix: solve eslint errors
jNullj Jun 10, 2023
998ea6f
Add tests for [GithubCommitActivity] filter by author
jNullj Jun 10, 2023
ab006fc
update [GithubCommitActivity] spec file for new author feat
jNullj Jun 10, 2023
43904a6
Fix null string for label of GithubCommitActivity
jNullj Jun 14, 2023
6703fe6
Update GithubCommitActivity example
jNullj Jun 14, 2023
9e358c8
improve error handeling for GithubCommitActivity
jNullj Jun 14, 2023
b704300
Merge branch 'master' into feat/9215/commit-activity-author-filter
jNullj Jun 14, 2023
c2b5769
update depricated functions
jNullj Jun 14, 2023
6b773c2
remove test for nonexisting error
jNullj Jun 14, 2023
3c2ddcb
Fixed test for commit activity unexisting repo
jNullj Jun 14, 2023
e60d71a
Update example for GithubCommitActivity
jNullj Jun 14, 2023
7b46dd9
Add test for invalid commit activity branch
jNullj Jun 14, 2023
713374a
Merge branch 'master' into feat/9215/commit-activity-author-filter
jNullj Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 71 additions & 8 deletions services/github/github-commit-activity.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import gql from 'graphql-tag'
import Joi from 'joi'
import parseLinkHeader from 'parse-link-header'
import { InvalidResponse } from '../index.js'
import { metric } from '../text-formatters.js'
import { nonNegativeInteger } from '../validators.js'
import { GithubAuthV4Service } from './github-auth-service.js'
import { transformErrors, documentation } from './github-helpers.js'
import {
transformErrors,
documentation,
errorMessagesFor,
} from './github-helpers.js'

const schema = Joi.object({
data: Joi.object({
Expand All @@ -18,11 +23,16 @@ const schema = Joi.object({
}).required(),
}).required()

const queryParamSchema = Joi.object({
authorFilter: Joi.string(),
})

export default class GitHubCommitActivity extends GithubAuthV4Service {
static category = 'activity'
static route = {
base: 'github/commit-activity',
pattern: ':interval(t|y|m|4w|w)/:user/:repo/:branch*',
queryParamSchema,
}

static examples = [
chris48s marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -31,6 +41,7 @@ export default class GitHubCommitActivity extends GithubAuthV4Service {
// Override the pattern to omit the deprecated interval "4w".
pattern: ':interval(t|y|m|w)/:user/:repo',
namedParams: { interval: 'm', user: 'eslint', repo: 'eslint' },
queryParams: { authorFilter: 'chris48s' },
Copy link
Member

Choose a reason for hiding this comment

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

Just to make the example "make sense", can we pick a username of someone who has made one or more commits to the example repos. e.g: for eslint/eslint, nzakas would make more sense as an example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will pick one of the users for the example, no problem

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Could we do the same on the other example (pick an author who has contributed to the repo)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

staticPreview: this.render({ interval: 'm', commitCount: 457 }),
keywords: ['commits'],
documentation,
Expand All @@ -45,6 +56,7 @@ export default class GitHubCommitActivity extends GithubAuthV4Service {
repo: 'squint',
branch: 'main',
},
queryParams: { authorFilter: 'jnullj' },
staticPreview: this.render({ interval: 'm', commitCount: 5 }),
keywords: ['commits'],
documentation,
Expand All @@ -53,9 +65,10 @@ export default class GitHubCommitActivity extends GithubAuthV4Service {

static defaultBadgeData = { label: 'commit activity', color: 'blue' }

static render({ interval, commitCount }) {
static render({ interval, commitCount, authorFilter }) {
// If total commits selected change label from commit activity to commits
const label = interval === 't' ? 'commits' : undefined
const label = interval === 't' ? 'commits' : this.defaultBadgeData.label
const authorFilterLabel = authorFilter ? ` by ${authorFilter}` : ''

const intervalLabel = {
t: '',
Expand All @@ -66,7 +79,7 @@ export default class GitHubCommitActivity extends GithubAuthV4Service {
}[interval]

return {
label,
label: `${label}${authorFilterLabel}`,
message: `${metric(commitCount)}${intervalLabel}`,
}
}
Expand Down Expand Up @@ -103,6 +116,30 @@ export default class GitHubCommitActivity extends GithubAuthV4Service {
})
}

async fetchAuthorFilter({
interval,
user,
repo,
branch = 'HEAD',
authorFilter,
}) {
const since =
this.constructor.getIntervalQueryStartDate({ interval }) || undefined

return this._request({
url: `/repos/${user}/${repo}/commits`,
options: {
searchParams: {
sha: branch,
author: authorFilter,
per_page: '1',
since,
},
},
errorMessages: errorMessagesFor('repo not found'),
chris48s marked this conversation as resolved.
Show resolved Hide resolved
})
}

static transform({ data }) {
const {
repository: { object: repo },
Expand All @@ -115,6 +152,20 @@ export default class GitHubCommitActivity extends GithubAuthV4Service {
return repo.history.totalCount
}

static transformAuthorFilter({ res, buffer }) {
if (buffer.message === 'Not Found') {
throw new InvalidResponse({ prettyMessage: 'invalid branch' })
}
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 ever hit this line?
If I call something like /github/commit-activity/t/badges/shields/does-not-exist?authorFilter=chris48s it tells me "repo not found". If we can't distinguish between branch not found and repo not found then we can just make the other error "repo or branch not found" (this is quite common).

While I was testing this, I also noticed that the branch not found case doesn't seem to be hanled correctly. It is not new in this PR (already exists in prod https://shields.io/github/commit-activity/t/badges/shields/does-not-exist ), but if you have a chance to look at it while you are looking at this, that would be amazing.

Given neither of these work but the spec tests pass, I'd suggest we move the "branch not found" test cases out to the service tests layer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made api calls with both unexisting repo and branch, and i could not find any way to seperate the two, they both reply with 404 same content/headers, thats why this code will never activate.
I will change the error into "repo or branch not found" as suggested.
You are currect as well about the interval='t' not being used, should i remove it with this PR? it would mix into the commit when squashed, which i think would be undesireable... Should be a mistake from #9196 .
Used it becouse the call from the GraphQL api also used this, but it appears that even the GraphQL code is never used.
I also tested the GraphQL requests, seems like you can seperate wrong branch and wrong repository, altho i am not sure if we want to do that. Wrong repo will return an error object which is used, while bad branch returns a null repo object without erros.
I think thats the reason the transform function tests if (!repo). but the _requestGraphql function cataches this in validation as it differ from schema, which require object inside repo and it gets null.
I could fix that, but i think it needs it's own issue to keep this commit clean when we merge, if so let me know and i will open this issue, and i could offer my fix there.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I am not following. What do you mean when you say

You are correct as well about the interval='t' not being used

?

This behaves the same way if I call https://shields.io/github/commit-activity/m/badges/shields/does-not-exist

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad, there is no diffrence, no idea how i mixed this in my reply. I edited the reply as i was debugging.
Should have read it twice before posting.

Copy link
Member

Choose a reason for hiding this comment

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

Could we add a couple of test cases to the service tests file (.tester.js) covering invalid branch (for both the GraphQL and Rest paths).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I separated the GraphQL test and fixed the error handling in PR #9258 as this is not related directly to this change, i think its better for history.
I will add another test for the REST path in this PR


const parsed = parseLinkHeader(res.headers.link)

if (!parsed) {
return 0
}

return parsed.last.page
}

static getIntervalQueryStartDate({ interval }) {
const now = new Date()

Expand All @@ -131,9 +182,21 @@ export default class GitHubCommitActivity extends GithubAuthV4Service {
return now.toISOString()
}

async handle({ interval, user, repo, branch }) {
const json = await this.fetch({ interval, user, repo, branch })
const commitCount = this.constructor.transform(json)
return this.constructor.render({ interval, commitCount })
async handle({ interval, user, repo, branch }, { authorFilter }) {
let commitCount
if (authorFilter) {
const authorFilterRes = await this.fetchAuthorFilter({
interval,
user,
repo,
branch,
authorFilter,
})
commitCount = this.constructor.transformAuthorFilter(authorFilterRes)
} else {
const json = await this.fetch({ interval, user, repo, branch })
commitCount = this.constructor.transform(json)
}
return this.constructor.render({ interval, commitCount, authorFilter })
}
}
11 changes: 11 additions & 0 deletions services/github/github-commit-activity.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ describe('GitHubCommitActivity', function () {
.with.property('prettyMessage', 'invalid branch')
})
})
describe('transformAuthorFilter', function () {
it('throws InvalidResponse on invalid branch', function () {
expect(() =>
GitHubCommitActivity.transformAuthorFilter({
buffer: { message: 'Not Found' },
})
)
.to.throw(InvalidResponse)
.with.property('prettyMessage', 'invalid branch')
})
})
describe('getIntervalQueryStartDate', function () {
/** @type {sinon.SinonFakeTimers} */
let clock
Expand Down
52 changes: 52 additions & 0 deletions services/github/github-commit-activity.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,43 +12,95 @@ const isCommitActivity = Joi.alternatives().try(
isZeroOverTimePeriod
)

const authorFilterUser = 'jnullj'

t.create('commit activity (total)').get('/t/badges/shields.json').expectBadge({
label: 'commits',
message: isMetric,
})

t.create('commit activity (total) by author')
.get(`/t/badges/shields.json?authorFilter=${authorFilterUser}`)
.expectBadge({
label: `commits by ${authorFilterUser}`,
message: isMetric,
})

t.create('commit activity (1 year)').get('/y/eslint/eslint.json').expectBadge({
label: 'commit activity',
message: isMetricOverTimePeriod,
})

t.create('commit activity (1 year) by author')
.get(`/y/badges/shields.json?authorFilter=${authorFilterUser}`)
.expectBadge({
label: `commit activity by ${authorFilterUser}`,
message: isMetricOverTimePeriod,
})

t.create('commit activity (1 month)').get('/m/eslint/eslint.json').expectBadge({
label: 'commit activity',
message: isMetricOverTimePeriod,
})

t.create('commit activity (1 month) by author')
.get(`/m/badges/shields.json?authorFilter=${authorFilterUser}`)
.expectBadge({
label: `commit activity by ${authorFilterUser}`,
message: isMetricOverTimePeriod,
})

t.create('commit activity (4 weeks)')
.get('/4w/eslint/eslint.json')
.expectBadge({
label: 'commit activity',
message: isMetricOverTimePeriod,
})

t.create('commit activity (4 weeks) by author')
.get(`/4w/badges/shields.json?authorFilter=${authorFilterUser}`)
.expectBadge({
label: `commit activity by ${authorFilterUser}`,
message: isMetricOverTimePeriod,
})

t.create('commit activity (1 week)').get('/w/eslint/eslint.json').expectBadge({
label: 'commit activity',
message: isCommitActivity,
})

t.create('commit activity (1 week) by author')
.get(`/w/badges/shields.json?authorFilter=${authorFilterUser}`)
.expectBadge({
label: `commit activity by ${authorFilterUser}`,
message: isCommitActivity,
})

t.create('commit activity (custom branch)')
.get('/y/badges/squint/main.json')
.expectBadge({
label: 'commit activity',
message: isCommitActivity,
})

t.create('commit activity (custom branch) by author')
.get(`/y/badges/squint/main.json?authorFilter=${authorFilterUser}`)
.expectBadge({
label: `commit activity by ${authorFilterUser}`,
message: isCommitActivity,
})

t.create('commit activity (repo not found)')
.get('/w/badges/helmets.json')
.expectBadge({
label: 'commit activity',
message: 'repo not found',
})

// test for error handling of author filter as it uses REST and not GraphQL
t.create('commit activity (repo not found)')
.get('/w/badges/helmets.json?authorFilter=zaphod')
.expectBadge({
label: 'commit activity',
message: 'repo not found',
})