Refactor detector GraphQL and implement journal detection type #92
Conversation
Why these changes are being introduced: Since we created the `StandardIdentifiers` model, we started namespacing our detection models. Relevant ticket(s): Tangentially related to [TCO-50](https://mitlibraries.atlassian.net/browse/TCO-50) How this addresses that need: This adds the `Detectors` namespace to `StandardIdentifiers` to match our other detection models (`Journal` and `SuggestedResource`). Side effects of this change: Some cascading changes were made in Metrics::Algorithms and the SearchEventType GraphQL.
jazairi
force-pushed
the
tco-50-journal-detection-graphql
branch
from
b28f37a
to
5fd7b4d
Compare
jazairi
force-pushed
the
tco-50-journal-detection-graphql
branch
from
5fd7b4d
to
f0979b6
Compare
Why these changes are being introduced: We've implemented a journal detection model but not the corresponding GraphQL. As this will be the second of many detectors, it also makes sense to reconsider how the detector GraphQL is modeled. Relevant ticket(s): * [TCO-50](https://mitlibraries.atlassian.net/browse/TCO-50) How this addresses that need: This makes `journals` and `standard_identifiers` fields within a new `DetectorsType`. Much like the `Detector` module, this new type is not to any ActiveRecord models and acts more as a namespace to contain the various detectors. Side effects of this change: * The graphql-ruby docs [advise against overusing JSON](https://graphql-ruby.org/api-doc/2.3.14/GraphQL/Types/JSON.html) as a GraphQL type, but this feels like a good use case for it. * I'm not certain whether my solution to the `detectors` field is idiomatic. It feels particularly odd given that, in the SearchEventType, there is a method for the `phrase` field that returns the same thing. * The gitignore file has been updated to ignore Lando config. This ended up being unrelated to this changeset, but it might be useful during cassette regeneration.
jazairi
force-pushed
the
tco-50-journal-detection-graphql
branch
from
f0979b6
to
7083502
Compare
There was a problem hiding this comment.
This looks good to me. I confess that I'm still working through the best practices for designing GraphQL types, so I'm not sure whether there's things here that I'm missing, but seeing this implementation get much more standardized feels right. It also points to a place where maybe our model implementations might need more consistent approaches (using "new" in some cases while others have "full_term_match") - but this isn't the PR to worry about that.
Thanks for taking this on!
| field :standard_identifiers, [Types::StandardIdentifiersType], description: 'Currently supported: ISBN, ISSN, PMID, DOI' | ||
|
|
||
| def standard_identifiers | ||
| Detector::StandardIdentifiers.new(@object).identifiers.map do |identifier| |
There was a problem hiding this comment.
I know that this is correct, and I don't think that this work is where any changes should be made, but I'm starting to wonder whether a new method is the right thing to be calling in this type of context. Comparing Detector::StandardIdentifiers.new() here with Detector::Journal.full_term_match() below forces me to remember that our "new" doesn't actually create new records and pollute our dataset as a side effect.
There was a problem hiding this comment.
Yeah, that's a good point. Putting an initialize method on a non-ActiveRecord model is a bit confusing. Looking at the code, I think we did it that way to make use of an instance variable, but there could yet be a way to refactor it using singleton methods. I opened this spike ticket to investigate.
| { kind: identifier.first, value: identifier.last } | ||
| end | ||
| def detectors | ||
| @object.term.phrase |
There was a problem hiding this comment.
Looking at the set of changes, both here and within the Detectors type, I appreciate the work you've done to standardize how they're implemented.
This changeset refactors the GraphQL for detectors while adding a journal detector type. There is now a
DetectorsType, which has thestandardIdentifersandjournalsfields. Previously,standardIdentifiers(our only detector implemented as GraphQL) was a field onTermTypeandSearchEventType. This worked well for a single detector, but it will become unwieldy as we add more.This design also mirrors our models, which are namespaced in a
Detectormodule. To that point, a separate commit moves theStandardIdentifiersmodel, which was introduced before theDetectormodule, into that namespace.Please read the commit messages accompanying this PR, as there are some quirks and side effects that I noted there.
Developer
Ticket(s)
TCO-50
Accessibility
all issues introduced by these changes have been resolved or opened
as new issues (link to those issues in the Pull Request details above)
Documentation
ENV
Stakeholders
Dependencies and migrations
NO dependencies are updated
NO migrations are included
Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing