PR#3246: Add comment reactions - pagure

pagure

#3246 Add comment reactions

Merged 5 years ago by pingou. Opened 5 years ago by lsedlar.

lsedlar/pagure reactions into master

Fix formatting to conform to line length limit

Lubomír Sedlář • 5 years ago

9339088

Add comment reactions

Lubomír Sedlář • 5 years ago

59889e5

alembic/versions/e34e799e4182_add_reactions_to_issue_comments.py

file added

+29

		`@@ -0,0 +1,29 @@`
		`+ """add_reactions_to_comments`
		`+`
		`+ Revision ID: e34e799e4182`
		`+ Revises: 131ad2dc5bbd`
		`+ Create Date: 2018-05-17 18:44:32.189208`
		`+`
		`+ """`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = 'e34e799e4182'`
		`+ down_revision = '131ad2dc5bbd'`
		`+`
		`+ from alembic import op`
		`+ import sqlalchemy as sa`
		`+`
		`+`
		`+ def upgrade():`
		`+ ''' Add the _reactions column to table issue_comments.`
		`+ '''`
		`+ op.add_column(`
		`+ 'issue_comments',`
		`+ sa.Column('_reactions', sa.Text, nullable=True)`
		`+ )`
		`+`
		`+`
		`+ def downgrade():`
		`+ ''' Remove the column _reactions from table issue_comments.`
		`+ '''`
		`+ op.drop_column('issue_comments', '_reactions')`

alembic/versions/ef98dcb838e4_add_reactions_to_request_comments_py.py

file added

+29

		`@@ -0,0 +1,29 @@`
		`+ """add_reactions_to_request_comments.py`
		`+`
		`+ Revision ID: ef98dcb838e4`
		`+ Revises: e34e799e4182`
		`+ Create Date: 2018-06-02 19:47:52.975503`
		`+`
		`+ """`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = 'ef98dcb838e4'`
		`+ down_revision = 'e34e799e4182'`
		`+`
		`+ from alembic import op`
		`+ import sqlalchemy as sa`
		`+`
		`+`
		`+ def upgrade():`
		`+ ''' Add the _reactions column to table pull_request_comments.`
		`+ '''`
		`+ op.add_column(`
		`+ 'pull_request_comments',`
		`+ sa.Column('_reactions', sa.Text, nullable=True)`
		`+ )`
		`+`
		`+`
		`+ def downgrade():`
		`+ ''' Remove the column _reactions from table pull_request_comments.`
		`+ '''`
		`+ op.drop_column('pull_request_comments', '_reactions')`

pagure/default_config.py

file modified

+11

		`@@ -422,3 +422,14 @@`
		`# Allows to require that the users are members of a certain group to be added`
		`# to a project (not a fork).`
		`REQUIRED_GROUPS = {}`
		`+`
		`+ # Predefined reactions. Selecting others is possible by typing their name. The`
		`+ # order here will be preserved in the web UI picker for reactions.`
		`+ REACTIONS = [`
		`+ ("Thumbs up", "emojione-1F44D"), # Thumbs up`
		`+ ("Thumbs down", "emojione-1F44E"), # Thumbs down`
		`+ ("Confused", "emojione-1F615"), # Confused`
		`+ ("Heart", "emojione-2764"), # Heart`
		`+ ]`
		`+ # This is used for faster indexing. Do not change.`
		`+ _REACTIONS_DICT = dict(REACTIONS)`

pagure/lib/model.py

file modified

