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

Implement contributors graph #25439

Closed
wants to merge 0 commits into from
Closed

Conversation

sahinakkaya
Copy link
Contributor

@sahinakkaya sahinakkaya commented Jun 22, 2023

Update: Unfortunately, this PR got closed by a mistake and can't be reopened. We are continuing discussion in #27882.

Overview

This is the implementation of a requested feature: Contributors graph (#847)

It makes Activity page a multi-tab page and adds a new tab called contributors. Contributors tab shows the contribution graphs over time since the repository existed. It also shows per user contribution graphs for top 100 contributors. Top 100 is calculated based on the selected contribution type (commits, additions or deletions).


Demo

Screen.Recording.2023-07-11.at.10.51.37.mov

Features:

  • Select contribution type (commits, additions or deletions)
  • See overall and per user contribution graphs for the selected contribution type
  • Zoom and pan on graphs to see them in detail
  • See top 100 contributors based on the selected contribution type and selected time range
  • Go directly to users' profile by clicking their name if they are registered gitea users

Benchmarks:

Here are the loading time of the Contributors page for some repositories:

  • Gitea repository with ~15k commits:
    • Page: 17079ms
    • Template repo/contributors: 316ms
  • My test repo with ~400 commits:
    • Page: 169ms
    • Template repo/contributors: 1ms

Issues to consider before merging

  • For big repositories, page loading time increases dramatically. We might consider setting a cron job as @lunny mentioned which I have no idea how to do so.
  • Again, for big repositories, updating all charts at the same time (while zooming, panning etc.) might cause performance issues. I don't experience this on my computer but I didn't test it on a low end device.
  • When contribution type is changed by user, whole page is re-rendered causing fetching the same data again which is unnecessary. This can be easily avoided by passing only the selected option to Vue. But I don't know how to do it so I need help here. (this is fixed 🥳)
  • Following issues are related with each other and likely to be fixed together (these are also fixed 🥳):
    • When contribution type is changed by user, we probably need to sort per user graphs based on the selected contribution type and not commits (which is default).
    • When user zooms or pans the graphs, we need to update title of the page to show correct dates. Currently this is hard-coded. (From repository start date - To current date)
    • When user zooms or pans the graphs, we probably need to update statistics on per user graphs (x commits, y additions, z deletions) to account only commits from the selected date range. Currently, they are calculated from start date to end date and sorted by total commits.
  • I don't know how to handle the errors so you might see things like iDontCareAboutErrors, _ := someFunc() while reviewing the code :D We probably need to handle them correctly so please point me in the right direction.
  • Most of the code I wrote is taken from other places in this repository and modified to fit my needs. Sometimes I encountered permission related code and I didn't bother to implement proper permission check for this feature. So I just used them as is. All the permission related code is taken from Activity page. We probably need to change them and implement proper permission check.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 22, 2023
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 22, 2023
@sahinakkaya sahinakkaya marked this pull request as draft June 22, 2023 12:13
@sahinakkaya
Copy link
Contributor Author

It would be nice to get some feedback and help about how to handle the issues I mentioned.

@sahinakkaya sahinakkaya mentioned this pull request Jun 22, 2023
@silverwind silverwind added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 22, 2023
@JakobDev
Copy link
Contributor

Looks cool!

A little thing about the new Tab: We already have a few Tabs for the Repo, depending n what is activated. It might be a good Idea, to add a new Insights Tab like GitHub, where we put this and other stats, that may be added in the future, in. The ACtivity could also be moved in the Insights Tab.

@sahinakkaya sahinakkaya marked this pull request as ready for review June 22, 2023 13:20
@lunny
Copy link
Member

lunny commented Jun 22, 2023

Thank you for your contributions. It's amazing.

Since it sums per day, so we can run the cron job per day and store the result in some table. Then all the performance problems will disappear. Another possible improvement is to retrieve the data via AJAX so the UI will not be blocked to wait data.

@sahinakkaya
Copy link
Contributor Author

sahinakkaya commented Jun 22, 2023

It might be a good Idea, to add a new Insights Tab like GitHub...

@JakobDev yeah I agree. I didn't want to do that in this PR because it is out of scope and I was not sure if I can do it without breaking something :D With the changes in this PR, it is possible to create Code Frequency graph very easily. So someone might do that after creating Insights tab.

@lunny Actually, it doesn't sum per day but per week. The structure is very similar to Github's mentioned in here. Still we can set up a cron job to run per day for big repositories though.

I have implemented an endpoint to get commit data but then deleted it because I couldn't find a way to use it :D Retrieving the data with an AJAX call makes sense. I will try that.

@6543 6543 added this to the 1.21.0 milestone Jun 22, 2023
routers/web/web.go Outdated Show resolved Hide resolved
routers/web/web.go Outdated Show resolved Hide resolved
@sahinakkaya sahinakkaya requested a review from 6543 June 22, 2023 20:24
@6543
Copy link
Member

6543 commented Jun 22, 2023

please dont forcepush it make's it hard to review :/

we do squashmerge in the end anyway

@silverwind
Copy link
Member

silverwind commented Jun 23, 2023

Based on the demo video, the graphs seem a bit too high. Should probably reduce their height to 50-70% of what they currently have.

@silverwind
Copy link
Member

Not sure I can help on the backend topics, but I will definitely help one the frontend ones.

@sahinakkaya
Copy link
Contributor Author

@lunny Did you have a chance to look at this again? I am waiting for reviews from maintainers.

@lunny
Copy link
Member

lunny commented Oct 19, 2023

@lunny Did you have a chance to look at this again? I am waiting for reviews from maintainers.

I will do it in following week.

modules/git/repo_commit.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Nov 1, 2023
@6543
Copy link
Member

6543 commented Nov 2, 2023

tested it and it looks like the template did fall apart a bit - as the menues do not have text anymore :/

@sahinakkaya
Copy link
Contributor Author

sahinakkaya commented Nov 2, 2023

tested it and it looks like the template did fall apart a bit - as the menues do not have text anymore :/

Yeah, I just realized it. It looks like it happened after #27856. I don't know how to fix it though. fixed.

@silverwind
Copy link
Member

Will check this out in a few days.

@silverwind silverwind closed this Nov 2, 2023
@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 2, 2023
@silverwind
Copy link
Member

silverwind commented Nov 2, 2023

I'm sorry I destroyed the PR by pushing my main branch onto it accidentially and we can not revive this PR now. Let's continue in #27880.

Lesson for me: use git push sahinakkaya chart:main to push to differently named branch because git apparently will prefer same-named branch as source, at least in my config.
Lesson for sahinakkaya: Never raise a PR from a main branch.

@sahinakkaya
Copy link
Contributor Author

I'm sorry I destroyed the PR by pushing my main branch onto it accidentially and we can not revive this PR now. Let's continue in #27880.

Can't I force push my branch and continue to this as if nothing happened?

Lesson for sahinakkaya: Never raise a PR from a main branch.

Duly noted.

@silverwind
Copy link
Member

silverwind commented Nov 2, 2023

Can't I force push my branch and continue to this as if nothing happened?

Yes, try that. I was unable to force-push your branch again as the PR got closed and I lost push permission with the closure, but you should be able to. I can then push my last commit onto that.

@sahinakkaya
Copy link
Contributor Author

@silverwind I force pushed but I don't see my commits in this PR. Maybe it needs to be re-opened by maintainers if that's possible.

@sahinakkaya
Copy link
Contributor Author

sahinakkaya commented Nov 2, 2023

According to this it should be possible to reopen but I don't know if did the right thing. I just did git push -f which is force pushing my main branch to sahinakkaya:main

Edit: reading that again, it should be possible to reopen this now. I just don't have the permission to do that.

@silverwind
Copy link
Member

silverwind commented Nov 2, 2023

Hmm no, I can't reopen currently:

image

I think it's a GitHub bug. I never seen a PR ressurect after resetting a branch.

@silverwind
Copy link
Member

silverwind commented Nov 2, 2023

If you like, push to a new branch name and make the PR from that, than I will close my new one.

@sahinakkaya
Copy link
Contributor Author

sahinakkaya commented Nov 2, 2023

Ok, I opened a new PR (#27882) with a new branch. You can close yours @silverwind.

@GiteaBot GiteaBot removed this from the 1.22.0 milestone Nov 2, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/translation size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants