Skip to content

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.

Signed-off-by: Julian Tölle <julian.toelle97@gmail.com>
@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

Signed-off-by: Julian Tölle <julian.toelle97@gmail.com>
@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 <julian.toelle97@gmail.com>
Signed-off-by: Julian Tölle <julian.toelle97@gmail.com>
@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
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
Signed-off-by: Julian Tölle <julian.toelle97@gmail.com>
@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