+34

		`@@ -1416,6 +1416,8 @@`
		`foreign_keys=[editor_id],`
		`remote_side=[User.id])`

		`+ _reactions = sa.Column(sa.Text, nullable=True)`
		`+`
		`@property`
		`def mail_id(self):`
		`''' Return a unique reprensetation of the issue as string that`
		`@@ -1429,6 +1431,20 @@`
		`''' Return the parent, in this case the issue object. '''`
		`return self.issue`

		`+ @property`
		`+ def reactions(self):`
		`+ ''' Return the reactions stored as a string in the database parsed as`
		`+ an actual dict object.`
		`+ '''`
		`+ if self._reactions:`
		`+ return json.loads(self._reactions)`
		`+ return {}`
		`+`
		`+ @reactions.setter`
		`+ def reactions(self, reactions):`
		`+ ''' Ensures that reactions are properly saved. '''`
		`+ self._reactions = json.dumps(reactions)`
		`+`
		`def to_json(self, public=False):`
		`''' Returns a dictionary representation of the issue.`

		`@@ -1443,6 +1459,7 @@`
		`'editor': self.editor.to_json(public=public)`
		`if self.editor_id else None,`
		`'notification': self.notification,`
		`+ 'reactions': self.reactions,`
		`}`
		`return output`

		`@@ -2000,6 +2017,8 @@`
		`foreign_keys=[editor_id],`
		`remote_side=[User.id])`

		`+ _reactions = sa.Column(sa.Text, nullable=True)`
		`+`
		`@property`
		`def mail_id(self):`
		`''' Return a unique representation of the issue as string that`
		`@@ -2013,6 +2032,20 @@`
		`''' Return the parent, in this case the pull_request object. '''`
		`return self.pull_request`

		`+ @property`
		`+ def reactions(self):`
		`+ ''' Return the reactions stored as a string in the database parsed as`
		`+ an actual dict object.`
		`+ '''`
		`+ if self._reactions:`
		`+ return json.loads(self._reactions)`
		`+ return {}`
		`+`
		`+ @reactions.setter`
		`+ def reactions(self, reactions):`
		`+ ''' Ensures that reactions are properly saved. '''`
		`+ self._reactions = json.dumps(reactions)`
		`+`
		`def to_json(self, public=False):`
		`''' Return a dict representation of the pull-request comment. '''`

		`@@ -2030,6 +2063,7 @@`
		`'editor': self.editor.to_json(public=public)`
		`if self.editor_id else None,`
		`'notification': self.notification,`
		`+ 'reactions': self.reactions,`
		`}`

pagure/static/reactions.js

file added

+37

		`@@ -0,0 +1,37 @@`
		`+ (function () {`
		`+ function send_reaction(commentid, reaction, emoji) {`
		`+ var _url = location.href + '/comment/' + commentid + '/react';`
		`+ var csrf_token = $("#csrf_token").val();`
		`+ $.ajax({`
		`+ url: _url + '?js=1',`
		`+ type: 'POST',`
		`+ data: {`
		`+ 'reaction': reaction,`
		`+ 'csrf_token': csrf_token,`
		`+ },`
		`+ success: function () {`
		`+ var reactions = $(".issue_reactions[data-comment-id=" + commentid + "]>.btn-group");`
		`+ var selected = reactions.find("span[class=" + emoji + "]");`
		`+ if (selected.length > 0) {`
		`+ var counter = selected.next();`
		`+ counter.text(parseInt(counter.text(), 10) + 1);`
		`+ } else {`
		`+ reactions.append(`
		`+ '<button class="btn btn-outline-secondary btn-sm" type="button">' +`
		`+ ' <span class="' + emoji + '"></span>' +`
		`+ ' <span class="count">' + 1 + '</span>' +`
		`+ '</button>');`
		`+ }`
		`+ },`
		`+ });`
		`+ }`
		`+`
		`+ $(document).ready(function () {`
		`+ $(".reaction-picker button").click(function () {`
		`+ var commentid = $(this).parents('.reaction-picker').data('comment-id');`
		`+ var reaction = $(this).attr('title');`
		`+ var emoji = $(this).find('span').attr('class');`
		`+ send_reaction(commentid, reaction, emoji);`
		`+ });`
		`+ });`
		`+ })();`

pagure/templates/_formhelper.html

file modified

+28

		`@@ -142,9 +142,37 @@`
		`{% if comment.edited_on %}`
		`<small class="text-muted">Edited {{ comment.edited_on \| humanize }} by {{ comment.editor.username }} </small>`
		`{% endif %}`
		`+ <div class="issue_reactions float-left pb-1" data-comment-id="{{ comment.id}}">`
		`+ <div class="btn-group">`
		`+ {% for r in comment.reactions \| sort %}`
		`+ <button class="btn btn-outline-secondary btn-sm"`
		`+ type="button"`
		`+ data-toggle="tooltip"`
		`+ title="{{ r }} sent by {{ comment.reactions[r] \| join_prefix(10) }}">`
		`+ <span class="{{ config.get('_REACTIONS_DICT', {})[r] }}"></span>`
		`+ <span class="count">{{ comment.reactions[r] \| length }}</span>`
		`+ </button>`
		`+ {% endfor %}`
		`+ </div>`
		`+ </div>`
		`<div class="issue_action icon float-right pb-1">`
		`<div class="btn-group" role="group" aria-label="Basic example">`
		`{% if id != 0 and g.fas_user %}`
		`+ {% if config.get('REACTIONS') %}`
		`+ <div class="btn-group dropup">`
		`+ <button class="btn btn-outline-secondary btn-sm dropdown-toggle" type="button" data-toggle="dropdown" title="Add reaction">`
		`+ <span class="oi" data-glyph="heart"></span>`
		`+ <span class="caret"></span>`
		`+ </button>`
		`+ <div class="dropdown-menu reaction-picker" data-comment-id="{{ comment.id }}">`
		`+ {% for label, emoji in config.get('REACTIONS') %}`
		`+ <button class="btn btn-outline-secondary btn-sm" type="button" title="{{ label }}">`
		`+ <span class="{{ emoji }}"></span>`
		`+ </button>`
		`+ {% endfor %}`
		`+ </div>`
		`+ </div>`
		`+ {% endif %}`
		`<a class="reply btn btn-outline-secondary btn-sm" data-toggle="tooltip"`
		`title="Reply to this comment - lose formatting">`
		`<span class="fa fa-share-square-o text-muted" title="Reply to this comment"></span>`

pagure/templates/issue.html

file modified

		`@@ -1046,5 +1046,6 @@`
		`{% if repo.quick_replies %}`
		`<script type="text/javascript" src="{{ url_for('static', filename='quick_reply.js') }}"></script>`
		`{% endif %}`
		`+ <script type="text/javascript" src="{{ url_for('static', filename='reactions.js') }}"></script>`

		`{% endblock %}`

pagure/templates/pull_request.html

file modified

		`@@ -1729,5 +1729,6 @@`
		`{% if repo.quick_replies %}`
		`<script type="text/javascript" src="{{ url_for('static', filename='quick_reply.js') }}"></script>`
		`{% endif %}`
		`+ <script type="text/javascript" src="{{ url_for('static', filename='reactions.js') }}"></script>`

		`{% endblock %}`

pagure/ui/filters.py

file modified

