Can't comment to a commit

When I try to comment with only :+1: to this commit, I'm not able to submit:

https://gitlab.com/gitlab-com/www-gitlab-com/commit/16d7a7af921af592bdb1433d5707d3a2fdb49059#note_2794584

Making it :+1: great, works though.

@grzesiek replicated it.

cc @vsizov

I also saw few errors in console:

Uncaught ReferenceError: awards_handler is not defined
application-3f06907660dd7f5126c13c5f28ac11bd.js:43 

Failed to load resource: the server responded with a status of 500 (Internal Server Error)
https://gitlab.com/gitlab-com/www-gitlab-com/notes

Failed to load resource: the server responded with a status of 500 (Internal Server Error)
https://gitlab.com/gitlab-com/www-gitlab-com/notes/2803949

Duplicate of #3733 (closed) but we'll keep this issue.

Milestone changed to 8.3

mentioned in issue #3733 (closed)

I think this warrants a patch release, it is a common thing to do and it is really confusing. /cc @rspeicher

@sytses I would like to pick this up tomorrow, if it is ok.

@grzesiek No need to ask, just assign to yourself.

Reassigned to @grzesiek

@grzesiek feel free to ask questions because this code is fresh in my head yet.

@vsizov Thanks Valery, I think that looking how things are done will also be good practice for me now :)

mentioned in merge request !1936 (merged)

I think we should unify the two ways to add emoji. When using the newly introduced award button, a comment should be posted, only containing the selected emoji, which I think is currently implemented. These comments are hidden from the feed. Alternatively, they could be shown as a .system-note. Posting a comment containing only an emoji has the same effect.

On the other hand, all comments containing an emoji at the beginning followed by some other text should be counted as awards. This is currently not the case.

Also, editing a comment and replacing its content with an emoji currently does not have any effect on the awards.

I talked with @vsizov and @JobV about this feature (possibility to add award-emoji by adding single-emoji comment). User adds a comment with single-emoji but instead of appearing as a subsequent comment, award-emoji is added in awards-bar at the top of the issues page.

Valery and Job suggested that this is a good feature, as it decreases clutter. My suggestion was that adding comment with single emoji is also something stronger than award-emoji, and having those two mechanisms separated is in fact additional feature (multiple people can express opinion using award emoji, but other people - e.g. someone who is authorized to or has higher rank in an organisation - can add comments also; such comments will be more visible and, above all, will be tighter bounded to person who posts this emoji-comment).

However, we decided to follow the path of minimizing clutter.

There is also a problem in adding award-emoji by submitting a comment with single emoji, when such award-emoji already exists. It raises ActionController::UrlGenerationError in Projects::Notes#create.

Editing commit's comment with single-emoji content raises an error NoMethodError in Projects::NotesController#update

@grzesiek Keep in mind the commit notes should be emoji-agnostic. I mean we don't support award emoji on that page (just in case)

ActionController::UrlGenerationError in Projects::Notes#create it's problem of notes validation. If note is invalid it fails with this error. When you repeat the same emoji by posting it in comment, the actual note will be invalid and that's why that error appears (we can see it in browser debugger, user can't see it). I'm not sure how we should handle it.

@vsizov Yes, I know that we don't support award-emojis in commit notes. This is actually what I'm working on. I'm trying to find best approach to distinguish this two cases.

I've already found problem with this validation. And I'm not really sure what business rules are best here - should we just notify user that this emoji is already set, or unset this award-emoji ?

not sure about "unset this award-emoji" because POST create should not remove notes, it would be weird :)

What about "should we just notify user that this emoji is already set" it's maybe the best approach for now.

@vsizov Ok, I will work on notifying user if award-emoji is already set.

Another question - what could be a best approach to distinguish notes being added in commits view from notes in discussion view ?

Note to myself: emoji images inside a note, have invalid url address in my local development setup - need to investigate this.

Note to myself: emoji images inside a note, have invalid url address in my local development setup - need to investigate this.

Try to update development-kit.

@vsizov Just done this today morning with make update, I will try again maybe.

mentioned in commit 2b577551

mentioned in issue #3809 (closed)

Status changed to closed by commit 359d9460

mentioned in commit 359d9460

mentioned in commit 23975e09

mentioned in issue #3902 (closed)

mentioned in merge request !14785