-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
i149 Fix editing Contributions on Asset edit form #944
base: develop
Are you sure you want to change the base?
i149 Fix editing Contributions on Asset edit form #944
Conversation
They will now render alongside Contributions under the Credits section
Properly edit Contribution and ContributionResource records from the Asset form in the way each model expects. Also, properly create new ContributionResource records while ensuring blank form fields don't get saved to the database
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comment re: return values from Hyrax.query_service
.
I'm also thinking part of the Contribution buggy behavior may be due to the same problem we were having with instantiations, where they weren't getting removed from Fedora after being saved in Postgres, and then deleting the Postgres copy would cause the Valkyrie Resource (e.g. ContributionResource
or PhysicalInstantiationResource
) to fall back to Fedora when looking up the object, and upon finding it, uses a dynamic class that inherits from teh Valkyrie class... at. least I think that's what's going on.
but if it was causing an issue with instantiations, it stands to reason it will cause the same issue with Contributions and EssenceTracks as well.
If this is a consistent issue across all these models, it would be a good idea to apply a consistent method to handle it.
to_destroy = ActiveModel::Type::Boolean.new.cast(param_contributor['_destroy']) | ||
if to_destroy | ||
destroys << param_contributor[:id] | ||
next | ||
end | ||
|
||
if param_contributor[:id].present? | ||
contributor = Hyrax.query_service.find_by(id: param_contributor[:id]) | ||
if contributor.is_a?(Contribution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think contributor.is_a?(Contribution)
will ever be TRUE
because I think Hyrax.query_service.find_by
will always return a ContributionResource
, even if it's pulling the record from Fedora.
Here's a test I did just now on Demo:
# RSolr client
solr = Blacklight.default_index.connection
# Grab some Solr documents that still have regular "Contribution" for has_model_ssim, indicating
# it was saved to Fedora using the Contribution model (subclass of ActiveFedora::Base).
docs = solr.get('select', params: {q: 'has_model_ssim:Contribution', rows: 100} )['response']['docs']
# Map the Solr Document IDs to ActiveFedora Contribution models (or exceptions if they don't exist).
contributions_from_af = docs.map do |doc|
begin
Contribution.find(doc['id'])
rescue => e
e
end
end
# Map the Solr Document IDs to ContributionResource using the Hyrax.query_service (or exceptions if they don't exist).
contributions_from_query_service = docs.map do |doc|
begin
Hyrax.query_service.find_by(id: doc['id'])
rescue => e
e
end
end
# Print out the class of each object and whether it is recognized as a Contribution.
contributions_from_af.map { |c| {class: c.class, is_a_contribution: c.is_a?(Contribution)} }
# =>
# [{:class=>Contribution, :is_a_contribution=>true},
# {:class=>Contribution, :is_a_contribution=>true},
# {:class=>Contribution, :is_a_contribution=>true},
# ...
# Print out the class of each object, and whether it is recognized as a Contribution, or a ContributionResource.
contributions_from_query_service.map{ |c| {class: c.class, is_a_contribution: c.is_a?(Contribution), is_a_contribution_resource: c.is_a?(ContributionResource)} }
# =>
# [{:class=>#<Class:0x00007f64cd6f1240>, :is_a_contribution=>false, :is_a_contribution_resource=>true},
# {:class=>#<Class:0x00007f64cd6f1240>, :is_a_contribution=>false, :is_a_contribution_resource=>true},
# {:class=>#<Class:0x00007f64cd6f1240>, :is_a_contribution=>false, :is_a_contribution_resource=>true},
# ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afred this might not apply to AMS, but in other apps I've worked on, has_model_ssim
will be Contribution
regardless of whether it's in Fedora or Valkyrie. Can you confirm whether or not this is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also thinking part of the Contribution buggy behavior may be due to the same problem we were having with instantiations, where they weren't getting removed from Fedora after being saved in Postgres, and then deleting the Postgres copy would cause the Valkyrie Resource (e.g. ContributionResource or PhysicalInstantiationResource) to fall back to Fedora when looking up the object, and upon finding it, uses a dynamic class that inherits from teh Valkyrie class... at. least I think that's what's going on.
but if it was causing an issue with instantiations, it stands to reason it will cause the same issue with Contributions and EssenceTracks as well.
If this is a consistent issue across all these models, it would be a good idea to apply a consistent method to handle it.
I agree that that is definitely possible and that a consistent fix would be ideal. That being said, I chose to make the changes here for now because:
- The method of attaching Contributors to Assets is custom behavior; it does not follow the usual way that Hyrax associates records with each other
- I'm not sure if instantiations do or not, I haven't gotten that far yet
- This PR is the first step towards a solution. I wanted to get Contributors working end-to-end first before making changes to the other models
Ref:
This is the first step to resolving #149. More changes will be required to completely resolve the issue.
Changes:
Not addressed:
Untested: