Skip to content
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

feat: editing townsquare comments #6023

Merged
merged 3 commits into from
Apr 22, 2020

Conversation

walidmujahid
Copy link
Contributor

@walidmujahid walidmujahid commented Feb 19, 2020

Editing a comment is as simple as clicking the edit button on a comment. All other comments of the user will have their edit buttons disabled when editing of a comment is active. Posting new comments for the user will also be disabled while they are editing a comment. If you try to submit an edit with zero characters, it will not go through. An edited comment will show (edited) string next to the timAgo section.

2020-02-30 - Working Demo - Video: https://youtu.be/wibE0emPwBE
2020-02-22 - Video: https://youtu.be/H9tvgp3i0y8

  • feat: editing townsquare comments
    • add edit button and handler function
    • handle an EDIT method in api
    • add an 'edited' tag to show the comment was edited
  • fix introduced bugs
    • fix issue with empty comment container being disabled after hitting 'Enter'
  • code review
  • refactor code
  • maintainer review

closes #6010

@codecov
Copy link

codecov bot commented Feb 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@f1d43c2). Click here to learn what that means.
The diff coverage is 7.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6023   +/-   ##
=========================================
  Coverage          ?   27.58%           
=========================================
  Files             ?      282           
  Lines             ?    26167           
  Branches          ?     3853           
=========================================
  Hits              ?     7218           
  Misses            ?    18684           
  Partials          ?      265
Impacted Files Coverage Δ
app/townsquare/views.py 9.88% <0%> (ø)
app/townsquare/models.py 57.79% <100%> (ø)

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 f1d43c2...54844c8. Read the comment docs.

@walidmujahid walidmujahid force-pushed the activity-comments-edit branch 7 times, most recently from 8d68376 to 5ab94f1 Compare February 21, 2020 16:26
@walidmujahid walidmujahid force-pushed the activity-comments-edit branch from e0bce79 to 7837609 Compare February 22, 2020 17:20
@walidmujahid walidmujahid marked this pull request as ready for review February 22, 2020 17:23
@walidmujahid walidmujahid changed the title [WIP] - feat: editing townsquare comments feat: editing townsquare comments Feb 24, 2020
@walidmujahid walidmujahid force-pushed the activity-comments-edit branch from 7837609 to d4c1fcb Compare February 24, 2020 12:57
@walidmujahid walidmujahid force-pushed the activity-comments-edit branch 2 times, most recently from 8e55640 to 525ea23 Compare February 24, 2020 13:23
@walidmujahid walidmujahid force-pushed the activity-comments-edit branch 2 times, most recently from 2ba052c to 20432f3 Compare February 26, 2020 14:56
@frankchen07
Copy link
Contributor

frankchen07 commented Feb 26, 2020

@walidmujahid - this looks pretty dope! thank you for the videos!

  1. I didn't see if the trash can (delete comment) functionality works as intended, but I'm sure it does.

edit: just checked the delete comment seems like it's live already! I would want a "are you sure you want to delete this comment" or something but it should be fine right now

  1. wanted to make sure that only the user who made the comment can edit it (I think this was obvious but perhaps hard to show in the video).

@walidmujahid
Copy link
Contributor Author

walidmujahid commented Feb 26, 2020

@frankchen07 Deleting comments is actually feature that @thelostone-mc -if I am not mistaken- had rolled out.

wanted to make sure that only the user who made the comment can edit it (I think this was obvious but perhaps hard to show in the video).

Yes, only the user who posted the comment can edit. I actually tested this with the very useful user impersonation tool, but I did not show it in any video.

I would want a "are you sure you want to delete this comment" or something but it should be fine right now

That is actually a very good idea. I have opened a feature request at #6084 and add it to my list of things to do

@walidmujahid walidmujahid force-pushed the activity-comments-edit branch 3 times, most recently from e4c58a1 to 4942f7c Compare March 3, 2020 17:58
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a bunch of comments regarding indentation plus using const wherever possible

app/assets/v2/js/activity.js Outdated Show resolved Hide resolved
app/assets/v2/js/activity.js Outdated Show resolved Hide resolved
app/assets/v2/js/activity.js Show resolved Hide resolved
app/assets/v2/js/activity.js Outdated Show resolved Hide resolved
app/assets/v2/js/activity.js Outdated Show resolved Hide resolved
app/assets/v2/js/activity.js Outdated Show resolved Hide resolved
app/assets/v2/js/activity.js Outdated Show resolved Hide resolved
app/assets/v2/js/activity.js Outdated Show resolved Hide resolved
app/assets/v2/js/activity.js Show resolved Hide resolved
app/assets/v2/js/activity.js Show resolved Hide resolved
@walidmujahid
Copy link
Contributor Author

@octavioamu I have worked on the migrations issue. I hope it is better now :-)

@walidmujahid walidmujahid force-pushed the activity-comments-edit branch from 6062fb7 to 424dcad Compare April 2, 2020 23:05
@thelostone-mc
Copy link
Member

@walidmujahid so you migrations you've deleted , a few would aleady been in live
If we merge this in -> things would get messy
Could you simply readd all the migrations files which you haven't created
And then
ensure there is a single migration file for all your model changes

@walidmujahid
Copy link
Contributor Author

Yes. Sorry about that. I will do that as soon as possible.

@walidmujahid walidmujahid force-pushed the activity-comments-edit branch 4 times, most recently from b2785ed to f0ba30a Compare April 8, 2020 12:07
@walidmujahid
Copy link
Contributor Author

@walidmujahid so you migrations you've deleted , a few would aleady been in live
If we merge this in -> things would get messy
Could you simply readd all the migrations files which you haven't created
And then
ensure there is a single migration file for all your model changes

Resolved @thelostone-mc

@walidmujahid walidmujahid force-pushed the activity-comments-edit branch 2 times, most recently from 54844c8 to 81e0f86 Compare April 8, 2020 12:36
@walidmujahid walidmujahid force-pushed the activity-comments-edit branch from 32241fe to 7c5d0c3 Compare April 13, 2020 10:01
Comments can be edited. Editing can be cancled via Cancel button or
ESC.

This is part of the townsquare improvements:
gitcoinco#6003

fixes gitcoinco#6010
@walidmujahid walidmujahid force-pushed the activity-comments-edit branch from 7c5d0c3 to feec2a5 Compare April 16, 2020 04:16
@walidmujahid
Copy link
Contributor Author

walidmujahid commented Apr 16, 2020

@danlipert @thelostone-mc @octavioamu I have updated this with most recent upstream master changes and have recreated the migrations. Manual smoke testing for comment editing functionality passed. Is there anything preventing this from being merged into master?

@thelostone-mc
Copy link
Member

@walidmujahid nah it looks good but looks like CI fails due to eslint errors !
It's indentation fix from the looks of it -> if you can get those in, we can try and get this live this week

@thelostone-mc
Copy link
Member

@walidmujahid nvm I fixed it up !

@walidmujahid
Copy link
Contributor Author

@walidmujahid nvm I fixed it up !

Oh, okay. You were quick :-)

@thelostone-mc thelostone-mc merged commit bbefdc9 into gitcoinco:master Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants