#1556 Add quick replies to pull requests/issues
Merged by pingou. Opened by lsedlar.
lsedlar/pagure quick-reply  into  master

Download 1556.patch

This pull request adds a possibility to save frequently used responses for pull requests or issues and then use them by a click of a button.

The entire think is client side: the data is stored in browser local storage, all elements on page are added with JavaScript.

I have been using this as a user script for some time now and seems useful so I want to share it. I'm not sure if including it in Pagure itself is a good move. I'm open to suggestions.

Here is a screenshot: http://img.susepaste.org/cf6d9066
(Are attachments only possible for issues? I don't see the upload button on a pull request.)

Thanks for working on this, it's a really cool feature.

One thing that I wonder is if we should store this on the user's browser, vs in the project's settings. The later giving the advantages of allowing the quick replies to be shared among the developers of the project.

Depending on your time, we could merge this as is and add project-wide support in a later PR, that would be fine for me.

Are attachments only possible for issues?

That is correct.

Good point about sharing the replies. In current state they are only visible for the user who set them and they see them in all projects. Doing it per-project makes more sense, since for e.g. fedora-infra you probably want different options than in pagure.

I don't think it needs to be merged as is, Let's get it right first. I'll see what I can do about it.

For anyone who would like to test, I put it on fpaste with the GreaseMonkey header: https://paste.fedoraproject.org/481505/79203861/

I'd take the same approach as we did for milestones, priorities and all, a json blump in the DB :)

will it something the user will be able to set for himself per project or just some options he will get to use depending on the project?

This looks really interesting plus I am a huge supporter of your work @lsedlar ! I am going to try it out!

I think we need to have some kind of filter, I mean the suggestion will populate on each reply that is not what we want right ? we want some specific generic replies!

Also I like it on a user basis while if we can find a way a user can store this in local storage on basis of the project and retrieve , i am not convinced that make all replies public is a good idea!

rebased

GitHub Saved replies work per user. However, I like @pingou's idea of having the list shared by a whole project. The use case I have for this is requesting a missing Signed-off-by, asking for a rebase of a PR or stating that pull request passed review. All these options are also useful for other contributors.

Current version in PR saves the replies in database, and offers them to anyone who can post a comment. Only project owners can edit them.

We so need to re-use that in other parts of our code to reduce the redundancy, for some reason I have never found this in the jinja doc :(

This is looking good, but I am about to cut a new release and I would like some tests in that code before deploying it.

Would you like to write those tests? Otherwise I can do it, but in both cases I believe this feature will not land in the next release but likely the following one.

IMHO there's no need to rush merging this. I'll write the tests (probably by the end of the week).

Now there are tests as well.

Seeing this comment I probably put the tests in incorrectly named file. Please let me know if you want them elsewhere.

I just call the method in pagure.lib :)

rebased

Now the tests are using functions from pagure.lib to create test data, which is a little nicer.

I'm not sure I wrote this file you're just adding :)

if you use __requires___ you need to keep the import pkg_resources just under it, otherwise afaiu it's not going to work.

This is working great for me!

Two remarks though, the alembic migration needs to be adjusted (twice, one for your revision one for another that is also broken), this is what I did:

diff --git a/ alembic/versions/349a3890596_increase_length_group_name.py b/ alembic/versions/349a3890596_increase_length_group_name.py
index 4e84bd5..e217f1f 100644
--- a/ alembic/versions/349a3890596_increase_length_group_name.py       
+++ b/ alembic/versions/349a3890596_increase_length_group_name.py       
@@ -1,14 +1,14 @@
 """Increase length group name
 Revision ID: 349a3890596
-Revises: 5083efccac7
+Revises: 114d3a68c1fd
 Create Date: 2016-11-30 14:30:15.681269
 """
 # revision identifiers, used by Alembic.
 revision = '349a3890596'
-down_revision = '5083efccac7'
+down_revision = '114d3a68c1fd'
 from alembic import op
 import sqlalchemy as sa
diff --git a/ alembic/versions/588eabcd394c_add_quick_replies_field_to_project.py b/ alembic/versions/588eabcd394c_add_quick_replies_field_to_project.py
index 08b3576..8b301d5 100644
--- a/ alembic/versions/588eabcd394c_add_quick_replies_field_to_project.py      
+++ b/ alembic/versions/588eabcd394c_add_quick_replies_field_to_project.py      
@@ -1,14 +1,14 @@
 """add quick_replies field to project
 Revision ID: 588eabcd394c
-Revises: 5083efccac7
+Revises: 349a3890596
 Create Date: 2016-11-17 16:12:36.624079
 """
 # revision identifiers, used by Alembic.
 revision = '588eabcd394c'
-down_revision = '5083efccac7'
+down_revision = '349a3890596'
 from alembic import op
 import sqlalchemy as sa

After clicking on one QR, if I click on another it doesn't update the field.
I guess the idea is to prevent loosing what is in the text area (if it was something the user entered), but I wonder if we could improve on this, maybe along the lines of:

diff --git a/ pagure/static/quick_reply.js b/ pagure/static/quick_reply.js
index 6546c96..65586ea 100644
--- a/ pagure/static/quick_reply.js     
+++ b/ pagure/static/quick_reply.js     
@@ -1,7 +1,7 @@
 $(document).ready(function() {
   $('.qr-reply').on('click', function (e) {
     let tgt = $('#comment');
-    if (!tgt.val()) {
+    if ((tgt.val() && confirm("The content of the comment will be entirely changed, are you sure?")) || !tgt.val()) {
       tgt.val($(this).attr('data-qr'));
     }
     $('.qr .dropdown-toggle').dropdown('toggle');

(Please let's not keep the wording I put there ^_^)

What do you think?

I think I can remove it completely, it doesn't change the results.

It would if we had two different versions of SQLAlchemy installed and one of them was 0.7.

These lines are basically left over from when we had projects running on rhel6 with two versions of SQLAlchemy. Since we're running on rhel7 and pagure can't run on 6, we could do a large clean-up and drop them all (outside the scope of this PR though)

I updated the migrations.

My initial thought was for the button to only be active when the textarea is visible and empty. I updated it to display a tooltip explaining why it's not active.

Super cool! :)

Thanks!

Pull-Request has been merged by pingou

Metadata