New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reactions to issues/PR and comments #2856

Merged
merged 4 commits into from Dec 3, 2017

Conversation

7 participants
@lafriks
Member

lafriks commented Nov 5, 2017

Screenshots:

attels

attels

For unauthorized users and tooltip to show reacted users:
image

@lafriks lafriks added this to the 1.x.x milestone Nov 5, 2017

@andreynering

This comment has been minimized.

Contributor

andreynering commented Nov 7, 2017

Seems good.

Just a question to the team: Should it really look almost identical to GitHub's reactions? Or should it be different to don't look like a GitHub's copy?

@tboerger tboerger added the lgtm/need 2 label Nov 7, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 7, 2017

@andreynering reactions looks same also in other products (mattermost, discord etc)

@andreynering

This comment has been minimized.

Contributor

andreynering commented Nov 7, 2017

reactions looks same also in other products (mattermost, discord etc)

Yes, you're right. I think it's fine, then.

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 7, 2017

As for these 6 reactions and their naming it just for being API compatible with github

func (issue *Issue) GetReactionsByType() map[string]ReactionList {
var reactions = make(map[string]ReactionList)
for _, reaction := range issue.Reactions {
list, ok := reactions[reaction.Type]

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 8, 2017

Member

Can simplify to reactions[reaction.Type] = append(reactions[reaction.Type], reaction)

Also in Comment.GetReactionsByType().

@@ -1350,6 +1350,40 @@
}
}
}
.segment.reactions, .select-reaction {
&.dropdown .menu {
right: 0!important;

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 8, 2017

Member

I'm not a CSS expert, but !important is generally considered a bad practice. Are these necessary?

This comment has been minimized.

@lafriks

lafriks Nov 8, 2017

Member

Sadly but it's needed as semantic UI is full of these :(

return
}
if ctx.HasError() {

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 8, 2017

Member

What does this if block accomplish?

This comment has been minimized.

@lafriks

lafriks Nov 8, 2017

Member

I think yes but I will verify. It's to check if there is form validation error

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 29, 2017

Member

I'm still confused as to why this if block is necessary

ID int64 `xorm:"pk autoincr"`
Type string `xorm:"INDEX UNIQUE(s) NOT NULL"`
IssueID int64 `xorm:"INDEX UNIQUE(s) NOT NULL"`
CommentID int64 `xorm:"INDEX UNIQUE(s)"`

This comment has been minimized.

@bkcsoft

bkcsoft Nov 9, 2017

Member

Foreign key please? 😄

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 9, 2017

Member

@bkcsoft IIRC, xorm does not support foreign keys 😢

This comment has been minimized.

@lafriks

lafriks Nov 9, 2017

Member

And what also I don't quite like it inserts 0 values for IDs where it would normally should be null

This comment has been minimized.

@bkcsoft

bkcsoft Nov 23, 2017

Member

Why would it ever insert 0/null ? It would throw foreign key constrain and that would be that 😛

@lunny

This comment has been minimized.

Member

lunny commented Nov 21, 2017

@lafriks It's ready for review so that we can add it to v1.4?

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 21, 2017

@lunny still WIP, will finish it this week

@@ -22,6 +22,7 @@
{{end}}
</div>
{{end}}
{{template "repo/issue/view_content/reactions" Dict "ctx" $ "ActionURL" (Printf "%s/issues/%d/comment/%d/reactions" $.RepoLink $.Issue.Index .ID) "Reactions" .GetReactionsByType }}

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 23, 2017

Member

Why can't we just do {{template "repo/issue/view_content/reactions" .}} like we do elsewhere?

This comment has been minimized.

@lafriks

lafriks Nov 24, 2017

Member

@ethantkoenig because this template needs to be used in loop and loop variable needs to be passed to that template

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 24, 2017

Member

Oh I see. On another note, there aren't handlers for the :owner/:repo/issues/:index/comments/:id/reactions route, so trying to add a reaction to a comment always results in a 404.

$(container + '.select-reaction > .menu > .item').on('click', function(e){
e.preventDefault();
var url = $(this).closest('.select-reaction').data('action-url')
+ '/' + ($(this).hasClass('is-active') ? 'unreact' : 'react');

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 24, 2017

Member

This is-active class isn't used anywhere, so the action is always react, and it's impossible to unreact.

{{range $key, $value := .Reactions}}
<a class="ui label basic{{if $value.HasUser $.ctx.SignedUserID}} blue{{end}} has-emoji">
{{if eq $key "hooray"}}
:tada:

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 24, 2017

Member

There are no js handlers for clicking on these

This comment has been minimized.

@lafriks

lafriks Nov 24, 2017

Member

Yes, unreacting is not yet implemented :)

@@ -22,6 +22,7 @@
{{end}}
</div>
{{end}}
{{template "repo/issue/view_content/reactions" Dict "ctx" $ "ActionURL" (Printf "%s/issues/%d/comment/%d/reactions" $.RepoLink $.Issue.Index .ID) "Reactions" .GetReactionsByType }}

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 24, 2017

Member

Oh I see. On another note, there aren't handlers for the :owner/:repo/issues/:index/comments/:id/reactions route, so trying to add a reaction to a comment always results in a 404.

@@ -19,6 +19,7 @@
<div class="ui top attached header">
<span class="text grey"><a {{if gt .Issue.Poster.ID 0}}href="{{.Issue.Poster.HomeLink}}"{{end}}>{{.Issue.Poster.Name}}</a> {{.i18n.Tr "repo.issues.commented_at" .Issue.HashTag $createdStr | Safe}}</span>
<div class="ui right actions">
{{template "repo/issue/view_content/reactions" Dict "ctx" $ "ActionURL" (Printf "%s/issues/%d/reactions" $.RepoLink .Issue.Index) "Reactions" .GetReactionsByType }}

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 24, 2017

Member

.GetReactionsByType is not defined in the current context, so .GetReactionsByType will evaluate to nil.

In this case, it gets us what we want (we don't want to show the current reactions in the issue header, just the button for adding reactions, and the reactions template won't render the current reactions if Reactions is nil), but this is confusing and non-obvious.

Can we make this more straightforward? My proposal would be to have two separate templates: one for displaying the current reaction, and another template for the button/pop-out to add new reactions. In the issue header, we would only use the second template, but in other places we would use both.

@lafriks lafriks force-pushed the lafriks:feat/comment_reactions branch 4 times, most recently from 8133bce to 23a2415 Nov 26, 2017

@lafriks lafriks removed the status/wip label Nov 27, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 27, 2017

Ok, I think I have finished it now, so feel free to test and review it.

@lafriks lafriks modified the milestones: 1.x.x, 1.4.0 Nov 27, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 27, 2017

@ethantkoenig I think have fixed all recommendations

@codecov-io

This comment has been minimized.

codecov-io commented Nov 27, 2017

Codecov Report

Merging #2856 into master will decrease coverage by 0.14%.
The diff coverage is 12.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2856      +/-   ##
==========================================
- Coverage   33.43%   33.29%   -0.15%     
==========================================
  Files         270      272       +2     
  Lines       39595    39939     +344     
==========================================
+ Hits        13239    13297      +58     
- Misses      24466    24741     +275     
- Partials     1890     1901      +11
Impacted Files Coverage Δ
models/migrations/migrations.go 2.9% <ø> (ø) ⬆️
modules/setting/setting.go 46.93% <ø> (ø) ⬆️
models/issue_comment.go 51.62% <0%> (-2.12%) ⬇️
models/helper.go 66.66% <0%> (-33.34%) ⬇️
modules/auth/repo_form.go 32% <0%> (-0.88%) ⬇️
models/migrations/v50.go 0% <0%> (ø)
models/user.go 40.45% <100%> (+0.06%) ⬆️
models/models.go 49.56% <100%> (+0.22%) ⬆️
modules/context/context.go 49.65% <100%> (ø) ⬆️
routers/routes/routes.go 87.03% <100%> (+0.04%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e59adcd...3410384. Read the comment docs.

@@ -808,6 +835,15 @@ func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (err error) {
return nil
}
// GetReactionsByType returns issue reactions grouped by type
func (issue *Issue) GetReactionsByType() map[string]ReactionList {

This comment has been minimized.

@lunny

lunny Nov 28, 2017

Member

Maybe GetReactionsMap or GetReactionsGroupByType better name.

}
// GetReactionsByType returns comment reactions grouped by type
func (c *Comment) GetReactionsByType() map[string]ReactionList {

This comment has been minimized.

@lunny

lunny Nov 28, 2017

Member

The same as above

This comment has been minimized.

@lunny

lunny Nov 28, 2017

Member

Or maybe

func (reactions ReactionList) GroupByType() map[string]ReactionList {
        var returns = make(map[string]ReactionList)
        for _, reaction := range reactions {
 		returns[reaction.Type] = append(returns[reaction.Type], reaction)
 	}
  	return returns
}

And you can remove the above two methods.

@lunny

This comment has been minimized.

Member

lunny commented Nov 28, 2017

image
It seems it's not correct popup.

@lunny

This comment has been minimized.

Member

lunny commented Nov 28, 2017

And when click the reations, no request was sent to server.

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 28, 2017

@lunny did you do Ctrl+f5 seems css&js was used from browser cache

@lunny

This comment has been minimized.

Member

lunny commented Nov 28, 2017

image
firefox and safari on macOS are almost right. But on safari, see above screenshot. I will clear chrome cache and try it again.

@lunny

This comment has been minimized.

Member

lunny commented Dec 3, 2017

@lafriks need rebase

@lafriks lafriks force-pushed the lafriks:feat/comment_reactions branch from 77a3728 to 5460507 Dec 3, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 3, 2017

@ethantkoenig rebased and fixed your reported issues

@lunny

This comment has been minimized.

Member

lunny commented Dec 3, 2017

Lint error

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 3, 2017

@lunny already fixed that but drone did not pick up latest commit correctly :(

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 3, 2017

@lafriks You get a 500 if you view an issue that a deleted user reacted to. I think we need to delete all of a user's reactions when a user deletes his/her account.

@bkcsoft

This comment has been minimized.

Member

bkcsoft commented Dec 3, 2017

I think we need to delete all of a user's reactions when a user deletes his/her account.

Hence foreign keys...

@bkcsoft

This comment has been minimized.

Member

bkcsoft commented Dec 3, 2017

@lafriks lafriks force-pushed the lafriks:feat/comment_reactions branch from 762066c to 285a810 Dec 3, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 3, 2017

@ethantkoenig @bkcsoft Added checks for deleted user and reaction deletion when deleting user

// GetFirstUsers returns first 10 reacted user display names seperated by comma
func (list ReactionList) GetFirstUsers() string {
var buffer bytes.Buffer
var rem = 10

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 3, 2017

Member

Move to a named constant, since it is used multiple places?

@lafriks lafriks force-pushed the lafriks:feat/comment_reactions branch from 8b76f5a to d2add53 Dec 3, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 3, 2017

@ethantkoenig moved that hardcoded limit to UI settings

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 3, 2017

LGTM

@tboerger tboerger added lgtm/done and removed lgtm/need 1 labels Dec 3, 2017

@lafriks lafriks force-pushed the lafriks:feat/comment_reactions branch from cc58235 to d2add53 Dec 3, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 3, 2017

@ethantkoenig need your approval

@ethantkoenig

Forgot to approve 😛

lafriks added some commits Dec 3, 2017

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 3, 2017

@lafriks I don't mean to unnecessarily delay this PR, but it is too late to ask for tests?

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 3, 2017

@ethantkoenig I will do them in other PR

@lafriks lafriks merged commit 5dc37b1 into go-gitea:master Dec 3, 2017

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@lafriks lafriks deleted the lafriks:feat/comment_reactions branch Dec 3, 2017

@lafriks lafriks referenced this pull request Dec 4, 2017

Merged

Tests for reactions #3083

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment