-
Notifications
You must be signed in to change notification settings - Fork 124
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
♻️ Remove lonely mixin #6636
♻️ Remove lonely mixin #6636
Conversation
The only place leveraging the `Hyrax::ResourceIndexer` mixin was the the `Hyrax::Indexers::ResourceIndexer`. Further we had a case where the Solr Document's `has_model_ssim` was trampled by the mixin's behavior. Both settings of the `has_model_ssim` likely set the same value, but did so using two different methods. And given that, my preference would be to favor a single dot `.` call (e.g. `resource.internal_resource`) instead of a double dot (e.g. `resource.class.name`).
@@ -73,7 +71,9 @@ def to_solr | |||
"date_modified_dtsi": resource.updated_at, | |||
"system_create_dtsi": resource.created_at, | |||
"system_modified_dtsi": resource.updated_at, | |||
"has_model_ssim": resource.internal_resource |
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.
This is a notable change; one that I'm working through in the double_combo
branch.
I think a data dictionary explaining the intention of to_rdf_representation
and internal_resource
would help clarify.
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.
What is meant by the following, and by implication the very important has_model_ssim
key:
resource.class.name
resource.class.internal_resource
resource.class.to_rdf_representation
The resource.class.name
is there for application logic; it’s uses are related to the Active::Model
conventions of a class’s name. Thus related to the Rails application.
The resource.class.internal_resource
is a construct of Hyrax, introduced as we moved into a Wings era. Providing a way to know a bit about the object’s history. I’ve seen internal_resource
values of Wings (GenericWork)
or GenericWork
or GenericWorkResource
. We derive this value from the resource.class.to_rdf_representation
(when present).
The resource.class.to_rdf_representation
appears to be the closest “naming” of the resource’s concept. That is we could have a resource stored in Fedora (via ActiveFedora), Fedora (via Valkyrie), Postgres (via Valkyrie), and Solr (via either Valkyrie or ActiveFedora). That resource’s RDF representation would be the same across those storage locations. The most important place to get this right is when we write to Solr; in part because Solr is non-canonical but also to allow our presentation logic to not concern itself with what strategy we used to persist the canonical object.
Ultimately, we need to work to disambiguate and describe these concepts within the Hyrax context. Explaining their usage so that we can connect into the correct location.
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.
Thanks for the research you've done here. This looks like a good change to me. 👍
The only place leveraging the
Hyrax::ResourceIndexer
mixin was the theHyrax::Indexers::ResourceIndexer
. Further we had a case where the Solr Document'shas_model_ssim
was trampled by the mixin's behavior.Both settings of the
has_model_ssim
likely set the same value, but did so using two different methods. And given that, my preference would be to favor a single dot.
call (e.g.resource.internal_resource
) instead of a double dot (e.g.resource.class.name
).@samvera/hyrax-code-reviewers