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.

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

New filter option for CommitActivity #9215

Closed
jNullj opened this issue May 29, 2023 · 7 comments · Fixed by #9251
Closed

New filter option for CommitActivity #9215

jNullj opened this issue May 29, 2023 · 7 comments · Fixed by #9251
Assignees
Labels
service-badge Accepted and actionable changes, features, and bugs

Comments

@jNullj
Copy link
Collaborator

jNullj commented May 29, 2023

📋 Description

I would like to show my commit activity in a specific repo on my github profile readme using a shield.

To do that i want to modify the comit activity shield to enable filtering by user.

I suggest the label to read "commits by [user]" for total amount and "commit activity by [user]" for anything else (only if a user filter is selected).

I think that adding the filtered username to the path will be confusing as commit activity already includes the repo's owner in the path. I suggest adding it in an argument.

Here i present my vision with examples:
"https://img.shields.io/github/commit-activity/m/eslint/eslint?userFilter=exampleUser"
image
"https://img.shields.io/github/commit-activity/t/eslint/eslint?userFilter=exampleUser"
image

i suggest to modify the UI as follows:

image

I recently added total commits to the commitActivity shield and i don't mind adding this as well if you will have me.

I opened an issue to discuess those changes first please let me know if this looks alright and aligns with the project's goals.

@chris48s
Copy link
Member

Do you have a proposed implementation for this?

I think the difficulty is that we'd ideally want the user param to take github username (e.g: jNullj or chris48s) as the input, but the author param to history() in GitHub's GraphQL API needs a CommitAuthor id. We can't just pass it a string. So I think we would have to make multiple API calls (one to resolve username to an id, and the another to get total number of commits filtered by author).

Can you see a way of doing it with a single request?

@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label May 30, 2023
@jNullj
Copy link
Collaborator Author

jNullj commented Jun 2, 2023

I looked for a way to do that with a single request, here is my insight:

  1. Using only Github's GraphQL would require 2 requests as you indicated, I could not find a workaround.
  2. We could use 1 call to Github's REST API, for example https://api.github.com/repos/badges/shields/commits?author=jNullj then count the number of commits. (it also supports branch, and date range) but this is more wasteful then the GraphQL method (unused data return) and multiple requests may be required for paginated results when the number of commits is high, considering this, it is actually more probable to reach a higher number of requests then the GraphQL approach.
  3. We could ask the user to provide the user id to save on the extra request, but that would be a bad UX.

@chris48s
Copy link
Member

chris48s commented Jun 3, 2023

Counting https://api.github.com/repos/badges/shields/commits?author=jNullj works for contributors who have only made a handful of commits. It falls down for users who have made lots though. For example if I call https://api.github.com/repos/badges/shields/commits?author=chris48s it returns 30 commits because the response is paginated. I have made many more than 30 commits to this repo.
You can up the page size up to a point, but eventually you have to start paging though multiple requests.

That said, there is another pattern that can help us here 💡 If we call a GH rest endpoint with &per_page=1 and then parse the last page number from the link header, we can find out the total number of objects with one request.
This is the approach we take in

const { res, buffer } = await this._request({
url: `/repos/${user}/${repo}/contributors`,
options: { searchParams: { page: '1', per_page: '1', anon: isAnon } },
errorMessages: errorMessagesFor('repo not found'),
})
const parsed = parseLinkHeader(res.headers.link)

So far so good. Doing that brings up another interesting question though.. As far as I can tell, there are two possible queries we can make here. If I look at my own contributions for this repo

curl "https://api.github.com/repos/badges/shields/commits?author=chris48s&per_page=1" --include --silent | grep link

tells me I am the author of 496 commits

curl "https://api.github.com/repos/badges/shields/commits?committer=chris48s&per_page=1" --include --silent | grep link

tells me I am the committer of 255 commits

Neither of those is the same number reported on https://github.com/badges/shields/graphs/contributors though, and the number on https://github.com/badges/shields/graphs/contributors is also not those two added together 🙃

I think probably most people's expectation would be that this badge would report the same number of commits as the contributors page for their repo and I can see it being a common support question if we were to add this based on author=. I wonder if there is any API call we can make that gets us the same number we see there? My guess would be they are different due to co-authored commits or something.

@jNullj
Copy link
Collaborator Author

jNullj commented Jun 3, 2023

The &per_page=1 and header trick is amazing, first time i see such a thing, never realized the api returns this in the header, i will use that in another project i have where i request page by page.

Personaly when i think about who made the commit what comes to mind is the person who wrote it, the author.
In some repositories the commiter is also the author while in others the committer is the maintainer or even a bot.
One might rebase a commit or make some other changes that apply the changes, commiting it while not being the author.
A short search leads me to this stackoverflow answear.

But i understand this could be confusing, i suggest adding a new field with a combobox, something like commiterFilterType which defaults to author.

About the https://github.com/badges/shields/graphs/contributors page, The link to the commits in that page for each user links to the commits filtered by author and not commiter.... but i could also see as you mentioned that it isn't the same as the api call returns.
Notice how in the api docs the sha parameter has a default: "Default: the repository’s default branch (usually main)."
I think that the contributors page takes into account more branches.

I was able to learn from the docs that they count from master/main and gh-pages, but when i tried to use git to test that it still didn't add up... also rebase might effect this as well. notice the small info about rebase...

@chris48s
Copy link
Member

chris48s commented Jun 4, 2023

I think author is the conceptually right number.

Looking into this a bit more, another important difference is going to be that counting the commits by author includes merge commits, whereas the contributors page excludes merge commits.

I think I'm happy to just go ahead an implement this based on /commits?author using the per_page=1 / link header trick as that is the most efficient query. The number we get from that is consistent with what you get from

{
  repository(owner: "badges", name: "shields") {
    object(expression: "master") {
      ... on Commit {
        history(author: {id: "MDQ6VXNlcjYwMjU4OTM="}) {
          totalCount
        }
      }
    }
  }
}

It seems like the contributors page is the outlier here 🤷 and it looks like there is no API query we can make that is going to report same number.

@jNullj
Copy link
Collaborator Author

jNullj commented Jun 4, 2023

LGTM, I offer writing that if you would like the helping hand, altho this is small and I don't code for a living so I hope to live to the project standard.
Please assign the issue so it is clear which of us you would prefer adding this.

@chris48s
Copy link
Member

chris48s commented Jun 5, 2023

You seemed to do OK in #9196 I believe in you 😄

jNullj added a commit to jNullj/shields-fun-fork that referenced this issue Jun 10, 2023
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
jNullj added a commit to jNullj/shields-fun-fork that referenced this issue Jun 10, 2023
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
chris48s pushed a commit that referenced this issue Jun 15, 2023
* feat: Add author filter option for CommitActivity

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

* fix: solve eslint errors

* Add tests for [GithubCommitActivity] filter by author

Add tests for the new filter by author feature.

* update [GithubCommitActivity] spec file for new author feat

Add test for new transformAuthorFilter function of GithubCommitActivity added for the author filter feature.

* Fix null string for label of GithubCommitActivity

* Update GithubCommitActivity example

* improve error handeling for GithubCommitActivity

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.

* update depricated functions

PR #9233 replaced errorsMessages with httpErrors.
This commit updates the new changes to stay up to date with that PR

* remove test for nonexisting error

this exception was removed in commit 9e358c8 and is not needed anymore

* Fixed test for commit activity unexisting repo

* Update example for GithubCommitActivity

Picked a user with commits in the repo as an example that would work

* Add test for invalid commit activity branch

Add test for REST API calls in commit activity branch

---------

Co-authored-by: jNullj <jNullj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants