Group comments as discussions on a pull request.
Created by: kouno
This solve issue #1007 (closed).
Comment on commit:
Comment on a merge request diff:
Comment on a commit line (not on the diff tab, but the commit URL):
Merge request reports
Activity
- app/helpers/discussion_helper.rb 0 → 100644
1 module DiscussionHelper 2 def part_of_discussion?(note) 3 note.for_commit? || note.for_merge_request_diff_line? 4 end 5 Created by: riyad
there is
for_diff_line?for that: https://github.com/gitlabhq/gitlabhq/blob/master/app/models/note.rb#L86By Administrator on 2012-10-21T03:24:00 (imported from GitLab project)
By Administrator on 2012-10-21T03:24:00 (imported from GitLab)
- app/helpers/discussion_helper.rb 0 → 100644
3 note.for_commit? || note.for_merge_request_diff_line? 4 end 5 6 def has_rendered?(note) 7 discussions_for(note).include?(discussion_id_for(note)) 8 end 9 10 def discussion_rendered!(note) 11 discussions_for(note).push(discussion_id_for(note)) 12 end 13 14 def discussion_params(note) 15 if note.for_diff_line? 16 @reply_allowed = true 17 @line_notes = discussion_notes(note) 18 { note: note, diff: note.diff } Created by: riyad
@kouno I think you need to rebase your changes. Also you should definitely add some specs.
By Administrator on 2012-10-18T13:48:40 (imported from GitLab project)
By Administrator on 2012-10-18T13:48:40 (imported from GitLab)
Created by: sroth80021
+1 Although the 'started a discussion' line does take up vertical space (a con perhaps), I like this idea. Many code review tools support this concept, and some support threading (via indenting) which this would also allow us to move towards.
It might be good to make the 'started a discussion' line shorter. Or perhaps it is even implicit, and not necessary.
I like the 'reply' button.
By Administrator on 2012-10-18T14:22:01 (imported from GitLab project)
By Administrator on 2012-10-18T14:22:01 (imported from GitLab)
Created by: kouno
@Riyad thanks! I'm still reading the codebase and trying to get a good understanding of how you do things here, but I should be able to add them soon.
@sroth80021, I just tried to be as close as I could to github discussions to keep it simple. My only concern yet is that comments are still ordered depending on the first comment in the discussion (and not the last one added). But that's also how github handle it... So I'm still wondering if it's worth implementing.
By Administrator on 2012-10-19T02:53:06 (imported from GitLab project)
By Administrator on 2012-10-19T02:53:06 (imported from GitLab)
Created by: kouno
@Riyad how about now ? :)
There is also a big thing that I didn't cover yet, which is the javascript part. It is updating the comments on a merge request automatically, and with the new layout, it's as good as broken.
I'm wondering if I should go and modify it to update discussions automatically too (which is nothing short of rewriting a good portion of the code I think), or if I should just disable it... Since you apparently coded a good part of it, I would gladly have your input here.
By Administrator on 2012-10-21T03:32:21 (imported from GitLab project)
By Administrator on 2012-10-21T03:32:21 (imported from GitLab)
81 81 closed true 82 82 end 83 83 84 trait :with_diffs do 85 st_commits do 86 repo = Grit::Repo.new(Rails.root.join('tmp', 'repositories', 'gitlabhq')) 87 [Commit.new(repo.commit('bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a'))] 88 end 89 st_diffs do 81 81 closed true 82 82 end 83 83 84 trait :with_diffs do 85 st_commits do 86 repo = Grit::Repo.new(Rails.root.join('tmp', 'repositories', 'gitlabhq')) 87 [Commit.new(repo.commit('bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a'))] 88 end 89 st_diffs do 90 diffs = [] 81 81 closed true 82 82 end 83 83 84 trait :with_diffs do 85 st_commits do 86 repo = Grit::Repo.new(Rails.root.join('tmp', 'repositories', 'gitlabhq')) 87 [Commit.new(repo.commit('bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a'))] 88 end 89 st_diffs do 90 diffs = [] 91 st_commits.each do |commit| 92 diffs += commit.diffs 93 end 94 diffs Created by: dzaporozhets
@Riyad can I assign this to you?
By Administrator on 2012-10-25T11:03:06 (imported from GitLab project)
By Administrator on 2012-10-25T11:03:06 (imported from GitLab)
Created by: dzaporozhets
@Riyad thank you
By Administrator on 2012-10-25T13:06:14 (imported from GitLab project)
By Administrator on 2012-10-25T13:06:14 (imported from GitLab)
- app/helpers/discussion_helper.rb 0 → 100644
20 { note: note } 21 end 22 end 23 24 def discussion_notes(note) 25 @notes.select do |other_note| 26 discussion_id_for(note) == discussion_id_for(other_note) 27 end 28 end 29 30 def discussion_id_for(note) 31 [note.line_code, note.noteable_id, note.noteable_type] 32 end 33 34 def discussions_for(note) 35 @discussions ||= { merge_requests: [], commits: [] } Created by: riyad
@kouno can you explain why you need to split merge request and commit discussions here. Wouldn't it be enough to have a flat list with discussion ids, because the noteable_type is included in it?
By Administrator on 2012-10-27T22:40:04 (imported from GitLab project)
By Administrator on 2012-10-27T22:40:04 (imported from GitLab)
- app/helpers/discussion_helper.rb 0 → 100644
20 { note: note } 21 end 22 end 23 24 def discussion_notes(note) 25 @notes.select do |other_note| 26 discussion_id_for(note) == discussion_id_for(other_note) 27 end 28 end 29 30 def discussion_id_for(note) 31 [note.line_code, note.noteable_id, note.noteable_type] 32 end 33 34 def discussions_for(note) 35 @discussions ||= { merge_requests: [], commits: [] } - app/helpers/discussion_helper.rb 0 → 100644
2 def part_of_discussion?(note) 3 note.for_commit? || note.for_merge_request_diff_line? 4 end 5 6 def has_rendered?(note) 7 discussions_for(note).include?(discussion_id_for(note)) 8 end 9 10 def discussion_rendered!(note) 11 discussions_for(note).push(discussion_id_for(note)) 12 end 13 14 def discussion_params(note) 15 if note.for_diff_line? 16 @reply_allowed = true 17 @line_notes = discussion_notes(note) Created by: riyad
This is wrong (as in morally wrong ;) ). You shouldn't be setting any @variable from the view layer (helpers belong to the view). If you really need something to do some house keeping, at least make it obvious that it's something out of the ordinary (call it @_foo or something). But under no circumstances should these then be accessed from a view template.
By Administrator on 2012-10-27T22:54:01 (imported from GitLab project)
By Administrator on 2012-10-27T22:54:01 (imported from GitLab)
- app/helpers/discussion_helper.rb 0 → 100644
2 def part_of_discussion?(note) 3 note.for_commit? || note.for_merge_request_diff_line? 4 end 5 6 def has_rendered?(note) 7 discussions_for(note).include?(discussion_id_for(note)) 8 end 9 10 def discussion_rendered!(note) 11 discussions_for(note).push(discussion_id_for(note)) 12 end 13 14 def discussion_params(note) 15 if note.for_diff_line? 16 @reply_allowed = true 17 @line_notes = discussion_notes(note) Created by: riyad
@kouno This is on top of your comits. Please have a look at https://github.com/riyad/gitlabhq/compare/4dfcd67bf0eb795a3e9557219695c8c27d56c961...f7461cf85b85f2d66cda1920b4c6823fb4463546 for what you should be doing. Especially the controller (riyad@3a119c2) and view (riyad@f7461cf) portions.
By Administrator on 2012-10-28T02:55:11 (imported from GitLab project)
By Administrator on 2012-10-28T02:55:11 (imported from GitLab)
- app/helpers/discussion_helper.rb 0 → 100644
2 def part_of_discussion?(note) 3 note.for_commit? || note.for_merge_request_diff_line? 4 end 5 6 def has_rendered?(note) 7 discussions_for(note).include?(discussion_id_for(note)) 8 end 9 10 def discussion_rendered!(note) 11 discussions_for(note).push(discussion_id_for(note)) 12 end 13 14 def discussion_params(note) 15 if note.for_diff_line? 16 @reply_allowed = true 17 @line_notes = discussion_notes(note) Created by: riyad
I've been playing around with this feature, but there are a lot of subtle things that are broken. :( e.g. Replying to a line note from a discussion will add it to multiple diffs if they happen to have the same line code, diffs don't get truncated to the "relevant" parts, vote counting is off when a commit note has +1 in it, you shouldn't be able to add diff notes at a new position in a discussion diff, ... So, even with the refactoring above, there is almost none of the original code untouched (though the idea is in there ;) ). I started from the refactoring above and did a clean room implementation. I also took the liberty to go a bit further and also fix a few inconsistencies and retouch the styling in some areas. (You can compare them from https://github.com/riyad/gitlabhq/branches . They are named "kouno-grouped-notes-on-merge-request" and "discussions")
I'm sorry, but this PR won't make it in this form.
By Administrator on 2012-10-30T10:20:30 (imported from GitLab project)
By Administrator on 2012-10-30T10:20:30 (imported from GitLab)
Created by: kouno
Well, I knew this wouldn't pass as is. I was still going through tests and playing around with it too. But I completely missed some of the bugs you referenced. sigh (I think I would have never noticed the +1 feature issue)
Anyway, I see your point.
Do you want to take over this feature since you already nearly rewrote everything? Unless you give me another chance to rewrite it, but I believe you already pinned down the beast pretty well. :)
By Administrator on 2012-10-30T11:03:04 (imported from GitLab project)
By Administrator on 2012-10-30T11:03:04 (imported from GitLab)
Created by: riyad
I'll take this over. Don't feel let down or anything, even though your code didn't make it in, you were the reason GitLab will have this feature in the next release. :) Without your efforts I don't know when we would have come around to tackle this. ;) Future PRs are still welcome. ;)
As for the endless scrolling (and also refreshing notes after page load): this is the single biggest unsolved problem with this feature. Ideas are welcome. ;)
By Administrator on 2012-10-30T14:22:13 (imported from GitLab project)
By Administrator on 2012-10-30T14:22:13 (imported from GitLab)
Created by: kouno
Thank you. I obviously will try again to contribute to Gitlab ;).
For endless scrolling, I think it would be alright to disable it. It really is good when you have a straight forward listing structure, but as soon as discussions come in, it just make it too complex. I used a scope on MergeRequest and Commit to retrieve a discussion, and I tried to display a discussion only if the first element of the discussion was in the 20 elements called by endless scroll. This can create weird behaviors such as having a request for notes which returns only a few notes (since the discussion was already displayed before), or even returns empty (if there is a lot of comments on a discussion).
A pull request won't have a thousand comments anyway, and even if it's the case, I don't think it would be a problem. (and did I say that github don't use it either? :))
By Administrator on 2012-10-30T22:09:39 (imported from GitLab project)
By Administrator on 2012-10-30T22:09:39 (imported from GitLab)
Created by: riyad
I have updated my branch for this feature: https://github.com/riyad/gitlabhq/tree/discussions It does some half-hearted diff truncation and also includes a rather bloated approach of fishing for new notes and moving them to the appropriate discussion (as can be seen in riyad@58ed5396) :/ As you correctly pointed out, it makes the whole comments section a pain to deal with. Continuous loading of notes (anywhere but the wall) is error prone and bloats the whole comments feature (which has become even bigger with the addition of discussions). It's also very inconsistent: diff line comments (i.e. for commit and mr diffs) are always loaded all at once. All other (non-diff line notes) are loaded in batches of 20. o.O
So you are right again. :) There shouldn't be ridiculous amounts of comments on a merge request, so we might get away with loading them all at once. So axing continuous loading (or bumping the ceiling quite high) anywhere but on the wall makes IMHO a lot of sense. :)
By Administrator on 2012-10-31T00:43:23 (imported from GitLab project)
By Administrator on 2012-10-31T00:43:23 (imported from GitLab)
