dependent option is ignored when using :through option

[atz]

See: https://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_one

Closes #1014

[coveralls]

Coverage decreased (-0.002%) to 97.727% when pulling 15a9cfc on dependent_through into 9ed2b70 on master.

[jmartin-sul]

rubocop.

one thing i just noticed in the doc you linked:

This method should only be used if the other class contains the foreign key.

which is not the case here.

IIRC, there's only one scope that uses that has_one as a convenience. maybe it wouldn't be so bad to re-work it to use joins explicitly? this thing, i think?

preservation_catalog/app/models/zip_endpoint.rb

Lines 24 to 31 in 9ed2b70

	# for a given druid, which zip endpoints have an archive copy of the given version?
	scope :which_have_archive_copy, lambda { |druid, version|
	joins(zipped_moab_versions: [:preserved_object])
	.where(
	preserved_objects: { druid: druid },
	zipped_moab_versions: { version: version }
	)
	}

we'd have to join through to complete_moabs explicitly. i don't think anything else needs that has_through (unless someone else has written a query that takes advantage of it since i added it).

maybe we should just punt on this for now and ticket the other thing and get rid of this has_one as part of that? status quo seems harmless enough for the moment. i'd be hesitant to add a rubocop exception for a thing we're technically mis-using.

[atz]

Rubocop flagging this is a bug in the old version we have:
rubocop/rubocop#4751

Unfortunately, after updating to a more recent version, we go from checking 122 files to 7. So that is lame. Include and Exclude are, uhhh, exclusive options now, apparently:

rubocop -L
config.ru
Rakefile
lib/tasks/resque.rake
lib/tasks/m2c_tasks.rake
lib/tasks/c2m_tasks.rake
lib/tasks/cv_tasks.rake
lib/capistrano/tasks/capistrano-resque-pool.rake

[atz]

@jmartin-sul we looked at that when we first stumbled on the docs. The other class does contain the foreign key, for the thing we are associating to. For this, it isn't important that ZippedMoabVersion has the FK to CompleteMoab, because that is already established by belongs_to :complete_moab. It is important that this class does not have the FK to PreservedObject. I was confused by this initially too, but it makes sense to me now.

Updated our rubocop and rubocop-rspec versions and got it to be cool w/ the 1-line change. As a side-benefit, a bunch of extra whitespace cleanup.

[jmartin-sul]

@atz: ah, ok, i misread the docs as implying that the FK chain should point back to the thing that has_one (i.e., in my possible misunderstanding, i read the docs as saying that the FKs in this case should be PreservedObject -> CompleteMoab -> ZippedMoabVersion, which is the opposite direction of what things are).

and that made sense to me, because the has_one usually seems to imply FK relationship back to the "haver", which also seems to fit with calling the thing that is "had" a dependent.

but if that's not the case, i retract my above comment.

i dunno, the language feels ambiguous to me. you've got more AR experience than me, so i'd trust your judgement on this.

[jmartin-sul]

anyway, regardless of whether we should be using has_one, the dependent option gets ignored, and the other style clean up looks good, so this seems like an improvement regardless of whether i'm understanding the other has_one guidance correctly. merged.