pagure

Clone

#450 Addind edit button and functionality to comments on a pull-request
Closed 8 years ago by pingou. Opened 8 years ago by farhaan.
farhaan/pagure edit-button  into  master

file added
+39 
@@ -0,0 +1,39 @@ 
 

+ """Adding column to store edited_by and edited_on a commnet
 

+ 

+ Revision ID: 15ea3c2cf83d
 

+ Revises: 21f45b08d882
 

+ Create Date: 2015-11-09 16:18:47.192088
 

+ 

+ """
 

+ 

+ # revision identifiers, used by Alembic.
 

+ revision = '15ea3c2cf83d'
 

+ down_revision = '21f45b08d882'
 

+ 

+ from alembic import op
 

+ import sqlalchemy as sa
 

+ 

+ 

+ def upgrade():
 

+ 

+     op.add_column(
 

+         'pull_request_comments',
 

+         sa.Column(
 

+             'editor_id',
 

+             sa.Integer,
 

+             sa.ForeignKey('users.id', onupdate='CASCADE'),
 

+             nullable=True)
 

+     )
 

+ 

+     op.add_column(
 

+         'pull_request_comments',
 

+         sa.Column(
 

+             'edited_on',
 

+             sa.DATETIME,
 

+             nullable=True)
 

+     )
 

+ 

+ 

+ def downgrade():
 

+     op.drop_column('pull_request_comments', 'editor_id')
 

+     op.drop_column('pull_request_comments', 'edited_on')

file modified
+8 
@@ -364,3 +364,11 @@ 
 

              self.branches.choices = [
 

                  (branch, branch) for branch in kwargs['branches']
 

              ]
 

+ 

+ class EditCommentForm(wtf.Form):
 

+     """ Form to verify that comment is not empty
 

+     """
 

+     update_comment = wtforms.TextAreaField(
 

+         'Comment<span class="error">*</span>',
 

+         [wtforms.validators.Required()]
 

+     )

file modified
+37 
@@ -802,6 +802,43 @@ 
 

      return 'Comment added'
 

  

  

+ def edit_pull_request_comment(session, request, comment, user,
 

+                               updated_comment, requestfolder, redis):
 

+     '''Edit a comment in the pull request'''
 

+     user_obj = __get_user(session, user)
 

+     comment.comment = updated_comment
 

+     comment.edited_on = datetime.datetime.utcnow()
 

+     comment.editor = user_obj
 

+ 

+     session.add(comment)
 

+     # Make sure we won't have SQLAlchemy error before we continue
 

+     session.flush()
 

+ 

+     pagure.lib.git.update_git(
 

+         request, repo=request.project, repofolder=requestfolder)
 

+ 

+     pagure.lib.notify.log(
 

+         request.project,
 

+         topic='pull-request.comment.edited',
 

+         msg=dict(
 

+             pullrequest=request.to_json(public=True),
 

+             agent=user_obj.username,
 

+         )
 

+     )
 

+ 

+     if redis:
 

+         redis.publish(request.uid, json.dumps({
 

+             'request_id': len(request.comments),
 

+             'comment_updated': text2markdown(comment.comment),
 

+             'comment_id': comment.id,
 

+             'comment_editor': user_obj.user,
 

+             'avatar_url': avatar_url(comment.user.user, size=16),
 

+             'comment_date': comment.date_created.strftime('%Y-%m-%d %H:%M'),
 

+         }))
 

+ 

+     return "Comment updated"
 

+ 

+ 

  def add_pull_request_flag(session, request, username, percent, comment, url,
 

                            uid, user, requestfolder):
 

      ''' Add a flag to a pull-request. '''

file modified
+9 
@@ -928,6 +928,15 @@ 
 

  

      date_created = sa.Column(sa.DateTime, nullable=False,
 

                               default=datetime.datetime.utcnow)
 

+     editor_id = sa.Column(
 

+         sa.Integer,
 

+         sa.ForeignKey('users.id', onupdate='CASCADE'),
 

+         nullable=True)
 

+ 

+     editor = relation('User', foreign_keys=[editor_id],
 

+                          remote_side=[User.id])
 

+ 

+     edited_on = sa.Column(sa.DATETIME, nullable=True)
 

  

      user = relation('User', foreign_keys=[user_id],
 

                      remote_side=[User.id],

file modified
+13 
@@ -74,12 +74,25 @@ 
 

        <a class="headerlink" title="Permalink to this headline"
 

          href="#comment-{{ id }}">¶</a>
 

        <aside class="issue_action icon">
 

+ 

+         {% if comment.edited_on %}
 

+             <span title="{{
 

+             comment.updated_info
 

+             }}">Edited by {{ comment.editor.username }} {{ comment.edited_on | humanize }}</span>
 

+         {% endif %}
 

+ 

          <a class="reply" title="Reply to this comment - loose formating">
 

            reply
 

          </a>
 

+ 

          {% if id != 0 and g.fas_user and (
 

              (comment.parent.status in [True, 'Open'] and g.fas_user.username == comment.user.username)
 

              or repo_admin) %}
 

+         <a href="{{ url_for(
 

+             'pull_request_edit_comment', username=username, repo=repo.name,
 

+             requestid=issueid, commentid=comment.id)}}" >
 

+             <span class="icon icon-edit blue"></span>
 

+         </a>
 

          <button type="submit" name="drop_comment" value="{{ comment.id }}"
 

              onclick="return confirm('Do you really want to remove this comment?');"
 

              title="Remove comment">

file added
+25 
@@ -0,0 +1,25 @@ 
 

+ {% extends "repo_master.html" %}
 

+ 

+ {% block repo %}
 

+ <section class="request_comment add_comment">
 

+     <form action="{{ url_for(
 

+         'pull_request_edit_comment', username=username, repo=repo.name,
 

+         requestid=requestid, commentid=comment.id) }}" method="post">
 

+ 

+         <div class="tabs  ui-widget ui-widget-content ui-corner-all" id="comment_block">
 

+ 

+             <div id="edit">
 

+                 <textarea id="update_comment" name="update_comment">{{ comment.comment }}</textarea>
 

+             </div>
 

+ 

+         <div>
 

+       

+   {{ form.csrf_token }}
 

+     <div>
 

+         <input type="submit" class="submit positive button" value="Update">
 

+         <input type="button" class="cancel" value="Cancel" onclick="location.href='{{ url_for(
 

+             'request_pull', username=username, repo=repo.name, requestid=requestid) }}'"/>
 

+     </div>
 

+   </form>
 

+ </section>
 

+ {% endblock %}

file modified
+73 -2 
@@ -10,6 +10,7 @@ 
 

  

  import flask
 

  import os
 

+ import datetime
 

  

  import pygit2
 

  from sqlalchemy.exc import SQLAlchemyError
 
@@ -368,11 +369,10 @@ 
 

          flask.abort(400, 'Pull-request is already closed')
 

  

      if not is_repo_admin(repo) \
 

-             or flask.g.fas_user.username != request.user.username :
 

+             or flask.g.fas_user.username != request.user.username:
 

          flask.abort(403, 'You are not allowed to edit this pull-request')
 

  

      form = pagure.forms.RequestPullForm()
 

- 

      if form.validate_on_submit():
 

          request.title = form.title.data
 

          SESSION.add(request)
 
@@ -540,6 +540,77 @@ 
 

          repo=repo.name, requestid=requestid))
 

  

  

+ @APP.route('/<repo>/pull-request/<int:requestid>/comment/<int:commentid>/edit',
 

+            methods=('GET', 'POST'))
 

+ @APP.route('/fork/<username>/<repo>/pull-request/<int:requestid>/comment'
 

+            '/<int:commentid>/edit', methods=('GET', 'POST'))
 

+ @cla_required
 

+ def pull_request_edit_comment(repo, requestid, commentid, username=None):
 

+     """Edit comment of a pull request
 

+     """
 

+     project = pagure.lib.get_project(SESSION, repo, user=username)
 

+ 

+     if not project:
 

+         flask.abort(404, 'Project not found')
 

+ 

+     if not project.settings.get('pull_requests', True):
 

+         flask.abort(404, 'No pull-requests found for this project')
 

+ 

+     request = pagure.lib.search_pull_requests(
 

+         SESSION, project_id=project.id, requestid=requestid)
 

+ 

+     if not request:
 

+         flask.abort(404, 'Pull-request not found')
 

+ 

+     comment = pagure.lib.get_request_comment(
 

+         SESSION, request.uid, commentid)
 

+     if comment is None or comment.pull_request.project != project:
 

+         flask.abort(404, 'Comment not found')
 

+ 

+     if (flask.g.fas_user.username != comment.user.username
 

+             or comment.parent.status is False) \
 

+             and not is_repo_admin(project):
 

+         flask.abort(403,
 

+                     "You are not allowed to edit the comment")
 

+ 

+     form = pagure.forms.EditCommentForm()
 

+ 

+     if form.validate_on_submit():
 

+ 

+         updated_comment = form.update_comment.data
 

+         try:
 

+             message = pagure.lib.edit_pull_request_comment(
 

+                 SESSION,
 

+                 request=request,
 

+                 comment=comment,
 

+                 user=flask.g.fas_user.username,
 

+                 updated_comment=updated_comment,
 

+                 requestfolder=APP.config['REQUESTS_FOLDER'],
 

+                 redis=REDIS,
 

+             )
 

+             SESSION.commit()
 

+             flask.flash(message)
 

+         except SQLAlchemyError, err:  # pragma: no cover
 

+             SESSION.rollback()
 

+             LOG.error(err)
 

+             flask.flash(
 

+                 'Could not edit the comment: %s' % commentid, 'error')
 

+ 

+         return flask.redirect(flask.url_for(
 

+             'request_pull', username=username,
 

+             repo=project.name, requestid=requestid))
 

+ 

+     return flask.render_template(
 

+         'pull_request_comment_update.html',
 

+         select='requests',
 

+         requestid=requestid,
 

+         repo=project,
 

+         username=username,
 

+         form=form,
 

+         comment=comment
 

+     )
 

+ 

+ 

  @APP.route('/<repo>/pull-request/<int:requestid>/merge', methods=['POST'])
 

  @APP.route('/fork/<username>/<repo>/pull-request/<int:requestid>/merge',
 

             methods=['POST'])

file modified
+87 
@@ -1529,6 +1529,93 @@ 
 

                  follow_redirects=True)
 

              self.assertEqual(output.status_code, 404)
 

  

+     @patch('pagure.lib.notify.send_email')
 

+     def test_pull_request_edit_comment(self, send_email):
 

+         """ Test the pull request edit comment endpoint """
 

+         send_email.return_value = True
 

+ 

+         self.test_request_pull()
 

+ 

+         user = tests.FakeUser()
 

+         user.username = 'pingou'
 

+         with tests.user_set(pagure.APP, user):
 

+             # Repo 'foo' does not exist so it is verifying that condition
 

+             output = self.app.post('/foo/pull-request/1/comment/1/edit')
 

+             self.assertEqual(output.status_code, 404)
 

+ 

+             # Here no comment is present in the PR so its verifying that condition
 

+             output = self.app.post('/test/pull-request/100/comment/100/edit')
 

+             self.assertEqual(output.status_code, 404)
 

+ 

+             output = self.app.post('/test/pull-request/1/comment')
 

+             self.assertEqual(output.status_code, 200)
 

+             # Creating comment to play with
 

+             self.assertTrue(
 

+                 output.data.startswith('<section class="add_comment">'))
 

+ 

+             csrf_token = output.data.split(
 

+                 'name="csrf_token" type="hidden" value="')[1].split('">')[0]
 

+ 

+             data = {
 

+                 'csrf_token': csrf_token,
 

+                 'comment': 'This look alright but we can do better',
 

+             }
 

+             output = self.app.post(
 

+                 '/test/pull-request/1/comment', data=data,
 

+                 follow_redirects=True)
 

+             self.assertEqual(output.status_code, 200)
 

+ 

+             self.assertIn(
 

+                 '<title>PR#1: PR from the feature branch - test - '
 

+                 'Pagure</title>', output.data)
 

+             self.assertIn(
 

+                 '<li class="message">Comment added</li>', output.data) 

+             # Check if the comment is there
 

+             self.assertIn(
 

+                 '<p>This look alright but we can do better</p>', output.data)
 

+             output = self.app.get('/test/pull-request/1/comment/1/edit')
 

+             self.assertEqual(output.status_code, 200)
 

+ 

+             self.assertIn('<section class="request_comment add_comment">', output.data)
 

+             # Checking if the comment is there in the update page
 

+             self.assertIn(
 

+                 'This look alright but we can do better</textarea>', output.data)
 

+ 

+             csrf_token = output.data.split(
 

+                 'name="csrf_token" type="hidden" value="')[1].split('">')[0]
 

+ 

+             data = {
 

+                 'csrf_token': csrf_token,
 

+                 'update_comment': 'This look alright but we can do better than this.',
 

+             }
 

+             output = self.app.post(
 

+                 '/test/pull-request/1/comment/1/edit', data=data,
 

+                 follow_redirects=True)
 

+             # Checking if the comment is updated in the main page
 

+             self.assertIn(
 

+                 '<p>This look alright but we can do better than this.</p>', output.data)
 

+             self.assertIn(
 

+                 '<title>PR#1: PR from the feature branch - test - '
 

+                 'Pagure</title>', output.data)
 

+             # Checking if Edited by User is there or not
 

+             self.assertIn(
 

+                 '<span title="">Edited by pingou just now</span>', output.data)
 

+             self.assertIn(
 

+                 '<li class="message">Comment updated</li>', output.data)
 

+ 

+ 

+             #  Project w/o pull-request
 

+             repo = pagure.lib.get_project(self.session, 'test')
 

+             settings = repo.settings
 

+             settings['pull_requests'] = False
 

+             repo.settings = settings
 

+             self.session.add(repo)
 

+             self.session.commit()
 

+ 

+             output = self.app.post(
 

+                 '/test/pull-request/1/comment/edit/1', data=data,
 

+                 follow_redirects=True)
 

+             self.assertEqual(output.status_code, 404)
 

  

  if __name__ == '__main__':
 

      SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureFlaskForktests)

no initial comment

What happens if comment is None?

This is odd, on the first link we're sending the comment.id as part of the form, on the second page as part of the URL, I think we can do better

Why these two lines if there was a check before?

The name of the file is misleading, as this seems to be for editing the comment?

This is either create or edit of a comment, not drop.

You need to put this before the submit button.

It looks like this form is for editing comments.
You should edit the title in that case.

We don't want any GET requests to drop.

This is NOT the right function to be adding editing functionality.
You should add a new entry point for editing if you want that, and not overload the drop endpoint.

Comment in-line.
Basically:
1. You should not reuse existing endpoints (drop) for new functionality: add new endpoints.
2. You should probably implement in-line editing. The separate comment for is a very bad user experience.

Final remark: I do NOT like the possibility of editing comments.
Especially not given the fact that we send emails for every comment, and the fact that you can reply from just the email.
Reasoning: one of the following two things happen when editing a comment:

  1. We send another email with the edited comment. In this case, allowing edits will make people check their comments less before posting, encourage editing, and as such spam people a lot more, and making the person replying unable to see what the last version is.

  2. We do not send another email with the edited comment. In this case, someone might respond by email to a comment that has already been edited, which may lead to some bad results.

For these exact reasons I do not like deleting of comments either, but well...

We need to catch the exception here in case writing to the DB doesn't work and we'll need to log that exception to warn the admins something is wrong with the DB.

Something we could do here, is check that the request is a POST and not a GET.

You should configure your IDE to remove spaces from empty lines
  1. You should not reuse existing endpoints (drop) for new functionality: add new endpoints.

In theory I agree but in this case we cannot as the buttons are both in the same (html) form and we can't embed a form in another one.

What I'll like to ask @farhaan though is to split the two use-case of this endpoint into two methods to make it clearer.

  1. You should probably implement in-line editing. The separate comment for is a very bad user experience.

I agree on this as well, but one step at a time :)

Isn't the validated version of this available in form.update_comment or something like that?

(which means that if you have your validators set correctly, you can also get rid of this check).

Remove the space after repo=

Why do you do this and not a new form so that you can have all validation done in the form?

After all comments are handled, please squash commits that don't show up anymore.

Indentation is wrong here :)

Also note that there should be no spaces around operators.

The id attribute on this should not be the comment ID, but an ID that javascript could use to refer to it.
I would say to use update_comment here as well.

I think you should remove this print statement :)

This should be before the if validate.

This too should be checked before form valdiate, so that people don't see the form for comments tehy aren't allowed to touch.

This version of onclick is probably not the best.
Especially if we want to integrate this into in-line editing, this will break horribly.

Note that for this version of the PR, it will work.
But just noting it for when you add in-line editing.

This line breaking is not per PEP8.

Actually, it is, but my browser had a horrible time displaying it.
Just ignore the previous comment.

We might want to record the last edit of the comment in the database?
That way, we could display a "This comment was edited at X by Y" meta-comment, allowing people to see why a comment and its replies might not be the same.

Note that while I don't think you need to add the text itself to the template (although that should be really easy), we should add the database field and make the edit in this PR, since making database changes is a complicated process.

Note that when the "This comment was edited" is implemented, that would resolve a lot of my complaints against enabling editing.

Especially if it is also possible to enable/disable editing on a per-project level, I would be fine with adding the feature.

The title seems odd, maybe we could be either shorter: Comment or more explicit Comment to update

Note: here we know the comment, so we know the PR/issue on which it was made so we can figure out where to go back if you want to cancel your edit

sa.Text is great for postgresql, but breaks for MySQL, could we use another column type?

(I recently had to adjust pagure to work on MySQL, so I'd like to keep this compatibility a little :))

Trailing space
  • on between the two variables?

Maybe comment why it's returning a 404 here ?

Doing a POST request here?

Do we have the old content on this page?

And the comment, is it in the page?

Do we have the new content on the page or the old one?

Should we keep the indentation consistent (2 spaces) ?

You need to use the {{ url_for() }} construct here, otherwise if you deploy pagure at http://pingoured.fr/pagure/ the url will break

Already there and do we want to send an email when a comment is edited?

What is form? What does it mean? What does it contain?

All you use it for is to get the comment, so drop the un-clear/un-defined object and give the comment directly

I think we should check here if the user editing the comment is allowed to do it, and if not, raise an exception.

Was the comment added?

You're passing both the comment and the commentid, isn't one enough?

What happens if comment is None?

I am still not a big fan of the title of this field

We already retrieved the comment in the controller, so we're doing two queries against the DB.

Either we should do it here or in the controller, but doing two queries to retrieve the same info doesn't sound like a good idea.

Missing a space maybe?

This is the wrong one!
Your should use something like comment_changed.
Also, you should add a field comment_id so that the UI knows which comment to update.

How about issue.comment.edited to stay in the theme?

The other code paths do fedmsg publishing first and then redis, so that fedmsg happens even if something goes wrong with redis.

Why is this a string?
Just use a remote reference to the users.user_id table, just like the user = relation just a few lines beneath.

PEP8

This is a project, not a repo.
repo != project, since one project has several repos.

How about testing the the updated_by string was set correctly?

If the field is editor_id maybe the relation should be editor?

Missing a space between 'request_pull' and 'username'

There should be a space between your # and your comment :)

This should be 'pull-request.comment.edited' to stay in sync with the rest of the code.

Since you have editor = Relation, you should be able to say comment.editor = user_obj.

space missing.

In this instance, you did not change the name of the variable.

I don't agree that admins should be able to change other users' comments. But that's up for @pingou.

You can get rid of this line

Please add line breaks.

This should probably be user_obj.user, and renamed to comment_updated_by, or something, so that hte UI can actually show who updated the comment rather than who posted it originally.

This looks like a strange name for a template.
How about pull_request_comment_update.html or something?

If you could squash the commits and tests pass, I think we can merge to the editing branch.

Ok merging this PR manually into the editing branch
Metadata
None
No Tags
Changes Summary 8
+39
file added
alembic/versions/15ea3c2cf83d_adding_column_to_store_edited_by_and_.py
+8 -0
file changed
pagure/forms.py
+37 -0
file changed
pagure/lib/__init__.py
+9 -0
file changed
pagure/lib/model.py
+13 -0
file changed
pagure/templates/_formhelper.html
+25
file added
pagure/templates/pull_request_comment_update.html
+73 -2
file changed
pagure/ui/fork.py
+87 -0
file changed
tests/test_progit_flask_ui_fork.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.