Show merged stories on duplicate submission

[myhro]

Closes #1215.

The approach consists in not filtering out merged stories (avoiding the unmerged filter) and showing them in the returned HTML.

Please let me know if should we do it differently.

[myhro]

I've tried to add a test for this change by creating a spec/controllers/stories_controller_spec.rb file:

# typed: false

require "rails_helper"

describe StoriesController do
  let(:user) { create(:user) }
  let(:story) { create(:story, user: user) }

  describe "#check_url_dupe" do
    it "fails when URL is missing" do
      expect {
        post :check_url_dupe, params: { story: { url: '' } }
      }.to raise_error(ActionController::ParameterMissing)
    end

    it "should return an existing story" do
      post :check_url_dupe, params: { story: { url: story.url } }, format: :html
      expect(response).to include("Previous discussions for this story")
    end
  end
end

The first test worked fine, but the second failed with:

NoMethodError: undefined method `keys' for nil:NilClass

  0) StoriesController#check_url_dupe should return an existing story
     Failure/Error: expect(response).to include("Previous discussions for this story")

     NoMethodError:
       undefined method `keys' for nil:NilClass
     # ./spec/controllers/stories_controller_spec.rb:18:in `block (3 levels) in <top (required)>'
2 examples, 1 failure, 1 passed

I spent a while trying to figure where this keys method is coming from, with no success. Not even a debugger helped.

Not sure if I'm being too naive and maybe creating tests for a controller is more complicated than that.

[pushcx]

That’s a real painful test failure to debug. I didn’t see it when I read your test, but after I pulled it down and got the exception for myself I caught that the test says expect(response), not expect(response.body). Just one of those things that takes a fresh pair of eyes to see.

I ran it with rspec -b to see the full backtrace and whew, it’s real deep in the plumbing of super_diff, the filenames sort of imply it was trying to compare objects. I think you’ve found a bug in super_diff here, if you want to report it.

I’m concerned about the approach in your PR. Changing public_similar_stories is going to introduce a bug where it’s called in app/view/stories/show.html.erb, it’ll show merged stories. I don’t know if there’s a test that’d catch this, there’s a lot of minor UI untested.

[myhro]

That’s a real painful test failure to debug. I didn’t see it when I read your test, but after I pulled it down and got the exception for myself I caught that the test says expect(response), not expect(response.body). Just one of those things that takes a fresh pair of eyes to see.

Oh my, thanks for catching that. It was going to take a while for me to figure that.

I ran it with rspec -b to see the full backtrace and whew, it’s real deep in the plumbing of super_diff, the filenames sort of imply it was trying to compare objects. I think you’ve found a bug in super_diff here, if you want to report it.

I was certainly not expecting a potential bug. Thanks for the tip in debugging it.

I’m concerned about the approach in your PR. Changing public_similar_stories is going to introduce a bug where it’s called in app/view/stories/show.html.erb, it’ll show merged stories. I don’t know if there’s a test that’d catch this, there’s a lot of minor UI untested.

After running a "find all references" for this method, I got tricked into believing it wasn't used anywhere else, but now I see it in app/view/stories/show.html.erb. Do you think adding a new public_similar_stories_merged version of this method would work? Or have you thought about a better approach?

[myhro]

Do you think adding a new public_similar_stories_merged version of this method would work? Or have you thought about a better approach?

I actually added a keyword merged parameter defaulting to false. Looks a bit more idiomatic.

[pushcx]

This adds too much complexity for this feature. I'm really suspicious of boolean arguments like you've added to public_similar_stories, this is two different methods smuggled in as one. I think the similar_stories method above it would already find the duplicate even if it's been merged.

[pushcx]

I've bumped brakeman on the master branch to fix the build, you can merge that in or just rebase your PR. Using --ensure-latest is new, we'll see if that's too much noise to be valuable.

[myhro]

I think the similar_stories method above it would already find the duplicate even if it's been merged.

I'm a bit confused about why the check was against story.public_similar_stories(@user) if the template was rendered with { similar: story.similar_stories }. Anyway, switching to the similar_stories method is indeed a simpler fix - just pushed this version.

I've bumped brakeman on the master branch to fix the build, you can merge that in or just rebase your PR. Using --ensure-latest is new, we'll see if that's too much noise to be valuable.

Thanks for the heads up. I've rebased the branch before updating the PR. CI checks are green now.

[myhro]

I've also added tests for the changes. Can you please take a look at it again?

[pushcx]

Looks great, thanks for working through a weird bug and my design concerns on this one!

[myhro]

You are welcome. Thanks for taking the time in reviewing and helping out to debug the test framework failures!

P.s.: any chance I can get a site invitation as a late Christmas gift? I've been lurking there for a few years already.

[pushcx]

Probably the check was against public_similar_stories because it was copied from app/views/stories/show.html.erb

I've sent an invite to your email, thanks again.

[myhro]

Awesome, thanks!