Skip to content
Snippets Groups Projects

Display all user activity on contribution calendar regardless of permission scope.

5 unresolved threads

Display all user contribution counts from both private and public as activity.

  • Be aware that I'm not a Ruby dev (Golang/PHP), I have followed existing code examples but please help me ensure standards are met.
  • Confirm performance impact on returning all records a user has contributed to. Memory consumption should increase, but execution should be less due to straight select without filtering.

Why was this MR needed?

VCS profiles are used more and more as reference points and resumes for both contributors and recruiters. The current behaviour of Gitlab is to suppress contributions made to private projects from the public domain via the contributions calendar, eg https://gitlab.com/kylehqcom. But that doesn't give a true sense of a users activity. In this context, "activity" should be based on any contributions to a project, private or public.

Of course this does not mean that details of private projects should be returned to the public. The code included in this MR simply returns contribution counts as user activity for both private and public projects. Only public project commit details are displayed as normal, but all count values are returned. This behaviour is respected on specific date selecting also.

Existing non authenticated view

master-no-auth

New non authenticated view

Note the No public contributions found addition. new-no-auth

New authenticated view

Note the so-private commit entry which is returned in the auth'd view and is the source to the contribution count of the non auth'd view. new-auth

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Please refer to https://gitlab.com/gitlab-org/gitlab-ce/issues/14078 for comments and +1's to this feature request.

