Implement Detector, Category, DetectorCategory #101
Conversation
matt-bernhardt
force-pushed
the
tco-82-knowledge-graph
branch
from
9c3bb23
to
aa9d4a0
Compare
matt-bernhardt
force-pushed
the
tco-82-knowledge-graph
branch
from
aa9d4a0
to
c9a372b
Compare
matt-bernhardt
force-pushed
the
tco-82-knowledge-graph
branch
from
fce4417
to
b3f3b6e
Compare
** Why are these changes being introduced: Our chosen architecture calls for a set of models that will comprise a sort of "knowledge graph", which TACOS will consult during the categorization process. This includes classes for Category, Detector, and a linking DetectorCategory class. The Detector and DetectorCategory classes will each define a confidence value. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/tco-82 ** How does this address that need: This defines those classes. The migration includes the creation of the needed records for each class: Three category records, six detectors, and five records which link between them. There are currently no detectors which map to two of the categories, although we have talked about those being needed. Additionally, one detector, SuggestedResource, is unique in that specific records will count toward each category - so it isn't appropriate to have a link record which uniformly connects to only one category. ** Document any side effects to this change: We previously had a Detector model file, but it was set as a Module in order to provide a namespace for its subclasses. This has been updated to be just a class, which impacts the dashboard and test files. Also, there was a method to define a table name prefix, which has been moved from the Detector file to the subclass files.
matt-bernhardt
force-pushed
the
tco-82-knowledge-graph
branch
from
b3f3b6e
to
57b3959
Compare
|
Quick postscript about this PR - Please check the structure of the migration, as I'm not sure I got this right. With uniqueness constraints on the database tables, and sample records in the migration, I may have created a PR where the rollback doesn't happen cleanly. I've tried working with I'm happy to have guidance or suggestions for changes here. |
There was a problem hiding this comment.
I need to pivot to something else and will finish the review tomorrow, but wanted to submit my initial input even if it isn't complete. Overall this looks like what we want.
I definitely think the db/seed.rb is the better way to handle the data creation you have in the migration (whether we seed it in prod or manually create it is tbd, but it'll be great to have that seed to auto run them in PR builds and locally whenever we find a need).
|
Thanks - let me look over these suggestions and make some changes. I'll un-tag for a review now, and re-tag when I'm done. |
- Move the record population out of migrations and into db/seeds.rb - Splits the migrations into three separate files, one per model - Add two indices on the linking model, from each direction - Removes the confidence value from the Detector model, leaving only the one on the DetectorCategory linking model
|
Okay, @JPrevost - I've made the changes you mentioned in your initial review. It should be ready for you to finish looking at now. |
| @@ -7,3 +7,56 @@ | |||
| # ["Action", "Comedy", "Drama", "Horror"].each do |genre_name| | |||
| # MovieGenre.find_or_create_by!(name: genre_name) | |||
| # end | |||
|
|
|||
There was a problem hiding this comment.
Note: If these don't run automatically in our PR builds (and maybe other Heroku tiers? We can talk about that) they can be configured to do that. I haven't checked if this repo is already set to run seeds in Heroku or not, just noting it before I forget as something we can address if it doesn't work as desired.
This adds models for Detector, Category, and the linking DetectorCategory. There are uniqueness and dependency constraints specified in the migration and class files, and confirmed via tests.
I did not write tests for the linking class, as I'm not sure there's a need? I'm happy to write these if desired, though.
The fixtures for these classes match what we've already implemented in code, with the thought that these should change slowly enough that we should probably keep the fixtures in sync? I'm not sure about this though.
The biggest side effect is that implementing the Detector class is done by changing the existing module to a class - which forces changes in a handful of other files.
Developer
Ticket(s)
https://mitlibraries.atlassian.net/browse/TCO-82
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
YES migrations are included Adding three models with associated records
Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing