Set a @versions ivar and pass to helpers to avoid duplicate slow lookups of obj.versions

[nimmolo]

The PR currently deletes a lookup of the user instance when building a user_link for the latest version's user, just taking the version.user_id without confirming the user still exists.

There's a chance this could give a dead link, but it seems remote. To me page slow loads are the bigger issue.

versions should be eager-loaded, so this PR also sets strict_loading on the main show query for all the controllers here. strict_loading suggested eager-loading many more associations, added here.

[coveralls]

coverage: 94.576% (-0.007%) from 94.583%
when pulling 29929bb on nimmo-versions-ivar
into 41d7a49 on main.

[nimmolo]

@JoeCohen - This test is failing, and I believe it should not.

As far as I understand, setting term.stub(:destroy, false) should make the term not destroyable in the test.

However, if the @glossary_term is eager-loaded with all its associations, the term is deleted when this action is called in the test.

What do you think is going on here?

  def test_destroy_fails
    term = glossary_terms(:no_images_glossary_term)

    login
    make_admin
    term.stub(:destroy, false) do
      GlossaryTerm.stub(:safe_find, term) do
        delete(:destroy, params: { id: term.id })
      end
    end

    assert_redirected_to(glossary_term_path(term.id),
                         "It should redisplay a Term it fails to destroy")
  end

[JoeCohen]

@nimmolo: I'll have a look at test_destroy_fails now.

[JoeCohen]

@nimmolo: Does test_destroy_fails fail locally for you? (Not for me.)

[nimmolo]

@JoeCohen Yes, fails both for me locally and on CI.

Check to be sure you're on this branch and running this test:
GlossaryTermsControllerTest#test_destroy_fails

I can make the test pass by swapping the commented lines for the current lines in GlossaryTermsController#find_glossary_term!, but that defeats the point of the eager-loading.

  def find_glossary_term!
    # @glossary_term = find_or_goto_index(GlossaryTerm,
    #                                     params[:id].to_s)
    @glossary_term = GlossaryTerm.includes(show_includes).strict_loading.
                     find_by(id: params[:id]) ||
                     flash_error_and_goto_index(GlossaryTerm, params[:id])
  end

[JoeCohen]

It's passing locally for me:

joe@Josephs-MacBook-Pro ~/mushroom-observer (nimmo-panels-show-name)$ git status
On branch nimmo-panels-show-name
Your branch is up to date with 'origin/nimmo-panels-show-name'.

nothing to commit, working tree clean
joe@Josephs-MacBook-Pro ~/mushroom-observer (nimmo-panels-show-name)$ rails t test/controllers/glossary_terms_controller_test.rb -n test_destroy_fails
Started with run options -n test_destroy_fails --seed 1755
...
1 tests, 5 assertions, 0 failures, 0 errors, 0 skips
...
joe@Josephs-MacBook-Pro ~/mushroom-observer (nimmo-panels-show-name)$ rails t 
Started with run options --seed 50929
...
2327 tests, 27078 assertions, 0 failures, 0 errors, 0 skips

[JoeCohen]

Sorry. Never moind. I'm in the wrong branch. Will keep looking.

[JoeCohen]

Looks to me like the stub works correctly. If I put a breakpoint in the test at the failing assertion, then ask if the term is destroyed, it returns false.

joe@Josephs-MacBook-Pro ~/mushroom-observer (nimmo-versions-ivar)$ rails t test/controllers/glossary_terms_controller_test.rb -n test_destroy_fails
...
   449|       GlossaryTerm.stub(:safe_find, term) do
   450|         delete(:destroy, params: { id: term.id })
   451|       end
   452|     end
   453| 
=> 454|     debugger
   455|     assert_redirected_to(glossary_term_path(term.id),
   456|                          "It should redisplay a Term it fails to destroy")
   457|   end
   458|   # ---------- helpers ---------------------------------------------------------
=>#0    GlossaryTermsControllerTest#test_destroy_fails at ~/mushroom-observer/test/controllers/glossary_terms_controller_test.rb:454
  #1    block in run (3 levels) at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/minitest-5.20.0/lib/minitest/test.rb:94
  # and 25 frames (use `bt' command for all frames)
(ruby) term.destroyed?
false

The implication is that the test is working and there's an issue in the controller.

[JoeCohen]

The test's inner stub GlossaryTerm.stub(:safe_find, term) stubs safe_find to return term,
which had previously been stubbed by term.stub(:destroy, false)

• In the old controller, find_glossary_term! called find_or_goto_index which called safe_find.
  So in the old controller, find_glossary_term! returned the stubbed term,
  When destroy was called on the stubbed term it returned false
• I think that in the new controller find_glossary_term! never hits safe_find.
  Therefore the new controller operates with an un-stubbed term.
  Therefore term.destroy destroys term.

Bottom line:
I don't know what to do. I can't figure out how to re-write the inner stub. I will give it some thought.
If worse comes to worse, we can replace the two stubs with stub_any_instance, but it would be nice to avoid that, as its not thread safe.

[nimmolo]

@JoeCohen - Thanks - this is great info! I had no idea about safe_find. I think i can fix... will report back shortly

[nimmolo]

Easy peasy. I just swapped find_by for safe_find and it works. Will now restore safe_find other places where i got rid of it.