Fix heatmap activity

[siddweiker]

Fixes #6087

Heatmap actions are not timezone aware

• user_heatmap.go model now returns contributions rounded down to 15 minute intervals instead of day
• heatmap.js now will convert the epoch time to the users local time and summarize it to the day

This will balance the amount of data sent and processing time. 15 minute intervals are used as that is the smallest difference in possible time zones.

One outstanding question I have is whether we need to modify the documentation for the swagger API call to note that contributions are rounded to 15 min intervals?

Testing

For testing I wrote a script to create 4 commits per hour for a whole day (96 total). Here is a screenshot before the fix:

Heatmap with the fix:

And here is a snippet of the entries to show that they should be all on the 27th:

[lafriks]

Just a wild idea, wouldn't it be better to pass users timezone to backend to do correct calculations based on that?

[kins-dev]

Just a wild idea, wouldn't it be better to pass users timezone to backend to do correct calculations based on that?

Why? What problem are you trying to solve by that and does it make sense?

For example let's say the server is in London. My team (Organization Foo) is in California, and there's a team (Organization Bar) in India.

For me, when I look at the Foo or Bar dashboard, I expect to see commits based on my frame of reference. There is an easy, simple, reliable way to get this behavior that does not require adding a new setting to the backend or even adding to the load on the backend. My browser which is typically in my frame of reference does it all.

I think if you're doing any timezone conversion on the server here, you're doing something wrong. Store things in UTC, present them in UTC, let the javascript engine do what it is designed to do. (Yes you have to chop it up and then let javascript reassemble things, but frankly that's required so the engine can properly do the conversion and dump into buckets. That is required because the heatmap library doesn't accept more than one entry per 24 hour period.)

[lafriks]

One thing is that API still requires returning counts per day, other thing is that timezone conversation can be easily done on unixtime just adding or removing specific count of seconds from UTC time and than just grouping as it was done before.

[siddweiker]

One thing is that API still requires returning counts per day

Where is it required to be counts per day?

[kins-dev]

One thing is that API still requires returning counts per day, other thing is that timezone conversation can be easily done on unixtime just adding or removing specific count of seconds from UTC time and than just grouping as it was done before.

https://try.gitea.io/api/swagger#/user/userGetHeatmapData

The model's description is:
UserHeatmapData represents the data needed to create a heatmap

Am I missing something here? If I use this API I need to handle a generic timestamp object, not a date object.

Also if you believe timezone handling is easy, well you should read up some on it:
https://stackoverflow.com/questions/2532729/daylight-saving-time-and-time-zone-best-practices

One of the major bullets is do not confuse time offsets with time zones. They are two different things and there are very specific rules around each.

The advice is to keep servers in UTC and "If using JavaScript, use moment.js with the moment-timezone extension." This is true if your are supporting older browsers that do not handle ECMAScript 6 as earlier versions do not support daylight savings time in an expected way:
https://stackoverflow.com/a/16951442

So let's look at browser support: https://caniuse.com/?search=es6

Based on global usage, 94+% of users won't run into the date/time problem if you just depend on ECMA. For the rest, there may be a case if they happen to be in the US and their in a location with day light savings that the values may be off by an hour for 1-2 weeks. Now you have to ask is it worth bringing in and maintaining another library for this? It seems like overkill.

But please, please do not try to do this as "just adding or removing specific count of seconds from UTC time"

Instead if you insist on making this harder (and frankly this issue has sat open for 2+ years because it looks like the Gitea team is making thngs harder than they need to be) use the go timezone library and don't roll this yourself. You will make things worse.

[siddweiker]

Any updates on getting this PR reviewed?

[zeripath]

Hi @siddweiker I didn't realise how small this PR is.

OK, I think it would generally be better for the client to request the data in the correct timezone but I can see how that would be difficult.

Given how small this PR then I think it's a worthy stopgap.

[zeripath]

Please fix conflicts.

This is not ultimately the correct solution but it's a good workaround

[codecov-commenter]

Codecov Report

Merging #15252 (234be2a) into main (44b8b07) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main   #15252   +/-   ##
=======================================
  Coverage   45.45%   45.46%           
=======================================
  Files         708      708           
  Lines       83410    83405    -5     
=======================================
+ Hits        37918    37922    +4     
+ Misses      39418    39410    -8     
+ Partials     6074     6073    -1     

Impacted Files	Coverage Δ
models/user_heatmap.go	87.87% <100.00%> (+6.29%)	⬆️
models/repo_list.go	77.04% <0.00%> (-0.78%)	⬇️
services/pull/pull.go	41.59% <0.00%> (ø)
modules/queue/workerpool.go	54.19% <0.00%> (+1.14%)	⬆️
modules/process/manager.go	75.30% <0.00%> (+2.46%)	⬆️
modules/util/timer.go	85.71% <0.00%> (+42.85%)	⬆️

---

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 44b8b07...234be2a. Read the comment docs.

[siddweiker]

Please fix conflicts.

This is not ultimately the correct solution but it's a good workaround

@zeripath Conflicts fixed. I also agree with your statement.