Display all user activity on contribution calendar regardless of permission scope.
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
New non authenticated view
Note the No public contributions found addition.
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.
Does this MR meet the acceptance criteria?
-
Changelog entry added -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
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
Activity
mentioned in issue #14078 (moved)
added 1 commit
- a7bac8e8 - Altered test assertions with new calendar behaviour.
added 1 commit
- f1a38e6a - Code changes and test cases to ensure user activity display on contribution calendar.
marked the task Squashed related commits together as completed
@kylehqcom Thank you for your contribution!
One small nitpick from me:
Can you please change line 2 ofcalendar_activities.html.haml
to sayVisible contributions for
.....You could have days where you contribute to public and private project.
Then it would be confusing if the number of contributions doesn't equal the displayed contributions :)@ClemMakesApps can you do a review as well please?
@yorickpeterse do you have any performance worries with this change?Edited by username-removed-25949added 1 commit
- d5f5af29 - Code changes and test cases to ensure user activity display on contribution calendar.
- @haynes Done
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 tovisible_to_user
andpublic_to_user
. Am I perhaps overlooking something?The difference being the instantiation of this class. The
@user
is the contributed user. Thecurrent_user
in this context (which can also be nil) is that of an authenticated user. I could probably have left thecontributed_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 fromContributionsCalendar
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
@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 inapp/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 @kylehqcom you'll want to change this value to the MR id which is 8495 instead of the branch name
Thanks @ClemMakesApps will do - that chicken egg thing when you create the changelog before the MR =]
@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-91641added 1 commit
- 9bfce5aa - Code changes and test cases to ensure user activity display on contribution calendar.
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
mentioned in merge request !7310
mentioned in issue #26571 (closed)
@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?
assigned to @DouweM
@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.
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() } 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)} I think we should be consistent about
public
vsvisible
. I also like @dimitrieh's suggestion in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8495#note_22863754.
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.
@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.
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.
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
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