unique nids in revisions.yml

[jywarren]

fixes #8864 i hope!

[gitpod-io]

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (main@0908bfd). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 04ff3e0 differs from pull request most recent head 16c21ed. Consider uploading reports for the commit 16c21ed to get more accurate results

@@           Coverage Diff           @@
##             main    #8865   +/-   ##
=======================================
  Coverage        ?   82.38%           
=======================================
  Files           ?       98           
  Lines           ?     5870           
  Branches        ?        0           
=======================================
  Hits            ?     4836           
  Misses          ?     1034           
  Partials        ?        0           

[jywarren]

OK, so now we probably have to create nodes for nid 39-51, based on copies of the most minimal version of this possible:

plots2/test/fixtures/nodes.yml

Lines 1 to 11 in 5b387b6

	one:
	nid: 1
	uid: 2
	title: "Canon A1200 IR conversion at PLOTS Barnraising at LUMCON"
	path: "/notes/jeff/<%= Time.now.strftime("%m-%d-%Y") %>/canon-a1200-ir-conversion-at-plots-barnraising-at-lumcon"
	created: <%= Time.now.to_i %>
	changed: <%= Time.now.to_i %>
	status: 1
	type: "note"
	cached_likes: 0
	slug: jeff-<%= Time.now.strftime("%m-%d-%Y") %>-canon-a1200-ir-conversion-at-plots-barnraising-at-lumcon

[noi5e]

@jywarren Would it still help to have someone work on this? I'm not sure how urgent this is now that we've switched CI over from Travis... Catching up on this thread just now.

I can probably give this a shot if it makes sense for me to do it (since I'm working with fixtures and adding a bunch of new ones). I'm thinking I would do something like below:

one: # vary this title
   nid: 1 # vary these from 39 - 51
   uid: 2 
   title: # probably good to vary this
   path: # will match this with title
   cached_likes: 0 
   slug: # will match this with path and title, just to be safe

Let me know if there's anything else good to know!

[noi5e]

Just played around with this, pushed some commits. There's still some new failing tests to figure out, so more work should be done on this.

I think it makes sense to add new nodes to match the new revisions, if we're going to go this route... I ran the tests on both scenarios, and it seems easier to untangle if there are new nodes matching the revisions.

Honestly, this might be a red herring-- I still basically get the same 7 failures, 12 errors running the tests. So I'm not sure if the fix will solve any of those outstanding testing issues.

[noi5e]

Currently test-runner is catching four errors in notes_controller_test.rb. They're all kind of similar, so here's just one of them:

plots2/test/functional/notes_controller_test.rb

Lines 657 to 664 in 9c409ec

	test 'should list only research notes with status 1 in index' do
	get :index
	notes = assigns(:notes)
	expected = [nodes(:one)]
	questions = [nodes(:question)]
	assert (notes & expected).present?
	assert !(notes & questions).present?
	end

I think what's going on is that get :index is only pulling the first 20 notes due to pagination, maybe? That's why nodes(:one) doesn't appear in the array as expected, because there's 20+ new node fixtures. Possible fix would be to:

• Get nodes(:one) to appear in the first 20 results, perhaps by playing around with timestamps?
• Rewrite the test?

Any ideas or suggestions?

EDIT: Here are the notes that :index retrieves via home#dashboard

plots2/app/controllers/home_controller.rb

Lines 79 to 87 in 9c409ec

	def activity
	blog = Tag.find_nodes_by_type('blog', 'note', 1).first
	# remove "classroom" postings; also switch to an EXCEPT operator in sql, see https://github.com/publiclab/plots2/issues/375
	hidden_nids = Node.hidden_response_node_ids
	notes = Node.where(type: 'note')
	.where('node.nid NOT IN (?)', hidden_nids + [0]) # in case hidden_nids is empty
	.order('nid DESC')
	.page(params[:page])
	notes = notes.where('nid != (?)', blog.nid) if blog

They're sorted in descending order by nid, so a note with nid:1 would appear toward the end of the array.

Will pagination even affect the results that are in @notes?

[jywarren]

I would love help, to be honest, i didn't manage to get to this today! Pls note that we seem to be pretty far from synced with the main branch though.

The failure mode here, which may help with your Qs, is that different databases and environments can /order/ fetched records differently, especially if they have the same created_at value, and in this case, if they have the same nid which otherwise would make their ordering predictable. So, absent something to order them, they get jumbled in order, which could account for your observed issue on the index page, (as it might move in and out of the first 20?).

That said, this may not solve other issues, not sure -- but it hopefully will reduce the incidence of some tests passing in one environment but failing in another due to subtle differences in how databases sort un-sortable data (i.e. how they handle a bunch of records with identical fields).

Hope this makes sense -- thank you!!!!

[jywarren]

OK, this will take a lot of work. But we should also try to incorporate a home-intro explanatory feature in nodes.yml and revisions.yml to ensure something more friendly shows up to developers when first booting the front page:

plots2/app/views/home/home.html.erb

Line 2 in 2c68070

<%= feature("home-intro") %>

[jywarren]

This needs a rebase! But also, let's add comments at the bottom of nodes.yml, revisions.yml, and other relevant files that clearly explain how we need unique nids.

[noi5e]

Ugh, the commit history for this PR was awful before! Sorry, that was my bad, it was an attempt gone wrong at git rebase.

But I just did another git rebase with the current main branch and it looks a lot better now.

Let's see how the tests do. There were four merge conflicts that were a little funny that I undid manually. But I'm sure the tests will be more revealing if I did the merge conflicts wrong.

If I recall correctly, this can't be merged as is. There's a system test that was failing that I mentioned above here, that I believed at the time had to do with results pagination on :index. But we'll see.

[jywarren]

Gotcha, let me see if i can get past that pagination one. Should be pretty easy.

[jywarren]

Also noting the functional test failures were:

========== FAIL NotesControllerTest#test_should_list_only_research_notes_with_status_1_in_index (81.68s)
        Expected false to be truthy.
        test/functional/notes_controller_test.rb:665:in `block in <class:NotesControllerTest>'

 FAIL NotesControllerTest#test_should_list_only_research_notes_with_status_1_in_liked (81.97s)
        Expected false to be truthy.
        test/functional/notes_controller_test.rb:704:in `block in <class:NotesControllerTest>'

 FAIL NotesControllerTest#test_should_list_only_research_notes_with_status_1_in_popular (82.26s)
        Expected false to be truthy.
        test/functional/notes_controller_test.rb:685:in `block in <class:NotesControllerTest>'

 FAIL NotesControllerTest#test_should_list_research_notes_with_status_1_&_4_in_index_if_admin_is_logged_in (82.65s)
        Expected false to be truthy.
        test/functional/notes_controller_test.rb:675:in `block in <class:NotesControllerTest>'

[jywarren]

OK let's just rewrite those 4 to actually assert exactly what they say - that in the first one, all results are status == 1, for example. That should solve it and the current assertions aren't very good anyways!

[jywarren]

plots2/test/functional/notes_controller_test.rb

Lines 665 to 704 in a291fe0

	assert (notes & expected).present?
	assert !(notes & questions).present?
	end
	test 'should list research notes with status 1 & 4 in index if admin is logged in' do
	UserSession.create(users(:admin))
	get :index
	notes = assigns(:notes)
	expected = [nodes(:one), nodes(:first_timer_note)]
	questions = [nodes(:question)]
	assert (notes & expected).present?
	assert !(notes & questions).present?
	end
	test 'should list only research notes with status 1 in popular' do
	UserSession.create(users(:admin))
	get :popular
	notes = assigns(:notes)
	expected = [nodes(:one)]
	questions = [nodes(:question)]
	assert (notes & expected).present?
	assert !(notes & questions).present?
	end
	test 'should list only research notes with status 1 in recent' do
	get :recent
	notes = assigns(:notes)
	expected = [nodes(:one)]
	questions = [nodes(:question)]
	assert (notes & expected).present?
	assert !(notes & questions).present?
	end
	test 'should list only research notes with status 1 in liked' do
	UserSession.create(users(:admin))
	get :liked
	notes = assigns(:notes)
	expected = [nodes(:one)]
	questions = [nodes(:question)]
	assert (notes & expected).present?

[jywarren]

OK, narrowed the test assertions a bit. Let's see. There may be other issues.

[jywarren]

We may need to push the "first-time-poster" node to the end so it shows up. But, maybe we are OK with it not appearing on the /notes index route anymore anyways.

[jywarren]

OK, so the functional test could be fixed by #9321 -- let's rebase over.

======= FAIL NotesControllerTest#test_should_list_research_notes_with_status_1_&_4_in_index_if_admin_is_logged_in (30.38s)
        Expected false to be truthy.
        test/functional/notes_controller_test.rb:669:in `block in <class:NotesControllerTest>

System tests...

ERROR DashboardTest#test_User_can_flag_a_node_from_dashboard (974.95s)
Minitest::UnexpectedError:         Capybara::ElementNotFound: Unable to find css "#flag_node1"
            test/system/dashboard_test.rb:34:in `block in <class:DashboardTest>'

I bet it's a reordering issue related to the nids. Let's check...

[jywarren]

plots2/test/system/dashboard_test.rb

Lines 26 to 36 in 04f5093

	test "User can flag a node from dashboard" do
	visit '/'
	click_on 'Login'
	page1 = nodes(:one)
	fill_in("username-login", with: "Bob")
	fill_in("password-signup", with: "secretive")
	click_on "Log in"
	visit '/dashboard'
	find("#flag_node#{page1.id}").click()
	assert find("div.alert", text: "Node flagged.")
	end

[jywarren]

I bet that's pagination. Let's adjust that so it uses the last node.

[jywarren]

Rebased over the fix...

[jywarren]

ALMOSTTTTTT

[codeclimate]

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

View more on Code Climate.

[jywarren]

Even after these NID fixes, this still went wrong:

Progress: | FAIL TagSelectionTest#test_graph (0.33s)
        Expected: 18
          Actual: 23
        test/unit/tag_selection_test.rb:14:in `block in <class:TagSelectionTest>'

====================================================================|

I wonder if that's a different issue, non-NID? We can follow up in a new issue.

https://github.com/publiclab/plots2/pull/8865/checks?check_run_id=2126281351

[jywarren]

Yes, the tag test seems totally unrelated:

plots2/test/unit/tag_selection_test.rb

Lines 5 to 16 in d70efef

	test 'graph' do
	end_time = Time.now
	start_time = end_time - 1.year
	ts_count = TagSelection.select(:following, :created_at)
	.where(following: true, created_at: (start_time..end_time))
	.count
	graph = TagSelection.graph(start_time, end_time)
	assert_equal ts_count, graph.values.sum
	assert_equal Hash, graph.class
	end