feat: Add author filter option for [GithubCommitActivity] #9251
Conversation
jNullj
force-pushed
the
feat/9215/commit-activity-author-filter
branch
3 times, most recently
from
6cac096
to
a932b25
Compare
Add a new filter option to [GithubCommitActivity], allowing users to filter the commit activity by a specific author. To make the filter more explicit, The label display "commits by [author]" for the total amount of commits and "commit activity by [author]" for other intervals when an author filter is selected. To maintain a clear and organized code structure, The filtered author is added as an argument and not to the shield path. The request to find the number of commits by the author is made using the REST api rather then the GraphQL api to make it in 1 request rather then 2. Resolves badges#9215
Add tests for the new filter by author feature.
Add test for new transformAuthorFilter function of GithubCommitActivity added for the author filter feature.
jNullj
force-pushed
the
feat/9215/commit-activity-author-filter
branch
from
a932b25
to
ab006fc
Compare
|
I am ready for a review! Hope i got some quality on first attempt. |
|
Just to make it a bit nicer to see behavior before running this, here is a short video of how it looks like on my testing env. (note, i used node 20) test_author_filter1.mp4 |
|
🚀 Updated review app: https://pr-9251-badges-shields.fly.dev |
| @@ -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' }, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I will pick one of the users for the example, no problem
There was a problem hiding this comment.
Thanks. Could we do the same on the other example (pick an author who has contributed to the repo)
| if (buffer.message === 'Not Found') { | ||
| throw new InvalidResponse({ prettyMessage: 'invalid branch' }) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
chris48s
added
the
service-badge
The author filter error handling removed was redundent as it would never execute, there is no way to seperate branch not found from repo not found.
PR badges#9233 replaced errorsMessages with httpErrors. This commit updates the new changes to stay up to date with that PR
this exception was removed in commit 9e358c8 and is not needed anymore
Picked a user with commits in the repo as an example that would work
Add test for REST API calls in commit activity branch
|
I've merged #9258 Can you sort out the merge conflict. I suspect once that conflict is resolved, that will be the last thing and this will be ready to merge too, but I'll give it another quick read over tomorrow. Cheers |
|
🚀 Updated review app: https://pr-9251-badges-shields.fly.dev |
Add a new filter option to [GithubCommitActivity], allowing users to filter the commit activity by a specific author.
To make the filter more explicit, The label display "commits by [author]" for the total amount of commits and "commit activity by [author]" for other intervals when an author filter is selected.
To maintain a clear and organized code structure, The filtered author is added as an argument and not to the shield path.
The request to find the number of commits by the author is made using the REST api rather then the GraphQL api to make it in 1 request rather then 2.
Resolves #9215