[System Tests] addComment(URL), Add New Question Fixture

[noi5e]

This is a draft PR, I'm not there yet. Feedback very welcome as I'm learning a lot about plots2, and a lot else.

This adds a single new system test, testing for "User adds comment to a question page." In particular, this tests for commenting via addComment(URL) (a JS method that seemingly comes with comment forms).

In my understanding, question pages seem to reuse the same form partial (below) that other nodes (wikis, notes) use. A crucial difference for question comments is that a ?type=question parameter is added to the AJAX request URL. So that's the difference in this test:

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

Point of Concern
The other comment tests in comment_test.rb are testing comments on the page /wiki/wiki-page-path/comments. wiki-page-path seems to be a dummy article that exists only in the testing environment. It doesn't exist in development locally, nor in production at publiclab.org (navigating to https://site + /wiki/wiki-page-path 404s at both).

My concern is that this test should probably be testing on an actual question page (/question/{id-as-string}/comments, or the equivalent). However, I don't know what the testing convention is.

• How is wiki-page-path generated? Is it seeded into the development database somehow?
• Is there a way to make a similar question page for testing purposes? (Or for that matter, a note, which we also need tests for)
• There are tests for posting a new question from scratch in post_question_test.rb, it seems a little bit involved though:
  

  plots2/test/system/post_question_test.rb

  Lines 23 to 71 in cbb807b

  	test 'posting a question' do
  	visit '/questions/new'
  	find('[placeholder="What\'s your question? Be specific."]').set("Let's test this, shall we?");
  	page.execute_script <<-JS
  	// Create button element
  	var btn = document.createElement('button');
  	btn.id = 'copy-to-clipboard';
  	btn.textContent = "Click me to copy the text!";
  	// Copying to the clipboard requires user's interaction
  	btn.addEventListener('click', copyToClipboard);
  	document.body.appendChild(btn);
  	function copyToClipboard() {
  	// Create temporary textarea element
  	var tempEl = document.createElement('textarea');
  	// Set the value to the textarea
  	tempEl.value = `
  	1. Post your suggested activity as an Answer below (not a comment).
  	2. Other people can Comment on that idea.
  	3. Other people can Like (star) that idea.
  	`;
  	document.body.appendChild(tempEl);
  	// Copy the content from the textarea
  	tempEl.select();
  	document.execCommand('copy');
  	document.body.removeChild(tempEl);
  	};
  	JS
  	# Copy text to clipboard
  	find('#copy-to-clipboard').click()
  	# Paste it in the question's body
  	find('.wk-wysiwyg').native.send_keys [:control, 'v']
  	find('.ple-publish').click();
  	# Wait for note to be published
  	wait_for_ajax
  	message = find('.alert-success').text
  	note_title = find('.note-show h1', match: :first).text
  	assert_equal( "Let's test this, shall we?", note_title )
  	assert_equal( "×\nResearch note published. Get the word out on the discussion lists!", message )
  	end

  What's more preferable: generating something like wiki-page-path or creating a new question from scratch in the test, and posting the comment to it?

(This PR is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

[gitpod-io]

[ebarry]

Thank you @noi5e!!! great, incisive questions! I trust the maintainers will engage with you to answer these 🌳

[jywarren]

Hi @noi5e - so, /wiki/wiki-page-path is likely seeded test data - sometimes called fixtures - which are instantiated during testing as a 'sample set' of database records to perform tests on. This one is oddly named as we usually name them something slightly more realistic, and sometimes silly, like /wiki/jeffs-awesome-page or something. But I looked and indeed it is a text fixture entry for the Node table:

plots2/test/fixtures/nodes.yml

Lines 122 to 134 in cbb807b

	# this wiki has a differnt title and path
	# this is a gatekeeper to test nodes with differnt title and path
	wiki_page:
	nid: 11
	uid: 1
	title: "Wiki page title"
	path: "/wiki/wiki-page-path"
	created: <%= Time.now.to_i %>
	changed: <%= Time.now.to_i %>
	status: 1
	type: "page"
	cached_likes: 0
	slug: wiki-page-title

That means it's an OK place to test comments, but yeah, it's also not the most common place for commenting to happen. Commenting may be set up differently here than in other places, such as on a note or question. Or on a note with a comment, where we're responding to the comment. (think of our 8 scenarios). So i think you're right to want to do tests on a question page and a note page as well. You could look at the tests for questions and notes to see what an appropriate fixture page is to do these, or, better, add a new fixture, to reduce interdependence of tests (that way, if we remove another feature and its tests, we don't break this test).

Also, i think that type=question suffix is no longer needed either! We can see it's only used in one place and presumably the place in the controller where it actually did something has since been removed: https://github.com/publiclab/plots2/blob/main/app/controllers/comment_controller.rb#L34

So, we could remove that conditional from the comment form and the corresponding lines using answer_id from the controller (i see two -- one in delete as well).

Hope this helps, thanks @noi5e for the great work here!

[jywarren]

Oh! And regarding that very involved system test above -- // Create button element etc... it's a test specifically testing whether you can paste text into the comment box and get it to upload! Kind of cool, if obscure. Maybe we ought to rename that test accordingly! I could imagine it being useful for testing pasting an image... but pasting text? Not sure, but let's look deeper.

We can look at the poorly named blame view (why blame??? appreciate, more like it!) and see who added it and why: https://github.com/publiclab/plots2/blame/cbb807ba8e2302f09dafc0060475aa118e34c2c6/test/system/post_question_test.rb#L23-L71

Looks like it was @VladimirMikulic ! Hello Vladimir! Good to see you! Just curious about that segment using the clipboard! Thanks!!!!

[VladimirMikulic]

Hello @jywarren. Good to see you as well! That segment using clipboard is JavaScript hack/workaround to fake the actual user's interaction. My commit message suggests that this test exists because of an error that you had encountered with PL Editor as described here. It serves as a reinforcement that the error is spotted if occurs again in the future.

[noi5e]

@jywarren (Thanks for the explanation on fixtures BTW, very helpful)

Okay, coming a little further along with this, hit a new roadblock debugging the test.

I've created a new fixture in nodes.yml simulating a question:

question4: # fresh question fixture for testing comments on question pages
  nid: 37
  uid: 2
  title: "Can I post comments here"
  path: "/questions/jeff/12-07-2020/can-i-post-comments-here"
  created: <%= DateTime.new(2020,12,7).to_i %>
  changed: <%= DateTime.new(2020,12,7).to_i %>
  status: 1
  type: "note"
  cached_likes: 0
  slug: jeff-12-07-2020-can-i-post-comments-here

The corresponding test looks like:

test 'question page: add synchronous comment via javascript with URL only' do
    visit "/questions/jeff/12-07-2020/can-i-post-comments-here"
    page.evaluate_script("addComment('yes you can', '/comment/create/37')")
    assert_selector('#comments-list .comment-body p', text: 'yes you can')
  end

In particular the test seems to stall out at visit "/questions/jeff/12-07-2020/can-i-post-comments-here". The error I receive is:
("GET /questions/jeff/12-07-2020/can-i-post-comments-here" - (127.0.0.1)): #<NoMethodError: undefined method 'path' for nil:NilClass>

Here's the show route in questions_controller.rb:

plots2/app/controllers/questions_controller.rb

Lines 69 to 77 in c069cfb

	def show
	if params[:author] && params[:date]
	@node = Node.find_notes(params[:author], params[:date], params[:id])
	@node ||= Node.where(path: "/report/#{params[:id]}").first
	else
	@node = Node.find params[:id]
	end
	redirect_to @node.path unless @node&.has_power_tag('question')

With some good old-fashioned puts logging I've determined that @node in this context doesn't have the requisite power_tag('question'). I'm a little murky about what happens next. Because it's missing power_tag('question'), the controller can't proceed as normal and load the question-page view.

So it goes down the other leg of the conditional and redirect_to @node.path. But what does that mean? I think... It redirects to /notes instead of /questions? Not sure. Turns out @node.path is undefined in this context (weirdly, I thought I set that when I defined the fixture), and that's what throws the error.

• Any insight into what is throwing the error here?
• It seems like setting power_tag('question') is going to be crucial if I'm going to go down the road of simulating a fresh question page. I looked through the other tests and couldn't find any similar examples on how to do this. Any ideas?

[noi5e]

I added a missing piece here. Thanks to @cesswairimu on chat, I finally am getting what a power_tag is and how that relates to a question. I added a fixture that simulates a tag:

question4: 
  tid: 35
  name: question:general

And a fixture that simulates a node-tag:

question4:
  tid: 35
  uid: 2
  nid: 37
  date: <%= DateTime.now.to_i %>

This is all so that the note has a power tag of question:general.

Unfortunately, I'm still getting the same error I was getting above when running the test. I have a feeling but can't confirm that the hang-up is happening on this line:

plots2/app/controllers/questions_controller.rb

Line 71 in c069cfb

@node = Node.find_notes(params[:author], params[:date], params[:id])

There might be something wrong with parsing the URL path for author, date, and ID that makes the query come up undefined... Not sure. Any insights would be appreciated, as well as ideas on how to debug Rails tests (something that is still pretty new for me)

[cesswairimu]

I will pull this up on my end and have a look

[noi5e]

@cesswairimu FYI, was able to understand this a little better.

Here's a crucial mistake I was making in the fixture:

path: "/questions/jeff/12-07-2020/can-i-post-comments-here"

The path should lead with /notes, not /questions. questions_controller.rb can't find a matching @node because it calls the method find_notes:

plots2/app/controllers/questions_controller.rb

Line 71 in c069cfb

@node = Node.find_notes(params[:author], params[:date], params[:id])

Which looks like this in node.rb 🤦‍♂️:

plots2/app/models/node.rb

Lines 904 to 906 in c069cfb

	def self.find_notes(author, date, title)
	Node.where(path: "/notes/#{author}/#{date}/#{title}").first
	end

Side-note: it looks like when a note is generated in node.rb, its path begins with /notes:

plots2/app/models/node.rb

Lines 144 to 160 in c069cfb

	# should only be run at actual creation time --
	# or, we should refactor to use node.created instead of Time.now
	def generate_path
	time = Time.now.strftime('%m-%d-%Y')
	case type
	when "note"
	username = User.find_by(id: uid).name # name? or username?
	"/notes/#{username}/#{time}/#{title.parameterize}"
	when "map"
	"/map/#{title.parameterize}/#{time}"
	when "feature"
	"/feature/#{title.parameterize}"
	when "page"
	"/wiki/#{title.parameterize}"
	end
	end

It looks like the method node.path will return a path /questions if you specify it as a parameter:

plots2/app/models/node.rb

Lines 134 to 142 in c069cfb

	# can switch to a "question-style" path if specified
	def path(type = :default)
	if type == :question
	self[:path].gsub('/notes/', '/questions/')
	else
	# default path
	self[:path]
	end
	end

But I'm thinking... There's no question in the database saved with a path that begins with /questions instead of /notes. Is that right?

So I have some more clarity there, but getting a new error when I change /questions to /notes:

It looks like my fixture needs to have latest defined?

plots2/app/controllers/questions_controller.rb

Line 82 in c069cfb

@title = @node.latest.title

I don't yet understand what latest is all about, have to research that, but it's coming along. Slow but steady...

[noi5e]

@jywarren @cesswairimu Did some more research, it looks like @node.latest queries a node's revisions. I haven't yet researched what a revision is (it seems like it contains the body of the post? every time the user edits the note, it's saved as a revision?), but I went ahead and made a fixture for it in revisions.yml:

question4: # fresh question fixture for testing comments on question pages
  nid: 37
  uid: 2
  title: "Can I post comments here"
  body: "I'm gonna do it"
  timestamp: <%= DateTime.new(2020,12,7).to_i %>
  status: 1

After that, the test passes. I think I actually... got there?

Just pushed my commits, I think this one's ready for review! I'm still very new to everything here, so my work could use some double-checking. Thanks for reading my mountain of notes.

[jywarren]

You solved it! Sorry, this happened over a period i was mostly offline, but glad you dug deeply and got to the answers! You're totally right on all counts, regarding revisions especially.

But I'm thinking... There's no question in the database saved with a path that begins with /questions instead of /notes. Is that right?

That sounds right. Being a question or not is determined by the power tag, so we can assume no nodes actually have 'questions' in the path -- we can think of the /questions/ type paths as aliases, essentially, enabled by the power tag's presence.

The whole system is a little odd, but it basically works. I think you go this all worked out, it's ready for a final code review and if it passes all tests (we can manually confirm this by running them in GitPod for now) we can mark this ready to merge!

[jywarren]

This looks perfect, as long as all the tests pass! Thanks, @noi5e !!! Great work and your deep digging here is much appreciated! @RuthNjeri take a look in case you run into similar issues! Thanks, all, and thanks @cesswairimu for your help on this too!

[jywarren]

Hi! I've tried running tests in GitPod, to confirm (as we're still a little stuck on the Travis migration), but I found that the gems and database setup aren't completing smoothly in there. Has our GitPod environment broken down a bit? Sometimes we find we need to check in and tweak things to make sure the newcomer experience stays smooth. @noi5e did you find GitPod to be working smoothly? Thanks!

[noi5e]

@jywarren You know, I have dev set up locally since I posted that comment about GitPod to chat! So I haven't been using GitPod since.

[codeclimate]

Code Climate has analyzed commit bf2198c and detected 0 issues on this pull request.

View more on Code Climate.

[noi5e]

Wow, good thing I tested this locally. Because I made a new question fixture, it invalidated the results of this unit test for nodes:

plots2/test/unit/node_test.rb

Lines 360 to 364 in ff3417a

	test 'should find all questions' do
	questions = Node.questions
	expected = [nodes(:question), nodes(:question2), nodes(:first_timer_question), nodes(:question3), nodes(:sun_question)]
	assert_equal expected, questions
	end

I updated the expected array to include the new fixture, question4.

Now there doesn't seem to be any added test failures or errors from this branch (I get the same numbers as when I test my main branch).

Side-note: I've noticed some of the question tests are failing... I've looked into the issue and it doesn't seem to be caused by my fixture. I will make a new issue about it.

[Sagarpreet]

I've noticed some of the question tests are failing

1. Are those tests failing due to any recent PR merge?
2. After reverting your changes, are those tests still failing?

[noi5e]

@Sagarpreet Thank you, the failing tests I mentioned seem to be unrelated to my PR:

• they fail on my main branch, with the same expected and actual results
• they fail with the same results on the PR branch

This is all happening on my local build. Travis and Gitpod are both down, so don't have a way of verifying.

It seems like it's worth it to say something on Gitter, so I'll do that right now!

[jywarren]

This is great, and very cool to see you caught:

I updated the expected array to include the new fixture, question4.

Now there doesn't seem to be any added test failures or errors from this branch (I get the same numbers as when I test my main branch).

Glad to hear that no new test failures are caused by this. One thing to bear in mind is that there can be some test failures that are environment dependent, which is not ideal but is understandable in a complex app. For example, i think some tests rely on Redis, perhaps? But in general you can imagine that testing could get into the specifics of setup. Unit tests shouldn't if they're testing internal, fundamental behaviors of code, but even then maybe we test mails sent, or something, and get in trouble that way. And, we can get complacent about test consistency when we use a Continuous Integration service like Travis, so maybe this is good for us to confront, thinking in the long term.

Thanks for your extra attention to the tests and i'll go ahead and merge this now!

[jywarren]

Just noting also that I'm also more OK with merging this as it only affects tests and not any new functionality. Thanks again!