Fix `nil?`checks for associated records: check the ID only

[nimmolo]

Housekeeping PR that came up when examining very old code involved with the feature I'm working on, auto-suggesting locations.

Even though it's convenient and looks more elegant, it's inefficient to nil-check an associated record directly, unless it's been eager loaded — apparently, it will instantiate the record.

Bad habit:

@observation.location.nil?

Good habit:

@observation.location_id.nil?

[nimmolo]

Also - heads up, very unexpected behavior with fixtures: Giving a fixture a nil association apparently creates a phantom fixture ID for the nil record — but no fixture.

Example:

# includes observations that violate date constraints
# members: roy
nowhere_2023_09_project:
  <<: *DEFAULTS
  user: dick
  admin_group: dick_only
  user_group: falmouth_2023_09_users
  start_date: 2023-09-01
  end_date: 2023-09-30
  location: nil
  observations: brett_woods_2023_09_obs, falmouth_2023_09_obs, falmouth_2022_obs, nybg_2023_09_obs # roy (non-admin) owns nybg_2023_09_obs

project = projects(:nowhere_2023_09_project)
debugger

(rdbg) project.location
nil
(rdbg) project.location_id
221495974
(rdbg) Location.find(221495974)
eval error: Couldn't find Location with 'id'=221495974
  /Users/nimmo/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/activerecord-7.1.3.4/lib/active_record/core.rb:253:in `find'
  (rdbg)//Users/nimmo/Documents/GitHub/developer-startup/mushroom-observer/app/helpers/projects_helper.rb:1:in `styled_obs_lat'
nil

In this case, the default project has no associated location, so it's not necessary to set location: nil, i just commented it out.

[coveralls]

coverage: 94.393%. remained the same when pulling b5d0f23083e75e9dbf271d0a880792e33969c4e0 on nimmo-check-association-id-existence into 086ae300e348a2f5de869ab0e0f705047f159814 on main.

[JoeCohen]

@nimmolo: Thanks for your work on this, and for alerting us to the inefficiencies and the fixture issue. Some questions/comments:

• Should these be reported to the Rails team:
  • The unexpected fixture behavior seems like a bug. It's a least a trap for the unwary that should be documented.
  • Instantiation of the record seems like at least an opportunity to speed up Rails, if not a bug.
• Should we update other fixture sets to common out unnecessary nil associations? I sometimes add an explicit nil to indicate that I don't want the association changed, or to override a default fixture value. If I'm using explicit fixture nils, it seems like I must nil-check the associated record directly (because <association>.id.nil? will get the wrong result in tests).
• Seems like sometimes I must do what you describe as a bad habit. For me it's complicated and confusing to figure out when I should nil-check the id vs. when I should (must) nil-check the association. As a result, I'll probably always nil-check the association directly; I'd rather be slower than buggy.

[nimmolo]

@JoeCohen Good call, I think the fixture behavior is a reportable bug.

I do think it might be wise to comment out the explicit nil associations in the fixtures, except where they are overriding a default. But it doesn't seem like there should be default associations, either, maybe. I'm not familiar enough with the fixtures, though.

Maybe in the few cases where we do want to override a default association, we can instead just add the non-nils where appropriate. I'm not sure there's ever really an appropriate use of object.association.nil? outside of this fixture issue, although i might have missed a case. It seems better to have better code and slightly more verbose fixtures than the opposite.

For example with an Observation: either it has a location_id or it doesn't; the location_id is a property of the Observation record strictu sensu. But an observation.location is always a derived thing, that comes from Rails instantiating Location.safe_find(observation.location_id). It's not verifiable independent of the id, and in fact the Location record that we're trying to verify "is no longer associated with the Observation" may still exist... it's just that the Observation isn't tracking it any more.