OLD CLOSED Comment Editor Overhaul Issue

[cesswairimu]

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]

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]

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

[noi5e]

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

[sagarpreet-chadha]

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]

@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]

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:

plots2/app/controllers/wiki_controller.rb

Lines 467 to 470 in cbb807b

	def comments
	show
	render :show
	end

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

plots2/config/routes.rb

Line 58 in cbb807b

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?

plots2/app/views/comments/_form.html.erb

Line 3 in cbb807b

<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:

plots2/app/controllers/comment_controller.rb

Lines 19 to 56 in cbb807b

	# 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:

plots2/test/system/comment_test.rb

Line 32 in cbb807b

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

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

plots2/app/assets/javascripts/comment.js

Lines 62 to 70 in 484bf69

	// 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!

plots2/app/assets/javascripts/submit_form_ajax.js

Lines 1 to 4 in 876d0fc

	// 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]

@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]

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]

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

[jywarren]

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:

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?

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

[noi5e]

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

[jywarren]

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:

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]

@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]

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