samvera / hyrax

Hyrax is a Ruby on Rails Engine built by the Samvera community. Hyrax provides a foundation for creating many different digital repository applications.
http://hyrax.samvera.org/
Apache License 2.0
184 stars 124 forks source link

WINGS: Valkyrie resource created from ActiveFedora objects respond to reload #3928

Closed elrayle closed 5 years ago

elrayle commented 5 years ago

Descriptive summary

Any Valkyrie Resource transformed from an ActiveFedora object should not respond to :reload method.

Rationale

Valkyrie Resources do not have a concept of reloading a resource.

Expected behavior

resource = af_object.valkyrie_resource
resource.reload # should throw missing method

All active fedora methods are currently supported by a pass through to the active fedora version of the resource by catching missing method. This happens in lib/wings/hydra/pcdm/models/concerns/pcdm_valkyrie_behavior.rb #method_missing.

Update this method to check specifically for the :reload method and continue to throw method missing instead of forwarding the call to the active fedora object.

REQUIRES EXPLORATION

The reload method for active fedora objects is used in app code and in tests. Exploration is needed to determine if these reloads can go away when they are switched to Valkyrie::Resources. If this functionality is still needed, then implement a Hyrax reload method for resources. Something like...

module Hyrax
  ...

  def self.reload_resource(resource:)
    query_service = Hyrax.query_service
    query_service.find_by(id: resource.id)
  end

During the transition time from AF to Valkyrie, a second method could be created to handle both resources and af_objects.

module Hyrax
  ...

  def self.reload_object(:object)
    return object.reload if object.is_a? ActiveFedora::Base
    return reload_resource(resource: object) if object.is_a? Valkyrie::Resource
    # possibly throw method missing if object isn't an AF:Base or Valkyrie:Resource
  end

This would allow us to encapsulate the reload functionality making it easier to remove later if needed.

Actual behavior

resource = af_object.valkyrie_resource
resource.reload # returns active fedora object

Related work

Link to related tickets or prior related work here.

no-reply commented 5 years ago

This seems right to me.

There are a number of pass-through methods in Wings' handling that we'll want to start to take away now that Hyrax::Resource exists. In general, if we need utilities to support processes that used to be methods on AF::Base, implementing them as Valkyrie-native.

In this case, I'm not sure I see the value in a reload_object service that handles multiple inputs. It seems like keeping object.reload calls inline where they exist, until they can be replaced with resource = Hyrax.query_service.find_by(id: resource.id) (or removed entirely) seems like the best approach.

no-reply commented 5 years ago

3952 tries to fix this by removing method_missing entirely. I think this is the best approach, but there is some code relying on it already, so a solution to that will need to be found. See the failing tests on that PR.