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

CV2-6038 refactor has claim search filter into has article filter #2205

Conversation

melsawy
Copy link
Contributor

@melsawy melsawy commented Feb 6, 2025

Description

Refactor has_claim filter to include both FactCheck & Explainer

  • Add a field for the explainer title to the ES doc
  • Add/update the explainer information to ElasticSearch when explainer is added/updated.
  • Remove the explainer information when an explainer is deleted or remove from an item.
  • Rename has_claim filter to has_article
  • Include explainer title in keyword search
  • Add rake task to migrate existing data

References: Cv2 6038

How has this been tested?

Implemented automated tests.

Checklist

  • I have performed a self-review of my own code
  • I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • I have added regression tests, if the PR fixes a bug
  • I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • I considered secure coding practices when writing this code. Any security concerns are noted above.
  • I have commented my code in hard-to-understand areas, if any
  • I have made needed changes to the README
  • My changes generate no new warnings
  • If I added a third party module, I included a rationale for doing so and followed our current guidelines

@melsawy melsawy changed the title Cv2 6038 refactor has claim search filter into has article filter CV2-6038 refactor has claim search filter into has article filter Feb 7, 2025
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the ES callbacks for the title and description since we have already created cached fields for both, and there is an ES callback to synchronize the values.

Comment on lines -40 to -45
def remove_fields_from_elasticsearch_doc(keys, pm_id)
return if self.disable_es_callbacks || RequestStore.store[:disable_es_callbacks]
options = { keys: keys, pm_id: pm_id }
model = { klass: self.class.name, id: self.id }
ElasticSearchWorker.perform_in(1.second, YAML::dump(model), YAML::dump(options), 'remove_fields')
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of removing the field from the ES field, I update the value to nil. In the end, both approaches are the same, but I prefer updating the value to nil to be more consistent with other callbacks.

@melsawy melsawy marked this pull request as ready for review February 9, 2025 20:08
Copy link

codeclimate bot commented Feb 9, 2025

Code Climate has analyzed commit 54a73cf and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Thanks @melsawy , much simpler version than before. I left a few comments but really only one is a blocker IMO.

@melsawy melsawy requested a review from caiosba February 10, 2025 20:33
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Thanks Sawy for applying the suggestions from the previous review!

@melsawy melsawy merged commit 0e95c0e into develop Feb 11, 2025
15 of 16 checks passed
@melsawy melsawy deleted the CV2-6038-refactor-has-claim-search-filter-into-has-article-filter branch February 11, 2025 05:10
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.

2 participants