Add option to start a new resolvable discussion in an MR
Resolves https://gitlab.com/gitlab-org/gitlab-ce/issues/24378
EE port: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1556
To do:
-
Specs -
Reply by email support -
Better notifications (https://gitlab.com/gitlab-org/gitlab-ce/issues/24378#note_25365790) -
Consider the case where a new diff comment can become separated of an existing discussion (comment created while existing discussion was not visible, for example before it was loaded - uncommon, or when its position was out of date - unlikely) -
Remove/ignore the notes.original_discussion_id
column -
Documentation https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10390
Out of scope:
- API support (https://gitlab.com/gitlab-org/gitlab-ce/issues/20901)
Button in form:
After tabbing to the button:
After hitting Command+Option+Return (or Ctrl-Alt-Enter on Windows), a new discussion appears:
/cc @smcgivern
Merge request reports
Activity
Mentioned in issue #24378 (closed)
Added 464 commits:
-
bde566a1...9ad0d879 - 463 commits from branch
master
- ce65e61f - WIP: Add option to start a new resolvable discussion in an MR
-
bde566a1...9ad0d879 - 463 commits from branch
@victorwu this is something Douwe and I discussed before. He's taken it upon himself to implement this :D Feel free to get involved!
Looking awesome @DouweM. I'll participate in the issue comment thread.
Added 789 commits:
-
ce65e61f...d0c0c75c - 788 commits from branch
master
- cde1449b - WIP
-
ce65e61f...d0c0c75c - 788 commits from branch
Added 180 commits:
-
cde1449b...8fad76b6 - 179 commits from branch
master
- e5c2269d - WIP
-
cde1449b...8fad76b6 - 179 commits from branch
Added 116 commits:
-
aa42c3ea...09806605 - 115 commits from branch
master
- 842486f5 - WIP
-
aa42c3ea...09806605 - 115 commits from branch
added 5480 commits
-
842486f5...2cf17c42 - 5478 commits from branch
master
- 17e2d79e - WIP
- 677268ae - WIP
-
842486f5...2cf17c42 - 5478 commits from branch
mentioned in issue #20901 (closed)
@DouweM Are you happy with me getting started on the frontend on a different branch?
added 868 commits
-
677268ae...2d006b46 - 867 commits from branch
master
- d295383c - Merge branch 'master' into new-resolvable-discussion
-
677268ae...2d006b46 - 867 commits from branch
I added a merge commit from
master
just to get things running again. Will continue commits to a different branch. (!9701 (closed))Edited by Luke BennettI see this adds discussions for MRs and commits, judging by the original issue it should also be available on issues, maybe even snippets e.t.c?
Edited by Luke Bennettmentioned in merge request !9701 (closed)
@lbennett Only on MRs for now, I was playing with commits but that won't ship (that's why the commits are WIP). Issues and snippets are totally out of scope right now.
The point is to add these to MRs, where they can be resolved just like comments on the diff can be.
I'll be doing some refactoring on the backend, but you should be able to work on the frontend, as long as you focus on the MR page :)
added 81 commits
-
d295383c...eb7b03af - 77 commits from branch
master
- 51224be0 - WIP
- 046280ae - WIP
- f9329101 - WIP
- 4dc97edc - WIP
Toggle commit list-
d295383c...eb7b03af - 77 commits from branch
added 419 commits
-
3cfc5490...d34300e3 - 417 commits from branch
master
- 3e07a05d - WIP
- f05ff965 - WIP
-
3cfc5490...d34300e3 - 417 commits from branch
- Resolved by Luke Bennett
mentioned in issue #29266 (closed)
added 83 commits
-
50866e49...6239bf72 - 82 commits from branch
master
- 63726a5e - Add option to start a new discussion on an MR
-
50866e49...6239bf72 - 82 commits from branch
added 86 commits
-
63726a5e...32da7602 - 85 commits from branch
master
- f1493840 - Add option to start a new discussion on an MR
-
63726a5e...32da7602 - 85 commits from branch
added 39 commits
-
f1493840...06c3c71b - 36 commits from branch
master
- c605d4f0 - Add option to start a new discussion on an MR
- 8a1ecd82 - Satisfy Rubocop
- ad60a472 - Fix some specs
Toggle commit list-
f1493840...06c3c71b - 36 commits from branch
- Resolved by Douwe Maan
added 127 commits
-
ad60a472...b7166806 - 124 commits from branch
master
- b890294c - Add option to start a new discussion on an MR
- 999e1161 - Satisfy Rubocop
- 20b5d753 - Fix some specs
Toggle commit list-
ad60a472...b7166806 - 124 commits from branch
I rebased against master, got some crazy conflicts in another branch and I think this might help, sorry!
Edited by Luke Bennettadded 18 commits
-
20b5d753...9226ce9e - 15 commits from branch
master
- 1335d81e - Add option to start a new discussion on an MR
- 9aac980a - Satisfy Rubocop
- d2e55c1a - Fix some specs
Toggle commit list-
20b5d753...9226ce9e - 15 commits from branch
- Resolved by Luke Bennett
added 5 commits
-
d2e55c1a...79ef2f89 - 2 commits from branch
master
- 8044e458 - Add option to start a new discussion on an MR
- 21b47123 - Satisfy Rubocop
- 2e9d44a8 - Fix some specs
Toggle commit list-
d2e55c1a...79ef2f89 - 2 commits from branch
@lbennett No problem. Did you just rebase against master or did some other stuff to? If it's just the rebase, I can force push the rebase + some other changes I already did locally :)
@DouweM Just rebased against master, no conflicts either.
marked the task Better notifications (https://gitlab.com/gitlab-org/gitlab-ce/issues/24378#note_25365790) as completed
added 51 commits
Toggle commit listadded 1 commit
- 5dabf302 - Better notification emails for notes and (diff) discussions
@rspeicher Can you please do a preliminary review? I have yet to add some code comments around tricky bits, as well as specs and docs, but otherwise it should be functionally complete (aside from whatever changes https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9701 will bring). I'd like to get some input on the approach before I write specs that may have to be removed/refactored a lot :)
assigned to @rspeicher
added 1 commit
- 8e5151ef - Enable discussions on issues, commits and snippets
That looks good to me @DouweM - thanks!
added 146 commits
-
8e5151ef...1ac6bb55 - 138 commits from branch
master
- 77778205 - Add option to start a new discussion on an MR
- 5a532210 - Satisfy Rubocop
- 9a4716fe - Fix some specs
- bcf26918 - Fix merge conflict issue
- a5b7b894 - Fix field name
- bbecf968 - Better notification emails for notes and (diff) discussions
- 37f1ed05 - Enable discussions on issues, commits and snippets
- b79133ca - Add specs
Toggle commit list-
8e5151ef...1ac6bb55 - 138 commits from branch
added 136 commits
-
b79133ca...977f6e37 - 128 commits from branch
master
- 977beb25 - Add option to start a new discussion on an MR
- df206b9b - Satisfy Rubocop
- 5c764739 - Fix some specs
- 44766f5a - Fix merge conflict issue
- 3be8e06b - Fix field name
- cd0099d8 - Better notification emails for notes and (diff) discussions
- 44b3273d - Enable discussions on issues, commits and snippets
- 59d64900 - Add specs
Toggle commit list-
b79133ca...977f6e37 - 128 commits from branch