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

fix: only count users own actions for heatmap contributions #5647

Merged
merged 6 commits into from
Jan 6, 2019
Merged

fix: only count users own actions for heatmap contributions #5647

merged 6 commits into from
Jan 6, 2019

Conversation

apricote
Copy link
Contributor

@apricote apricote commented Jan 5, 2019

Problem / Current Behaviour

The current query used to count contributions includes notifications sent to watchers of the repository.

Imagine following setup:

  • Two users with ids 0 and 1
  • User 0 has a repository with id 0
  • User 1 watches the repository
  • User 0 creates a new issue "Test" (or any other action) in the repo
  • The action table now contains following entries:
    sqlite> SELECT * FROM action ORDER BY id DESC LIMIT 10;
    id          user_id     op_type     act_user_id  act_user_name  repo_id     repo_user_name  repo_name   ref_name    is_private  content     created_unix  comment_id  is_deleted
    ----------  ----------  ----------  -----------  -------------  ----------  --------------  ----------  ----------  ----------  ----------  ------------  ----------  ----------
    19766       0           6           0                           0                                                  0           43|Test     1546719456    0           0
    19765       1           6           0                           0                                                  0           43|Test     1546719456    0           0
    

The current query only checks that the user_id matches, this means that the issue created by user 0 will show up as 1 contribution for both users.

Proposed Fix

By also checking for act_user_id we can assure that only actions that the user did himself are counted as a contribution.

We can not drop the check for user_id, assuming the scenario from above, the one issue created would show up as 2 (1 + number of watchers) contributions for the user who created the issue.

Drawback

Edit: Resolved by only adding the condition if the user is not an organization.

The organization's heatmap will now only contain meta-actions like ActionCreateRepo, previously all contributions made by users in this repository were also counted, resulting in a nice summary heatmap for all activity.

@codecov-io
Copy link

codecov-io commented Jan 5, 2019

Codecov Report

Merging #5647 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5647      +/-   ##
==========================================
+ Coverage    37.8%   37.81%   +0.01%     
==========================================
  Files         322      322              
  Lines       47489    47498       +9     
==========================================
+ Hits        17951    17962      +11     
+ Misses      26950    26948       -2     
  Partials     2588     2588
Impacted Files Coverage Δ
models/user_heatmap.go 83.87% <100%> (+6.59%) ⬆️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️

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 97dafdc...0c7877f. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 5, 2019
@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 5, 2019
@lafriks lafriks added this to the 1.8.0 milestone Jan 5, 2019
@jonasfranz
Copy link
Member

@apricote Do you consider implementing a case differentiation between organizations and users? e.g. showing the "old" heatmap for organizations and the "new" heatmap for users?

@apricote
Copy link
Contributor Author

apricote commented Jan 6, 2019

Here are some screenshots to visualize the changes:

Current Heatmaps (master)

All three look too similar to reflect actual individual contributions.

  • User 1
    gitea_heatmap_current_user1

  • User 2
    gitea_heatmap_current_user2

  • Organisation
    gitea_heatmap_current_org

"Fixed" Heatmaps (PR)

  • User 1
    gitea_heatmap_fixed_user1

  • User 2, visibly less contributions than User 1
    gitea_heatmap_fixed_user2

  • Organisation, now mostly empty

gitea_heatmap_fixed_org

@apricote
Copy link
Contributor Author

apricote commented Jan 6, 2019

@jonasfranz I added a check for organisations, their heatmap will now again show the combined contributions.

Example Heatmaps
  • User 1
    gitea_heatmap_fixed_user1
  • User 2
    gitea_heatmap_fixed_user2
  • Organisation
    gitea_heatmap_current_org

@lunny
Copy link
Member

lunny commented Jan 6, 2019

CI failed on mssql test

Signed-off-by: Julian Tölle <[email protected]>
Signed-off-by: Julian Tölle <[email protected]>
@apricote
Copy link
Contributor Author

apricote commented Jan 6, 2019

@lunny Fixed now

@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 6, 2019
models/user_heatmap.go Outdated Show resolved Hide resolved
As suggested by lafriks in his review
@jonasfranz
Copy link
Member

@lafriks need your review

@lafriks lafriks merged commit c42bde7 into go-gitea:master Jan 6, 2019
@lafriks
Copy link
Member

lafriks commented Jan 6, 2019

Please send backport to release/v1.7

lafriks pushed a commit that referenced this pull request Jan 6, 2019
@lafriks lafriks added the backport/done All backports for this PR have been created label Jan 6, 2019
@lafriks
Copy link
Member

lafriks commented Jan 6, 2019

Thanks for PR

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants