Skip to content
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

Extend Metrics::Algorithms feature to include Detector::SuggestedResource matches #84

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Aug 13, 2024

This extends the Metrics::Algorithms model which generates historical records of the application's performance to include matches on the SuggestedResources records that have been added recently. A part of this work is that the Detector::SuggestedResource model now gets a full_term_match method, which is the crucial link to allow the model to do anything about search traffic.

Along the way the calculate_fingerprint method becomes a class method, rather than an instance method. It is still part of the SuggestedResource model, and not in a helper.

Much of this approach has been copied from the Journal title matching work - which seems like a reasonable starting point, both because the uses are similar and because consistent approaches in the app feels like Doing This Right.

Developer

Ticket(s)

https://mitlibraries.atlassian.net/browse/TCO-25

Accessibility

  • ANDI or Wave has been run in accordance to our guide and
    all issues introduced by these changes have been resolved or opened
    as new issues (link to those issues in the Pull Request details above)
  • There are no accessibility implications to this change

Documentation

  • Project documentation has been updated, and yard output previewed
  • No documentation changes are needed

ENV

  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.

Stakeholders

  • Stakeholder approval has been confirmed
  • Stakeholder approval is not needed

Dependencies and migrations

NO dependencies are updated

YES migrations are included

Reviewer

Code

  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.

Documentation

  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.

Testing

  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-84 August 13, 2024 18:57 Inactive
@matt-bernhardt matt-bernhardt force-pushed the tco-25-historical-hints branch from 136efc8 to a7ddfd7 Compare August 13, 2024 20:17
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-84 August 13, 2024 20:17 Inactive
@matt-bernhardt matt-bernhardt force-pushed the tco-25-historical-hints branch from a7ddfd7 to 3614dac Compare August 13, 2024 20:18
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-84 August 13, 2024 20:18 Inactive
@matt-bernhardt matt-bernhardt changed the title Work in progress to include SuggestedResource matches in metrics Extend Metrics::Algorithms feature to include Detector::SuggestedResource matches Aug 14, 2024
@matt-bernhardt matt-bernhardt marked this pull request as ready for review August 14, 2024 13:59
@matt-bernhardt matt-bernhardt requested review from JPrevost and jazairi and removed request for JPrevost August 14, 2024 14:00
@JPrevost JPrevost self-assigned this Aug 14, 2024
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Please add the method documentation we agreed to experiment with on this repository.

app/models/detector/suggested_resource.rb Show resolved Hide resolved
app/models/metrics/algorithms.rb Show resolved Hide resolved
@matt-bernhardt
Copy link
Member Author

Oh, dagnabit. Yeah, I'll add that. Thanks for the prod.

@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-84 August 14, 2024 18:18 Inactive
** Why are these changes being introduced:

* We need to include the performance of our SuggestedResource records in
  the historical performance reports we are generating.

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/tco-25

** How does this address that need:

* This copies the approach for counting Journal matches, applying it to
  the SuggestedResource model as well. This includes a migration to add
  a field to the report model, and tests and fixtures to confirm that
  the counts are being generated correctly.

** Document any side effects to this change:

* Maybe not a side effect (but maybe it is) - the SuggestedResource model
  did not have a full_term_match method before this PR. This work adds it
  as well as tests for its behavior.
@matt-bernhardt matt-bernhardt force-pushed the tco-25-historical-hints branch from e151b6b to d0dbd12 Compare August 14, 2024 19:23
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-84 August 14, 2024 19:23 Inactive
@matt-bernhardt matt-bernhardt merged commit 7476eeb into main Aug 14, 2024
3 checks passed
@matt-bernhardt matt-bernhardt deleted the tco-25-historical-hints branch August 14, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants