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

User action heatmap #5131

Merged
merged 34 commits into from Oct 23, 2018

Conversation

@kolaente
Copy link
Member

kolaente commented Oct 20, 2018

This pr adds heatmaps to user's profile, just like the ones on github or gitlab. It takes every action from the action table to build the grid, this means things like issue comments are also included, not only code commits. The fetching is done via a new api endpoint.
I also added a config option to globally disable this.

With some tests, it does not seem like this is something which draws a lot performance (takes ~0.005 sec on a prod gitea server), so I see no reason to implement extra caching.

@markuman I saw your example and tried to re-use some things frontend wise, but unfortualy, the code is minified, so I couldn't use it. Feel free to comment if you have some ideas on how to improve the frontend code, js is not my strength.

Closes #6 and #4918

Screenshot

image

kolaente added some commits Oct 20, 2018

fmt
Show resolved Hide resolved models/user_heatmap.go Outdated

@bkcsoft bkcsoft added the lgtm/need 2 label Oct 20, 2018

kolaente added some commits Oct 20, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 20, 2018

Codecov Report

Merging #5131 into master will increase coverage by 0.04%.
The diff coverage is 70.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5131      +/-   ##
==========================================
+ Coverage   37.44%   37.48%   +0.04%     
==========================================
  Files         308      309       +1     
  Lines       45718    45782      +64     
==========================================
+ Hits        17119    17162      +43     
- Misses      26141    26159      +18     
- Partials     2458     2461       +3
Impacted Files Coverage Δ
modules/setting/setting.go 48.24% <100%> (+0.06%) ⬆️
routers/user/profile.go 33.03% <100%> (+0.61%) ⬆️
routers/user/home.go 41.98% <100%> (+0.34%) ⬆️
models/unit_tests.go 72.8% <100%> (+0.24%) ⬆️
routers/api/v1/api.go 74.84% <33.33%> (-0.54%) ⬇️
routers/api/v1/user/user.go 23.52% <66.66%> (+13.82%) ⬆️
models/user_heatmap.go 78.94% <78.94%> (ø)
models/repo_list.go 62.2% <0%> (-1.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f38fce9...e8a7559. Read the comment docs.

kolaente added some commits Oct 20, 2018

@lafriks lafriks added the changelog label Oct 20, 2018

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Oct 20, 2018

I don't think we need special setting to enable/disable this

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Oct 20, 2018

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 20, 2018

@lafriks @techknowlogick I also thought this can make massive load on the database, which is why I implemented the setting. I also did some benchmarks and discovered the mariadb caching on this itself is already very good (After the query was executed a few times, it takes 0.000 sec to get the results. This probably will be different when executed on a very large installation of Gitea. Maybe someone could test the query on try.gitea.io?

@lafriks As I already implemented it and set it to on by default, I don't think there's a need to remove the setting. If it still bothers someon, they can disable it.

kolaente added some commits Oct 21, 2018

@JD342

This comment has been minimized.

Copy link

JD342 commented Oct 21, 2018

@kolaente Second one looks super, though that last green cell is bugging me a bit since it's skewed more on the right, after the end of the grey separation line on the bottom.

Also, how does it behave on small screens such as phones?

Oh, and thanks for your effort, by the way. You're our hero.

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 21, 2018

@JD342 not that good on small screens, as I said it doesn't really scale well.
screen shot 2018-10-21 at 13 47 23

though that last green cell is bugging me a bit since it's skewed more on the right, after the end of the grey separation line on the bottom.

I think that is a bug in the library we're using to display this... hmmmmm.....

@JD342

This comment has been minimized.

Copy link

JD342 commented Oct 21, 2018

@kolaente Is it possible to display only the last month instead of the whole year for that case?

I like your animosity in the last commit of that screenshot.

You might want to get a coffee with some sugar, or some crackers.

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 21, 2018

@JD342

Is it possible to display only the last month instead of the whole year for that case?

I don't think that library can do that.

I like your animosity in the last commit of that screenshot.

That's nothing I wrote, its a git alias which gets a random commit message from http://whatthecommit.com/ - that way I dont need to do `git commit -am "now think of a commit message && git push" every time I test someting. I'm fine, don't worry. (I didn't even recognize the commit message until you pointed it out)

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 21, 2018

@kolaente I don't think that library can do that.

Oh it seems like it can. I'll try it.

@adelowo

This comment has been minimized.

Copy link
Member

adelowo commented Oct 21, 2018

@JD342 not that good on small screens, as I said it doesn't really scale well.

Github disables the heatmap on mobile. That could be an option.

@JD342

This comment has been minimized.

Copy link

JD342 commented Oct 21, 2018

@kolaente

I think that is a bug in the library we're using to display this... hmmmmm.....

It's probably just some missing margin/padding, since the same thing happens in the left with the names of the day.

That's nothing I wrote, its a git alias which gets a random commit message from http://whatthecommit.com/ - that way I dont need to do `git commit -am "now think of a commit message && git push" every time I test someting. I'm fine, don't worry. (I didn't even recognize the commit message until you pointed it out)

