-
Notifications
You must be signed in to change notification settings - Fork 11
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
CV2-6038 refactor has claim search filter into has article filter #2205
Conversation
…-into-has-article-filter
…-into-has-article-filter
app/models/annotations/embed.rb
Outdated
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.
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.
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 |
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.
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.
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. |
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 @melsawy , much simpler version than before. I left a few comments but really only one is a blocker IMO.
…-into-has-article-filter
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 Sawy for applying the suggestions from the previous review!
Description
Refactor has_claim filter to include both FactCheck & Explainer
has_claim
filter tohas_article
References: Cv2 6038
How has this been tested?
Implemented automated tests.
Checklist