Thanks, Kyle

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
16 16 find_union(segments, Project).includes(:namespace).order_id_desc
17 17 end
18 18
19 # Finds the projects "@user" has contributed to, NOT limited by visibility to the @user. By
20 # removing the visibility bounds, a more accurate representation of User activity can be
21 # returned.
22 #
23 # Returns an ActiveRecord::Relation.
24 def activity
25 segments = all_projects(@user)
26
27 find_union(segments, Project).includes(:namespace).order_id_desc
  • @kylehqcom Looking at this code and the definition of all_projects, this seems to still limit contributions to the ones visible to the currently logged in user. Note the calls to visible_to_user and public_to_user. Am I perhaps overlooking something?

  • The difference being the instantiation of this class. The @user is the contributed user. The current_user in this context (which can also be nil) is that of an authenticated user. I could probably have left the contributed_projects_finder.rb file alone and altered the calling code to pass in the contributed user value twice, but that is ugly/smelly and doesn't seperate concerns, hence why I made the seperate method call.

    Regardless, it looks as though we should use the https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7310 as the datasource moving forward IMO.

  • @kylehqcom I prefer passing current_user twice on the call site, to having two almost-identical methods in this class. Finder#execute(current_user) is an established pattern in the code base, so I think using it like that from ContributionsCalendar is fine.

  • Hi @DouweM, thanks for you feedback but personally I prefer the opposite or left as is.

    From coming into the code base with no prior knowledge & seeing the same user passed twice to the method call, to me the reader, it looks like an error/mistake. Plus I think it would only confuse another coder in future to try to decipher what this method does if they need to call it again. eg hmmm do I pass the user in context and the db user I want to see or only the context user or only the db user???

    By using a seperate call, you seperate the concerns and make it clear to the caller as to the nature of the method. A small duplication is always better than a bad abstraction, or in this case, overloading the method call.

    In any case, it is likely that all this will change as we wait for https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7310 to be merged so we know what/how to return results.

    Edited by username-removed-91641
  • Please register or sign in to reply
  • @haynes @kylehqcom Come to think of it, there is this MR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7310

    If I'm not mistaken said MR already includes private contribution counts in the calculated data, thus the MR from @kylehqcom might be redundant. Could one of you verify if this is the case?

    Edited by yorickpeterse-staging
  • @yorickpeterse That seems to be the case. at least partly.
    The other MR already includes the contribution counts, but is missing tests and the changes in app/views/users/calendar_activities.html.haml

    I propose to either close this one and copy the changes to the other, or merge the other MR first and change this one to only add the differences.
    @kylehqcom What do you think?

  • 1 ---
    2 title: All contribution counts from both private and public repos are displayed as calendar user activity.
    3 merge_request: issue-14078-private-contributions
  • @dimitrieh would you like to take a look at the new UI change for this? Not sure if the terminology (visible vs public contributions) could be improved

  • @haynes IMO I would be going with the new schema/query as the datasource for this MR. I'm not familiar with the event schema or how much work the filters are doing, this additional MR would suggest that the existing logic is doing far too much for a simple return count. And having these values pre-loaded via a cron means simple lookups over crunching on the fly.

    We can park this MR and make https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7310 a dependency and then leverage when in.

    Do we have an ETA however for when this will be in prod? Is it likely to be accepted soon?

    Migration changes are always far longer (and should be) to process than code changes. Eg I'm not even sure that another table is the right storage backer for such data, seems more "loggy" than a relational table to me. I'm sure there is a process internally that confirms architectural decisions... =]

    Edited by username-removed-91641
  • added 1 commit

    • 9bfce5aa - Code changes and test cases to ensure user activity display on contribution calendar.

    Compare with previous version

  • I've had another thought about using the https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7310 which is that of UX. Users will expect to see their commit changes fairly instantly. If the cron job runs daily, then it's likely to get a bit of heat from our/the users. If the intent is to just re-run the background job whenever a change is made, then it would seem to me that https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7310 is no better than the event logic as it's just duplicating it?

    And it brings up other pain points now that I have seen the issue https://gitlab.com/gitlab-org/gitlab-ce/issues/22623 - I think there is a race condition based on the same method being called at multiple times. Unless the table rows are locked (they might be but I don't know), there is no guarantee that contributions = contributions + 1 will give you the correct result. There is also no need for the primary key ID field, just use the unique user_id and date as the PK.

    Perhaps I should start commenting there! K

  • username-removed-91641 mentioned in merge request !7310

    mentioned in merge request !7310

  • added ~480950 label

  • @ClemMakesApps sorry it took so long to get to this :)

    I think its strange to just state that there are contributions, even if none are visible. This creates an imbalance and/or confusion...

    We could probably overcome this by stating something similar to: 23 contributions of which 4 hidden

    thoughts?

  • Hmm, thats a good suggestion. It would be a better transition to having an option of displaying all vs public activity on the calendar (if and when that happens).

    @DouweM can you take a look at this and review?

  • @kylehqcom @ClemMakesApps What do you think about allowing the user to choose whether or not to include private contributions, like GitHub does? I think that if we release this as is, we will get some complaints because some people do feel that private contributions should stay totally private. It's a form of information leaking which not every user/company may be comfortable with.

    Screen_Shot_2017-02-06_at_15.45.52

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 7 7 def initialize(contributor, current_user = nil)
    8 8 @contributor = contributor
    9 9 @current_user = current_user
    10 @projects = ContributedProjectsFinder.new(contributor).execute(current_user)
    10 @projects = ContributedProjectsFinder.new(contributor).activity()
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 32 32
    33 33 it { is_expected.to eq([private_project, public_project]) }
    34 34 end
    35
    36 describe 'activity without a current user' do
    37 subject { finder.activity }
    38
    39 it { is_expected.to eq([private_project, public_project]) }
    40 end
    41
    42 describe 'activity with a current user' do
    43 subject { finder.activity() }
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 24 24 = event.project_name
    25 25 - else
    26 26 %p
    27 No contributions found for #{@calendar_date.to_s(:short)}
    27 No public contributions found for #{@calendar_date.to_s(:short)}
  • What do you think about allowing the user to choose whether or not to include private contributions, like GitHub does? I think that if we release this as is, we will get some complaints because some people do feel that private contributions should stay totally private. It's a form of information leaking which not every user/company may be comfortable with.

    That makes sense. I think the community wanted that exact feature you're mentioning but we wanted to create a small first iteration for the community to contribute. I'm not sure where to draw the line

  • @DouweM I like the option and placement to choose it from. How does GH display their private contributions on the graph and list? I do not have any private repo's myself on there..

    @ClemMakesApps mmh i think making this functionality similar as to GH does is nice... would it be a big change to include that?

  • mmh i think making this functionality similar as to GH does is nice... would it be a big change to include that?

    It would probably include changing the database model. Wouldn't we also want the toggle to be in the profile settings page and the user page? I think those are the main things that would need to be added to this existing MR. @DouweM probably has a better grasp on the extra amount of work required

  • @ClemMakesApps Yep, that sounds about right.

  • Douwe Maan removed assignee

    removed assignee

  • @kylehqcom are you still interested in continuing your work on this merge request? If yes, please take a look at the feedback and merge conflicts.

  • added ~65372 label

  • Hi @adamniedzielski - This merge request is dependent on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7310 - a new dbase table has been created to store user contribution data. I wrote the above with the existing data model so I was wanting the new table in place so it could be utilised.

    From reading the updates, it also looks as though @yorickpeterse is implementing some/all of the features inside this MR on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7310 - I would be happy to help there as/if required. Thanks K

  • @kylehqcom @adamniedzielski !7310 is being worked on by @leibo. Perhaps they and @kylehqcom could work on this together somehow. Said MR does indeed include new types of contribution data if I'm not mistaken.

  • username-removed-378947 removed ~65372 label

    removed ~65372 label

  • Shouldn't this MR have Closes #14078 reference?

  • any updates on allowing the user to choose whether or not to include private contributions, like GitHub does?

  • ^ Also would like to know

  • MR !7310 got rejected, so it seems like a nope.

    Edited by username-removed-1536448
  • @kylehqcom thanks for your hard work on this! This is a really great feature.

    I think making it easy for people to maintain their privacy is very important, and activity on private projects is part of that. The current implementation protects privacy by calculating counts based on the access level of the viewer.

    https://gitlab.com/gitlab-org/gitlab-ee/issues/1233 is related. Although the specific nature of the activity may not be public, the data might still be protected under some privacy laws. We should probably err on the side of caution on this one.

    I'd love to see this merged as an opt-in feature :thumbsup:

  • Thanks @jramsay for your feedback. Sorry I completely dropped off this issue a while back. Basically due to the process so I put my efforts else where.

    I'm all about the user so happy caution is being taken on any privacy laws that may be harmful. One thing I just thought of however, wouldn't that also be an issue for Github right now? Perhaps they haven't thought about it?

    Either way, I'd like to see this delivered however is best. It's not such a big deal for me now that I'm fully employed, however for all Gitlab users whom use this platform as a resume, it would be handy for sure. Thanks K

  • Please register or sign in to reply
    Loading