Gotcha. Do consider the crackers, though. I want you to feel energetic and give your best in this PR. I like productive people, especially when they do work for me for free.

kolaente added some commits Oct 21, 2018

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 21, 2018

Github disables the heatmap on mobile. That could be an option.

I've just done that, seems to be the easiest solution.

@JD342

This comment has been minimized.

Copy link

JD342 commented Oct 21, 2018

I've just done that, seems to be the easiest solution.

Bummer. It'd have been nice to see it on mobile as well. If it's an issue, I could help out on this - maybe by opening a PR myself after this one.

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 21, 2018

Bummer. It'd have been nice to see it on mobile as well. If it's an issue, I could help out on this - maybe by opening a PR myself after this one.

That seems like a good idea, my current priority is to get the heatmap itself merged, we can think about such enhancments later.

Show resolved Hide resolved models/user_heatmap.go Outdated
Show resolved Hide resolved models/user_heatmap.go Outdated
Show resolved Hide resolved models/user_heatmap.go Outdated
@lunny

lunny approved these changes Oct 22, 2018

@lunny lunny merged commit 6759237 into go-gitea:master Oct 23, 2018

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@tonysgi

This comment has been minimized.

Copy link

tonysgi commented Oct 23, 2018

The heatmap is great. I noticed when I was creating a test on https://try.gitea.com that as a new user, the heatmap does not render until you have submitted the first commit. Until that happens, it is perpetually displaying the "heatmap.loading" icon. I was able to replicate on several local copies of Gitea as well. Prob should just show an empty heatmap? Should I submit a bug or is this comment good enough?

image

HoffmannP pushed a commit to HoffmannP/gitea that referenced this pull request Nov 14, 2018

User action heatmap (go-gitea#5131)
* Added basic heatmap data

* Added extra case for sqlite

* Built basic heatmap into user profile

* Get contribution data from api & styling

* Fixed lint & added extra group by statements for all database types

* generated swagger spec

* generated swagger spec

* generated swagger spec

* fixed swagger spec

* fmt

* Added tests

* Added setting to enable/disable user heatmap

* Added locale for loading text

* Removed UseTiDB

* Updated librejs & moment.js

* Fixed import order

* Fixed heatmap in postgresql

* Update docs/content/doc/advanced/config-cheat-sheet.en-us.md

Co-Authored-By: kolaente <konrad@kola-entertainments.de>

* Added copyright header

* Fixed a bug to show the heatmap for the actual user instead of the currently logged in

* Added integration test for heatmaps

* Added a heatmap on the dashboard

* Fixed timestamp parsing

* Hide heatmap on mobile

* optimized postgresql group by query

* Improved sqlite group by statement

@kolaente kolaente deleted the kolaente:feature/commit-heatmap branch Jan 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment