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

OLD CLOSED Comment Editor Overhaul Issue #8775

Closed
cesswairimu opened this issue Oct 9, 2020 · 14 comments
Closed

OLD CLOSED Comment Editor Overhaul Issue #8775

cesswairimu opened this issue Oct 9, 2020 · 14 comments

Comments

@cesswairimu
Copy link
Collaborator

@cesswairimu cesswairimu commented Oct 9, 2020

MOVED PLANNING ISSUE HERE -> #9069

I've decided to move my planning issue to a fresh issue page, mostly for issues of readability. Sorry for any confusion this causes! - @noi5e

@jywarren
Copy link
Member

@jywarren jywarren commented Nov 25, 2020

For the later stages of the Comment Editor project when it might be spun out into its own repository, we may want to carefully consider this workflow documented by Sagarpreet for what would help create a standard release cycle!

https://publiclab.org/notes/sagarpreet_chadha/10-20-2020/first-timer-only-release-workflow

@jywarren
Copy link
Member

@jywarren jywarren commented Dec 1, 2020

Noting this example of cross-wired comment boxes during image upload to a comment being edited (not newly written): #8670

@noi5e
Copy link
Contributor

@noi5e noi5e commented Dec 2, 2020

MOVED PLANNING ISSUE HERE -> #9069

I've decided to move my planning issue to a fresh issue page, mostly for issues of readability. Sorry for any confusion this causes! - @noi5e

@noi5e noi5e added this to the Comment editor milestone Dec 2, 2020
@sagarpreet-chadha
Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha commented Dec 6, 2020

Hi @noi5e , the planning issue looks great.
Which issue are you currently working on? Can you point me to the PR if any, also let me know if you are stuck somewhere or meed some clarity. Thanks 😄

@noi5e
Copy link
Contributor

@noi5e noi5e commented Dec 6, 2020

@sagarpreet-chadha Thank you for checking in with me! Since posting this, been writing my blogpost for Outreachy (in bits and pieces, almost done), researching and working on a PR for 8618, and brushing up on Rails-- which I don't have a strong background in, but have been learning a lot this week. Worked 2 days at my other job so am feeling a little behind, but did a lot of catch-up today and will do more tomorrow & Monday. I plan on writing tests for comments tomorrow.

I need a little clarification about testing comments. I took a look at /plots2/blob/main/test/system/comment_test.rb and it looks like there are already tests for entering comments both manually and via API URL, both pointing toward /wiki/wiki-page-path/comments.

If I'm going to write new comment tests for question and note pages, will it be for a different API URL or the same as above (wiki-page-path)? I ran rails routes but it's not obvious to me which ones I'm supposed to be testing. Thanks in advance, this will save some time hunting through the code. 😅

EDIT: Also let me know if you have specific requests for types of tests for me to write!

@jywarren jywarren changed the title [Planning] Comment Editor Overhaul Project [Planning] Comment Editor Overhaul Project (Outreachy) Dec 6, 2020
@jywarren
Copy link
Member

@jywarren jywarren commented Dec 6, 2020

Hi @noi5e i believe the "wiki comments" route is distinct from the "notes" one - both wikis and notes are variants of Node - but should be pretty similar.

It's been a while since I looked at this area of code, but typically Rails will have a controller action for each route, but the exception is when they are resourceful routes: https://guides.rubyonrails.org/routing.html#resource-routing-the-rails-default

Seeing this action is for viewing comments:

def comments
show
render :show
end

it seems there isn't a controller action for posting. But i do see that this route:

get '/wiki/:id/comments', to: 'wiki#comments'

points at that view action. So where are creation routes and actions? I see some here, not wiki-specific though:

plots2/config/routes.rb

Lines 368 to 373 in cbb807b

get 'comment/delete/:id' => 'comment#delete'
get 'comment/update/:id' => 'comment#update'
post 'comment/update/:id' => 'comment#update'
post '/comment/like' => 'comment#like_comment'
get '/comment/create/:id' => 'comment#create'
post 'comment/create/:id' => 'comment#create'

So let's look at the other end of things, the templates. Here is the basic comment form:

https://github.com/publiclab/plots2/blob/main/app/views/comments/_form.html.erb

I think question comments may re-use this form, based on the logic in the <form> tag here?

<form class="comment-form" id="comment-form" data-remote="true" action="/comment/create/<%= @node.nid %><%= "?type=question" if local_assigns[:type]=="question" %>" <% if local_assigns[:aid].blank? %>method="post"<% end %>>

So, they point at /comments/create/NID with a POST method, so the very last route from line 373 of routes.rb. Those point at the comment controller create action:

# handle some errors!!!!!!
# create node comments
def create
@node = Node.find params[:id]
@body = params[:body]
@user = current_user
begin
@comment = create_comment(@node, @user, @body)
if params[:reply_to].present?
@comment.reply_to = params[:reply_to].to_i
@comment.save
end
respond_to do |format|
@answer_id = 0
format.js do
render 'comments/create'
end
format.html do
if request.xhr?
render partial: 'notes/comment', locals: { comment: @comment }
else
tagnames = @node.tagnames.map do |tagname|
"<a href='/subscribe/tag/#{tagname}'>#{tagname}</a>"
end
tagnames = tagnames.join(', ')
tagnames = " Click to subscribe to updates on these tags or topics: " + tagnames unless tagnames.empty?
flash[:notice] = "Comment posted.#{tagnames}"
redirect_to @node.path + '#last' # to last comment
end
end
end
rescue CommentError
flash.now[:error] = 'The comment could not be saved.'
render plain: 'failure', status: :bad_request
end
end

So i think it's safe to say most comments are using the comment controller create action. But what about those system tests where that route is not mentioned?

The other way it's happening is here is via a JavaScript method:

page.evaluate_script("addComment('superhero', '/comment/create/11')")

That leads us to this JS file, which submits it to the same route via AJAX:

// JS API for submitting comments
function addComment(comment, submitTo, parentID = 0) {
submitTo = submitTo || '.comment-form'; // class instead of id because of the bound function on line 3
let data = { body: comment };
if (parentID) {
data.reply_to = parentID;
}
sendFormSubmissionAjax(data, submitTo);
}

The way our codebase looks now, the sendFormSubmissionAjax() function may look like it didn't have to be in its own file, but i think @nstjean (a fantastic Outreachy alum, hi!!!!!! 🎉 ) planned for it to streamline JS form submission across our codebase, which i fully support. Here it is!

// Takes in a data object that contains the info to be submitted in the form property: dataString
// and the url to submit to the controller
// Allows data to be submitted from anywhere on the page using Javascript without using the form itself
function sendFormSubmissionAjax(dataObj, submitTo, responseEl = "") {

Hope this is helpful! It's not the worst i've seen in terms of code organization, but it's definitely convoluted. This is partly just because this is a big and old codebase which has seen many different phases of revision. Actually, prior to system tests, which only got installed in the past 2 years or so, we had no way to do full-stack testing of JavaScript comment submission! So it was constantly breaking :-(

Thanks folks!!!

@noi5e
Copy link
Contributor

@noi5e noi5e commented Dec 7, 2020

@jywarren Thanks so much for that very helpful and detailed writeup!!! It really helped me figure out a lot of things about the codebase. Still a lot to learn. I made a tentative PR for a test-- with a lot of questions.

jywarren pushed a commit that referenced this issue Dec 14, 2020
… Page via JS + URL (#8801)

* add test for synchronous comments on question pages via JS & URL

* changed test so it visits /questions, not /wiki/wiki-page-path

* create new question fixture for testing complete with tags, node_tags, and revisions

* Update "Find all questions" node unit test
@jywarren
Copy link
Member

@jywarren jywarren commented Dec 14, 2020

Also looking to clarify these:

"Save and recover icons Key feature causing Mimi to workaround" (don't get this)

I couldn't actually find this in https://pad.publiclab.org/p/outreachy - where did it come from? It sounds vaguely familiar... sorry!

Preview doesn't work sometimes (I can't reproduce)

Maybe this is a good candidate for the kind of "standard suite of tests" we could automatically run on multiple variants of the comment editor, when we re-organize the test code? I wonder if that would catch it, or perhaps it's just already resolved.

Bold and italic buttons are "cross-wired" (can't reproduce)

This seems to be in #8478 and i wonder if the same strategy could help, of including this in a standard suite of comment editor tests and running across all variants.

Ensure grey "drag-and-drop to insert images" is everywhere (can't reproduce, would like some examples)

Same as above maybe... and i also couldn't find it in https://pad.publiclab.org/p/outreachy so maybe i can add more, knowing where it came from?

Issues with liking comments (See "Comment Likes Does Not Work #5113"). I understand the issue, basically I need to clarify what the expected behavior here is.

This might just be really old?? We don't have comment likes anymore, as it was replaced by the "reactions" system. But, maybe we clarify by asking Sasha, who made #5113

Thanks, @noi5e ! Hope these help!!!!

@noi5e
Copy link
Contributor

@noi5e noi5e commented Dec 14, 2020

Thanks @jywarren definitely helpful... I think the points you were looking for are in this Google doc

@jywarren
Copy link
Member

@jywarren jywarren commented Jan 18, 2021

Hi @noi5e i'm not sure if you saw this or fixed this already but i noticed this on our comments - there seems to be an unnecessary grey bar under the form:

image

Also, i think some of the spacing around the emojis is a little off. See how it seems like it has white padding on top and bottom, esp when compared to GitHub's style?

image

Just a couple tiny things maybe we could address in an upcoming PR, but nothing urgent. Thanks!

@noi5e
Copy link
Contributor

@noi5e noi5e commented Jan 18, 2021

@jywarren Definitely, I have noticed those too! Adding to my to-dos.

@jywarren
Copy link
Member

@jywarren jywarren commented Jan 21, 2021

I also was wondering after the speed optimizations of #9045, i remember that Skylight shows that actually posting a comment on PublicLab.org can take quite a while... and I thought we had optimized somewhat but not as much as we'd hoped. Do you still find it to be a slow load time to post a comment? I wonder if that's also a way to improve system test runtime, while improving user experience as well.

Here's a snapshot; it apparently still takes up to 10 seconds occasionally, but unfortunately our monitoring isn't good enough to show more than that this time is spent in the controller, it seems:

image

This is also not a requirement of your project, but i thought it might be interesting. The code driving this is here and here.

No worries really about this to be honest... but i am curious if you felt that comment post time was quite slow or not?

@noi5e
Copy link
Contributor

@noi5e noi5e commented Jan 21, 2021

@jywarren Yes! I definitely think that time to post comments is very slow! (also, time to react to a comment)

Interesting to learn about Skylight and what it does. I wonder what exactly is causing the slowdown, do you think it might be the ActiveRecord query?

EDIT: adding it to the stretch goals wishlist in this Planning Issue!

@noi5e noi5e changed the title [Planning] Comment Editor Overhaul Project (Outreachy) OLD CLOSED Comment Editor Overhaul Issue Jan 23, 2021
@noi5e noi5e closed this Jan 23, 2021
@noi5e
Copy link
Contributor

@noi5e noi5e commented Jan 23, 2021

MOVED PLANNING ISSUE HERE -> #9069

I've decided to move my planning issue to a fresh issue page, mostly for issues of readability. Sorry for any confusion this causes! - @noi5e

manchere added a commit to manchere/plots2 that referenced this issue Feb 13, 2021
… Question Page via JS + URL (publiclab#8801)

* add test for synchronous comments on question pages via JS & URL

* changed test so it visits /questions, not /wiki/wiki-page-path

* create new question fixture for testing complete with tags, node_tags, and revisions

* Update "Find all questions" node unit test
lagunasmel added a commit to lagunasmel/plots2 that referenced this issue Mar 2, 2021
… Question Page via JS + URL (publiclab#8801)

* add test for synchronous comments on question pages via JS & URL

* changed test so it visits /questions, not /wiki/wiki-page-path

* create new question fixture for testing complete with tags, node_tags, and revisions

* Update "Find all questions" node unit test
reginaalyssa added a commit to reginaalyssa/plots2 that referenced this issue Oct 16, 2021
… Question Page via JS + URL (publiclab#8801)

* add test for synchronous comments on question pages via JS & URL

* changed test so it visits /questions, not /wiki/wiki-page-path

* create new question fixture for testing complete with tags, node_tags, and revisions

* Update "Find all questions" node unit test
billymoroney1 added a commit to billymoroney1/plots2 that referenced this issue Dec 28, 2021
… Question Page via JS + URL (publiclab#8801)

* add test for synchronous comments on question pages via JS & URL

* changed test so it visits /questions, not /wiki/wiki-page-path

* create new question fixture for testing complete with tags, node_tags, and revisions

* Update "Find all questions" node unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants