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.

Sign up for GitHub

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

Fixed cache clearing for bulk-accepted suggestions. #2233

Merged
merged 1 commit into from
Feb 24, 2025
Merged

Fixed cache clearing for bulk-accepted suggestions. #2233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions app/models/concerns/relationship_bulk.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ def bulk_update(ids, updates, team)
end
relationships = Relationship.where(id: ids, source_id: source_id)
relationships.update_all(update_columns)
delete_cached_field(pm_source.id, ids)
Copy link
Contributor

Choose a reason for hiding this comment

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

@caiosba: before we were passing the relationship ids instead of the target project medias id, but we want to delete_cached_field for source and target project medias, correct?

Could we go through this file in our 1:1? I have other questions, like: why did you change the delete_cached_field method, and sort of related, why on like 31 we pass pm_source.id instead of source_id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's go through it in our 1:1, but I'm going to answer here for documentation as well.

There was a bug: We wanted to clear the cache for all project media IDs (source and targets, a.k.a. parent and children), but by mistake, the relationship IDs were being passed.

There is no difference between source_id and pm.source_id, it was only Sawy's preference :)

I took the opportunity to simplify, improve and refactor some things:

  • Rename the method, since we're clearing multiple cached fields instead of only one
  • Use the built-in method to clear all cached fields instead of manually clearing a hard-coded list of fields, because the previous implementation had knowledge it shouldn't have about how the cached fields work internally (like, the cache key format and which fields are affected by relationship). Also, it would miss it if we added a new field (which happened here - it was missing the media_cluster_origin field, that was added after this implementation

# Run callbacks in background
extra_options = {
team_id: team&.id,
Expand All @@ -29,6 +28,7 @@ def bulk_update(ids, updates, team)
action: updates[:action]
}
self.delay.run_update_callbacks(ids.to_json, extra_options.to_json)
delete_cached_fields(pm_source.id, relationships.map(&:target_id))
{ source_project_media: pm_source }
end
end
Expand All @@ -41,7 +41,7 @@ def bulk_destroy(ids, updates, team)
relationships.find_each{ |r| relationship_target[r.id] = r.target_id}
relationships.delete_all
target_ids = relationship_target.values
delete_cached_field(pm_source.id, target_ids)
delete_cached_fields(pm_source.id, target_ids)
# Run callbacks in background
extra_options = {
team_id: team&.id,
Expand All @@ -52,17 +52,9 @@ def bulk_destroy(ids, updates, team)
{ source_project_media: pm_source }
end

def delete_cached_field(source_id, target_ids)
# Clear cached fields
# List fields with `model: Relationship`
cached_fields = [
'is_suggested', 'is_confirmed', 'linked_items_count', 'suggestions_count','report_status','related_count',
'demand', 'last_seen', 'sources_as_sentence', 'added_as_similar_by_name', 'confirmed_as_similar_by_name'
]
cached_fields.each do |name|
Rails.cache.delete("check_cached_field:ProjectMedia:#{source_id}:#{name}")
target_ids.each { |id| Rails.cache.delete("check_cached_field:ProjectMedia:#{id}:#{name}") }
end
def delete_cached_fields(source_id, target_ids)
ids = [source_id, target_ids].flatten
ProjectMedia.where(id: ids).each { |pm| pm.clear_cached_fields }
end

def run_update_callbacks(ids_json, extra_options_json)
Expand Down
19 changes: 19 additions & 0 deletions test/models/relationship_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,4 +428,23 @@ def setup
assert_equal 4, Relationship.where(source_id: pm1.id, relationship_type: Relationship.suggested_type).count
assert_equal 0, Relationship.where(source_id: pm2.id, relationship_type: Relationship.suggested_type).count
end

test "should update media origin when bulk-accepting suggestion" do
Sidekiq::Testing.inline!
RequestStore.store[:skip_cached_field_update] = false
u = create_user is_admin: true

# Create suggestion
t = create_team
pm1 = create_project_media team: t
pm2 = create_project_media team: t
r = create_relationship source_id: pm1.id, target_id: pm2.id, relationship_type: Relationship.suggested_type
assert_equal CheckMediaClusterOrigins::OriginCodes::AUTO_MATCHED, pm2.media_cluster_origin

# Accept suggestion
with_current_user_and_team u, t do
Relationship.bulk_update([r.id], { source_id: pm1.id, action: 'accept' }, t)
end
assert_equal CheckMediaClusterOrigins::OriginCodes::USER_MATCHED, pm2.media_cluster_origin
end
end
Loading