Ensuring self-contained memoization of method (#725)

Prior to this commit, the guard condition `return self.collection_ids if
collections_created?` served as memoization (e.g. caching the results of
an expensive operation).  However, it was not a self-contained
memoization (e.g. the method is fully in charge of managing the
memoization).  It instead relied on the previous implementation of
`#collections_created?`.

Downstream implementations may change `#collection_created?` which could
create unintended consenquences of assignign collections based on the
OAI spec.  Also, if we look to other implementations in Bulkrax of
`#collections_created?` we see that many simply `return true`.

The impact of the previous code is that were someone to make the above
change (e.g. always `return true`), we end up never assigning any
`setSpec` `collection_ids` and always returning `[]` for
`#find_collection_ids`.  dependent on external methods.

With this commit, we are internalizing the memoization mechanism so that
this method is less dependent on external factories.

For further discussion of the interplay of the methods regarding
collection creation see the [comments found here][1].

References:

- scientist-softserv/adventist-dl#247
- https://github.com/scientist-softserv/adventist-dl/issues/240

[1]: https://github.com/scientist-softserv/adventist-dl/issues/240#issuecomment-1412456590

app/models/bulkrax/oai_entry.rb

@@ -91,7 +91,8 @@ def add_thumbnail_url
     # If OAI-PMH doesn't return setSpec in the headers for GetRecord, use parser.collection_name
     #   in this case, if 'All' is selected, records will not be added to a collection.
     def find_collection_ids
-      return self.collection_ids if collections_created?
+      return self.collection_ids if defined?(@called_find_collection_ids)
+
       if sets.blank? || parser.collection_name != 'all'
         collection = find_collection(importerexporter.unique_collection_identifier(parser.collection_name))
         self.collection_ids << collection.id if collection.present? && !self.collection_ids.include?(collection.id)
@@ -101,6 +102,8 @@ def find_collection_ids
           self.collection_ids << c.id if c.present? && !self.collection_ids.include?(c.id)
         end
       end
+
+      @called_find_collection_ids = true
       return self.collection_ids
     end
   end

spec/models/bulkrax/oai_entry_spec.rb

@@ -98,6 +98,11 @@ class ::Avacado < Work
       # rubocop:enable RSpec/VerifiedDoubles
 
       it 'derives the factory class before proceeding with adding other metadata' do
+        # In changing the memoization mechanism, I was encountering a case where the application was
+        # attempting to call SOLR.  Something previous not required due to implementation details.
+        # As we're not testing collection id related things, this "hack" seems acceptable.
+        entry.instance_variable_set("@called_find_collection_ids", true)
+
         # TODO: Not a big fan of this method chain antics.  Ideally we'd pass in the object at
         # instantiation time.  However I'm cribbing past work and trying to get this fix out into
         # the wild.
@@ -128,6 +133,11 @@ class ::Avacado < Work
       end
 
       it 'adds admin set id to parsed metadata' do
+        # In changing the memoization mechanism, I was encountering a case where the application was
+        # attempting to call SOLR.  Something previous not required due to implementation details.
+        # As we're not testing collection id related things, this "hack" seems acceptable.
+        entry.instance_variable_set("@called_find_collection_ids", true)
+
         allow(entry).to receive_message_chain(:record, :header, :identifier).and_return("some_identifier")
         allow(entry).to receive_message_chain(:record, :header, :set_spec).and_return([])
         allow(entry).to receive_message_chain(:record, :metadata, :children).and_return([])