+12

		`@@ -682,3 +682,15 @@`
		`""" For a given flag return the bootstrap label to use`
		`"""`
		`return pagure_config['FLAG_STATUSES_LABELS'][flag.status.lower()]`
		`+`
		`+`
		`+ @UI_NS.app_template_filter('join_prefix')`
		`+ def join_prefix(values, num):`
		+ """Produce a string joining first `num` items in the list and indicate
		`+ total number total number of items.`
		`+ """`
		`+ if len(values) <= 1:`
		`+ return "".join(values)`
		`+ if len(values) <= num:`
		`+ return ", ".join(values[:-1]) + " and " + values[-1]`
		`+ return "%s and %d others" % (", ".join(values[:num]), len(values) - num)`

pagure/ui/fork.py

file modified

+70

		`@@ -1556,3 +1556,73 @@`
		`return flask.redirect(flask.url_for(`
		`'ui_ns.view_repo', repo=repo.name, username=username,`
		`namespace=namespace))`
		`+`
		`+`
		`+ _REACTION_URL_SNIPPET = (`
		`+ 'pull-request/<int:requestid>/comment/<int:commentid>/react'`
		`+ )`
		`+`
		`+`
		`+ @UI_NS.route(`
		`+ '/<repo>/%s/' % _REACTION_URL_SNIPPET, methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/<repo>/%s' % _REACTION_URL_SNIPPET, methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/<namespace>/<repo>/%s/' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/<namespace>/<repo>/%s' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<repo>/%s/' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<repo>/%s' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<namespace>/<repo>/%s/' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<namespace>/<repo>/%s' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @login_required`
		`+ def pull_request_comment_add_reaction(repo, requestid, commentid,`
		`+ username=None, namespace=None):`
		`+ repo = flask.g.repo`
		`+`
		`+ form = pagure.forms.ConfirmationForm()`
		`+ if not form.validate_on_submit():`
		`+ flask.abort(400, 'CSRF token not valid')`
		`+`
		`+ request = pagure.lib.search_pull_requests(`
		`+ flask.g.session, requestid=requestid, project_id=repo.id`
		`+ )`
		`+`
		`+ if not request:`
		`+ flask.abort(404, 'Comment not found')`
		`+`
		`+ comment = pagure.lib.get_request_comment(`
		`+ flask.g.session, request.uid, commentid)`
		`+`
		`+ if 'reaction' not in flask.request.form:`
		`+ flask.abort(400, 'Reaction not found')`
		`+`
		`+ reactions = comment.reactions`
		`+ r = flask.request.form['reaction']`
		`+ if not r:`
		`+ flask.abort(400, 'Empty reaction is not acceptable')`
		`+ if flask.g.fas_user.username in reactions.get(r, []):`
		`+ flask.abort(409, 'Already posted this one')`
		`+`
		`+ reactions.setdefault(r, []).append(flask.g.fas_user.username)`
		`+ comment.reactions = reactions`
		`+ flask.g.session.add(comment)`
		`+`
		`+ try:`
		`+ flask.g.session.commit()`
		`+ except SQLAlchemyError as err: # pragma: no cover`
		`+ flask.g.session.rollback()`
		`+ _log.error(err)`
		`+ return 'error'`
		`+`
		`+ return 'ok'`

pagure/ui/issues.py

file modified

+76

		`@@ -365,6 +365,82 @@`
		`)`


		`+ _REACTION_URL_SNIPPET = 'issue/<int:issueid>/comment/<int:commentid>/react'`
		`+`
		`+`
		`+ @UI_NS.route(`
		`+ '/<repo>/%s/' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/<repo>/%s' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/<namespace>/<repo>/%s/' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/<namespace>/<repo>/%s' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<repo>/%s/' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<repo>/%s' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<namespace>/<repo>/%s/' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<namespace>/<repo>/%s' % _REACTION_URL_SNIPPET,`
		`+ methods=['POST'])`
		`+ @login_required`
		`+ @has_issue_tracker`
		`+ def issue_comment_add_reaction(repo, issueid, commentid, username=None,`
		`+ namespace=None):`
		`+ '''Add a reaction to a comment of an issue'''`
		`+ repo = flask.g.repo`
		`+`
		`+ issue = pagure.lib.search_issues(flask.g.session, repo, issueid=issueid)`
		`+`
		`+ if not issue or issue.project != repo:`
		`+ flask.abort(404, 'Comment not found')`
		`+`
		`+ form = pagure.forms.ConfirmationForm()`
		`+ if not form.validate_on_submit():`
		`+ flask.abort(400, 'CSRF token not valid')`
		`+`
		`+ if issue.private and not flask.g.repo_committer \`
		`+ and (not authenticated() or`
		`+ not issue.user.user == flask.g.fas_user.username):`
		`+ flask.abort(`
		`+ 404, 'No such issue')`
		`+`
		`+ comment = pagure.lib.get_issue_comment(`
		`+ flask.g.session, issue.uid, commentid)`
		`+`
		`+ if 'reaction' not in flask.request.form:`
		`+ flask.abort(400, 'Reaction not found')`
		`+`
		`+ reactions = comment.reactions`
		`+ r = flask.request.form['reaction']`
		`+ if not r:`
		`+ flask.abort(400, 'Empty reaction is not acceptable')`
		`+ if flask.g.fas_user.username in reactions.get(r, []):`
		`+ flask.abort(409, 'Already posted this one')`
		`+`
		`+ reactions.setdefault(r, []).append(flask.g.fas_user.username)`
		`+ comment.reactions = reactions`
		`+ flask.g.session.add(comment)`
		`+`
		`+ try:`
		`+ flask.g.session.commit()`
		`+ except SQLAlchemyError as err: # pragma: no cover`
		`+ flask.g.session.rollback()`
		`+ _log.error(err)`
		`+ return 'error'`
		`+`
		`+ return 'ok'`
		`+`
		`+`
		`@UI_NS.route('/<repo>/tag/<tag>/edit/', methods=('GET', 'POST'))`
		`@UI_NS.route('/<repo>/tag/<tag>/edit', methods=('GET', 'POST'))`
		`@UI_NS.route('/<namespace>/<repo>/tag/<tag>/edit/', methods=('GET', 'POST'))`

tests/test_pagure_flask_api_issue.py

file modified

		`@@ -2231,6 +2231,7 @@`
		`"notification": False,`
		`"id": 1,`
		`"parent": None,`
		`+ "reactions": {},`
		`"user": {`
		`"fullname": "PY C",`
		`"name": "pingou"`
		`@@ -2257,6 +2258,7 @@`
		`"notification": False,`
		`"id": 1,`
		`"parent": None,`
		`+ "reactions": {},`
		`"user": {`
		`"fullname": "PY C",`
		`"name": "pingou"`
		`@@ -2351,6 +2353,7 @@`
		`"notification": False,`
		`"id": 1,`
		`"parent": None,`
		`+ "reactions": {},`
		`"user": {`
		`"fullname": "foo bar",`
		`"name": "foo"`

tests/test_pagure_flask_api_ui_private_repo.py

file modified

		`@@ -3093,6 +3093,7 @@`
		`"editor": None,`
		`"id": 1,`
		`"parent": None,`
		`+ "reactions": {},`
		`"user": {`
		`"fullname": "PY C",`
		`"name": "pingou"`
		`@@ -3119,6 +3120,7 @@`
		`"editor": None,`
		`"id": 1,`
		`"parent": None,`
		`+ "reactions": {},`
		`"user": {`
		`"fullname": "PY C",`
		`"name": "pingou"`

tests/test_pagure_flask_ui_fork.py

file modified

+87

		`@@ -3172,5 +3172,92 @@`

		`shutil.rmtree(newpath)`

		`+ def _set_up_for_reaction_test(self):`
		`+ self.session.add(pagure.lib.model.User(`
		`+ user='jdoe',`
		`+ fullname='John Doe',`
		`+ password=b'password',`
		`+ default_email='jdoe@example.com',`
		`+ ))`
		`+ self.session.commit()`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(`
		`+ os.path.join(self.path, 'requests'), bare=True)`
		`+ self.set_up_git_repo(new_project=None, branch_from='feature')`
		`+ pagure.lib.get_authorized_project(self.session, 'test')`
		`+ request = pagure.lib.search_pull_requests(`
		`+ self.session, requestid=1, project_id=1,`
		`+ )`
		`+ pagure.lib.add_pull_request_comment(`
		`+ self.session,`
		`+ request=request,`
		`+ commit=None,`
		`+ tree_id=None,`
		`+ filename=None,`
		`+ row=None,`
		`+ comment='Hello',`
		`+ user='jdoe',`
		`+ requestfolder=None,`
		`+ )`
		`+ self.session.commit()`
		`+`
		`+ @patch('pagure.lib.notify.send_email')`
		`+ def test_add_reaction(self, send_email):`
		`+ """ Test the request_pull endpoint. """`
		`+ send_email.return_value = True`
		`+`
		`+ self._set_up_for_reaction_test()`
		`+`
		`+ user = tests.FakeUser()`
		`+ user.username = 'pingou'`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.get('/test/pull-request/1')`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ data = {`
		`+ 'csrf_token': self.get_csrf(output=output),`
		`+ 'reaction': 'Thumbs up',`
		`+ }`
		`+`
		`+ output = self.app.post(`
		`+ '/test/pull-request/1/comment/1/react',`
		`+ data=data,`
		`+ follow_redirects=True,`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ # Load the page and check reaction is added.`
		`+ output = self.app.get('/test/pull-request/1')`
		`+ self.assertEqual(output.status_code, 200)`
		`+ self.assertIn(`
		`+ 'Thumbs up sent by pingou',`
		`+ output.get_data(as_text=True)`
		`+ )`
		`+`
		`+ @patch('pagure.lib.notify.send_email')`
		`+ def test_add_reaction_unauthenticated(self, send_email):`
		`+ """ Test the request_pull endpoint. """`
		`+ send_email.return_value = True`
		`+`
		`+ self._set_up_for_reaction_test()`
		`+`
		`+ output = self.app.get('/test/pull-request/1')`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ data = {`
		`+ 'csrf_token': self.get_csrf(output=output),`
		`+ 'reaction': 'Thumbs down',`
		`+ }`
		`+`
		`+ output = self.app.post(`
		`+ '/test/pull-request/1/comment/1/react',`
		`+ data=data,`
		`+ follow_redirects=False,`
		`+ )`
		`+ # Redirect to login page`
		`+ self.assertEqual(output.status_code, 302)`
		`+ self.assertIn('/login/', output.headers['Location'])`
		`+`
		`+`
		`if __name__ == '__main__':`
		`unittest.main(verbosity=2)`

tests/test_pagure_flask_ui_issues.py

file modified

+120

		`@@ -3807,6 +3807,126 @@`
		`'title="Reply to this comment - lose formatting">',`
		`), 1)`

		`+ def _set_up_for_reaction_test(self, private=False):`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True)`
		`+`
		`+ self.session.add(pagure.lib.model.User(`
		`+ user='naysayer',`
		`+ fullname='John Doe',`
		`+ password=b'password',`
		`+ default_email='jdoe@example.com',`
		`+ ))`
		`+ self.session.commit()`
		`+ repo = pagure.lib.get_authorized_project(self.session, 'test')`
		`+ msg = pagure.lib.new_issue(`
		`+ session=self.session,`
		`+ repo=repo,`
		`+ title='Test issue',`
		`+ content='Fix me',`
		`+ user='pingou',`
		`+ ticketfolder=None,`
		`+ private=private,`
		`+ )`
		`+ pagure.lib.add_issue_comment(`
		`+ session=self.session,`
		`+ issue=msg,`
		`+ comment='How about no',`
		`+ user='naysayer',`
		`+ ticketfolder=None,`
		`+ )`
		`+ self.session.commit()`
		`+`
		`+ @patch('pagure.lib.git.update_git')`
		`+ @patch('pagure.lib.notify.send_email')`
		`+ def test_add_reaction(self, p_send_email, p_ugt):`
		`+ ''' Test adding a reaction to an issue comment.'''`
		`+ p_send_email.return_value = True`
		`+ p_ugt.return_value = True`
		`+`
		`+ self._set_up_for_reaction_test()`
		`+`
		`+ user = tests.FakeUser()`
		`+ user.username = 'pingou'`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.get('/test/issue/1')`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ data = {`
		`+ 'csrf_token': self.get_csrf(output=output),`
		`+ 'reaction': 'Thumbs down',`
		`+ }`
		`+`
		`+ output = self.app.post(`
		`+ '/test/issue/1/comment/1/react',`
		`+ data=data,`
		`+ follow_redirects=True,`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ # Load the page and check reaction is added.`
		`+ output = self.app.get('/test/issue/1')`
		`+ self.assertEqual(output.status_code, 200)`
		`+ self.assertIn(`
		`+ 'Thumbs down sent by pingou',`
		`+ output.get_data(as_text=True)`
		`+ )`
		`+`
		`+ @patch('pagure.lib.git.update_git')`
		`+ @patch('pagure.lib.notify.send_email')`
		`+ def test_add_reaction_unauthenticated(self, p_send_email, p_ugt):`
		`+ '''`
		`+ Test adding a reaction to an issue comment without authentication.`
		`+ '''`
		`+ p_send_email.return_value = True`
		`+ p_ugt.return_value = True`
		`+`
		`+ self._set_up_for_reaction_test()`
		`+`
		`+ output = self.app.get('/test/issue/1')`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ data = {`
		`+ 'csrf_token': self.get_csrf(output=output),`
		`+ 'reaction': 'Thumbs down',`
		`+ }`
		`+`
		`+ output = self.app.post(`
		`+ '/test/issue/1/comment/1/react',`
		`+ data=data,`
		`+ follow_redirects=False,`
		`+ )`
		`+ # Redirect to login page`
		`+ self.assertEqual(output.status_code, 302)`
		`+ self.assertIn('/login/', output.headers['Location'])`
		`+`
		`+ @patch('pagure.lib.git.update_git')`
		`+ @patch('pagure.lib.notify.send_email')`
		`+ def test_add_reaction_private_issue(self, p_send_email, p_ugt):`
		`+ '''Test adding a reaction to a private issue comment.'''`
		`+ p_send_email.return_value = True`
		`+ p_ugt.return_value = True`
		`+`
		`+ self._set_up_for_reaction_test(private=True)`
		`+`
		`+ user = tests.FakeUser()`
		`+ user.username = 'naysayer'`
		`+ with tests.user_set(self.app.application, user):`
		`+ # Steal CSRF token from new issue page`
		`+ output = self.app.get('/test/new_issue')`
		`+`
		`+ data = {`
		`+ 'csrf_token': self.get_csrf(output=output),`
		`+ 'reaction': 'Thumbs down',`
		`+ }`
		`+`
		`+ output = self.app.post(`
		`+ '/test/issue/1/comment/1/react',`
		`+ data=data,`
		`+ follow_redirects=True,`
		`+ )`
		`+ self.assertEqual(output.status_code, 404)`
		`+`

		`if __name__ == '__main__':`
		`unittest.main(verbosity=2)`

tests/test_pagure_lib_git.py

file modified

+2 -1

		`@@ -1687,7 +1687,7 @@`
		`index 458821a..77674a8`
		`--- a/123`
		`+++ b/456`
		`- @@ -3,13 +3,31 @@`
		`+ @@ -3,13 +3,32 @@`
		`"blocks": [],`
		`"close_status": null,`
		`"closed_at": null,`
		`@@ -1701,6 +1701,7 @@`
		`+ "id": 1,`
		`+ "notification": false,`
		`+ "parent": null,`
		`+ + "reactions": {},`
		`+ "user": {`
		`+ "default_email": "foo@bar.com",`
		`+ "emails": [`

lsedlar commented 5 years ago

A user can select from a predefined list of reactions for a comment. There can be multiple reactions from the same person on a single comment, but only one of each type.

Fixes: https://pagure.io/pagure/issue/812

pingou commented on line 31 of pagure/ui/issues.py 5 years ago

Let's use the full name: reaction

pingou commented on line 32 of pagure/ui/issues.py 5 years ago

We'll need to adjust this ;-)

pingou commented 5 years ago

Is this way of storing the reactions reasonable?

Looks fine to me :)

If it used user ids instead, it could display up-to-date name. But if a user is deleted, it would be more tricky. (Can a user be deleted? What happens to their comments then?)

Let's rely on username, then there is no need to update anything. Also, we can "fix" this by not listing all the usernames, imagine if 100 people react on 1 comment... :)

Is a predefined list of reactions good enough? Or should it support any emoji as suggested by @jflory7?

How hard would it be to change? Emoji support is tempting but I'm seeing our competitor is offering only 1 reaction/user in a restricted set.

Should only one reaction per comment per user be allowed?

See above, open to more suggestions :)

lsedlar commented 5 years ago

Thanks for the feedback!

How hard would it be to change? Emoji support is tempting but I'm seeing our competitor is offering only 1 reaction/user in a restricted set.

It should be doable. I just need to figure out how to put the autocomplete field into the dropdown in a reasonable way. I'll try to get something working over the weekend.

I'm not sure if multiple reactions from the same user are a good thing. If it's used for voting, some could fake multiple votes by using different emoji (:thumbsup: :100: :heart: :hearts: :heartbeat: :heartpulse: :two_hearts:).

rebased onto 6a6c36c34af3c389f4050c8ff856e3d78a972a7b

5 years ago

lsedlar commented 5 years ago

Updated to support any emoji as reaction. There is still a predefined list for easy access (I wouldn't want to type the name all the time). One user can send multiple reactions to one comment. Only 10 people for each reaction should be displayed.

Further issues to resolve:

make it work for pull request comments
the emoji selector dropdown is displayed below the search entry
write some tests
try and make it look more polished

Not working and I think it's fine:

reactions are not updated live via SSE; users must refresh page
reaction added via javascript do not get username of current user in the tooltip

rebased onto 7924676a7b6d1f8f3296b359d008991ecf0820f5

5 years ago

Metadata Update from @pingou:
- Request assigned

5 years ago

Metadata Update from @pingou:
- Request reset

5 years ago

5 new commits added

Move common js into a separate file
Add reactions to pull request comments
Move emoji dropdown above search entry
Less space around reaction buttons
Prettier button

5 years ago

lsedlar commented 5 years ago

Now it works for pull request comments as well.

Now only tests are missing.

ryanlerch commented 5 years ago

is the idea here to have a set of reactions? or just let a user do any emoji?

If any emoji, what is the difference between a reaction, and just adding an emoji as a comment. /me is kinda leaning towards having a subset of reactions rather than any emoji (which also could be bad for community -- :poop: )

lsedlar commented 5 years ago

Originally I started with a set of allowed reactions, then extended it into allowing any emoji. Even if that is allowed, I would want to keep a select few easily accessible.

I'm starting to think that any emoji is not a good idea because it causes a few issues: the way it works now emojione only gives us the code for the symbol, not any name. Therefore it's rather inaccessible, as the best textual representation we can do is something like 1F4A9. With a selected list we could know that is actually a pile of poop. This is problematic for vision impaired users and also for exposure in the API.

If any emoji, what is the difference between a reaction, and just adding an emoji as a comment.

Right now the difference is that e-mails are not sent for reactions (probably good), it will not show up in the heatmap on user page (probably should be fixed), and it will display differently. Otherwise no difference.

rebased onto 4fe90bcb7b0748dfda239193bc84020a8c0bfdf5

5 years ago

rebased onto 0124c8b70b01d67f251a5afbbffa6600c6fcc9d8

5 years ago

lsedlar commented 5 years ago

Changes since last version: