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

feat: DAH-2556 check translations cache LastModifiedDate #2283

Merged
merged 15 commits into from
Sep 17, 2024
Merged

feat: DAH-2556 check translations cache LastModifiedDate #2283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
56f4e55
feat: check translations cache LastModifiedDate
cade-exygy Sep 6, 2024
58bb1d8
Merge branch 'main' into DAH-2556-handle-outdated-translation-cache
cade-exygy Sep 8, 2024
683013d
feat: handle outdated translations and outdated listing cases
cade-exygy Sep 10, 2024
c6e4a35
fix: invalidate translation cache
cade-exygy Sep 11, 2024
7b544e3
Merge branch 'main' into DAH-2556-handle-outdated-translation-cache
cade-exygy Sep 11, 2024
a35e79e
fix: npm issue
cade-exygy Sep 11, 2024
7ff8e29
Merge branch 'main' into DAH-2556-handle-outdated-translation-cache
cade-exygy Sep 12, 2024
50b58a1
fix: move feature flag check up a level
cade-exygy Sep 12, 2024
4ca33bd
fix: flag check
cade-exygy Sep 12, 2024
d0a92c9
Merge branch 'main' into DAH-2556-handle-outdated-translation-cache
cade-exygy Sep 12, 2024
d4934e4
Merge branch 'main' into DAH-2556-handle-outdated-translation-cache
cade-exygy Sep 16, 2024
86a563e
test: cover cache invalidation checks
cade-exygy Sep 17, 2024
e0eeaba
Merge branch 'main' into DAH-2556-handle-outdated-translation-cache
cade-exygy Sep 17, 2024
f0769d7
fix: use each
cade-exygy Sep 17, 2024
32434fb
fix: translations object for test
cade-exygy Sep 17, 2024
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
1 change: 1 addition & 0 deletions app/services/cache_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def process_translations(listing)
ServiceHelper.convert_to_salesforce_field_name(key)
end,
translations,
listing['LastModifiedDate'],
)
end

Expand Down
1 change: 1 addition & 0 deletions app/services/force/event_subscriber_translate_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ def translate_and_cache(event)
event.listing_id,
event.updated_values.keys,
translations,
event.last_modified_date,
)
logger(
"Event Translations: #{cached_response.inspect}",
Expand Down
8 changes: 6 additions & 2 deletions app/services/force/listing_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ def self.listing(id, opts = {})
{}
end

unless translations_valid?(listing_translations)
unless translations_valid?(listing_translations, listing['LastModifiedDate'])
Rails.logger.info("Translations are not valid for #{id}")
# TODO: when should we force salesforce listing recache?
results = Request.new(parse_response: true).cached_get(endpoint, nil, true)
listing_translations = CacheService.new.process_translations(results.first)
end
Expand All @@ -53,7 +54,10 @@ def self.listing(id, opts = {})
listing
end

def self.translations_valid?(listing_translations)
def self.translations_valid?(listing_translations, listing_last_modified)
return false unless listing_translations[:LastModifiedDate] == listing_last_modified
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: This seems to duplicate the method CacheService#listing_unchanged?, which gets run every 10 minutes via the prefetch job. Does this make the prefetch job redundant now? I.e. do we still need a background process to update caches for all modified listings every 10 minutes, when we are now doing it on every visit to a listing details page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good question and is related to the open question in the description.

This function compares the listing LastModifiedDate and the translations cache LastModifiedDate.

This returns false when:

  • A listing has been updated but the event subscriber failed to update the translations.

  • With the force=true call in the google_translate_service, we should always refresh the listing object when we update translations. So in theory, the LastModifiedDate of the listing should be guaranteed to be accurate for the purposes of the comparison.

If this function is false, then we process translations -> force update the listing -> translate all fields.

So yes, if the event subscriber is working -> then everytime we update a listing -> we translate and force update the listing. We could probably remove the prefetch job in this case.

If the event subscriber is down for some reason, then we would not update translations or force refresh the listing.
If we disable the prefetch job under this scenario: The listing wouldn't reflect the new changes until a manual force refresh. And we wouldn't catch the disagreement on the listing service call.

We could remove the force=true call from the google translate service, and then this check in the listing detail would return false due to the translations being newer. (Unless the prefetch job gets it first). The intended behavior would be to refresh the listing. Then the dates should match.

This ticket primarily aims to resolve the potential complications of the event subscriber failing. We do plan on implementing event replays so that is a future consideration.

Hopefully this provides some insight into the potential paths forward. We could keep the prefetch job and introduce some redundancy that would cover all of the potential scenarios.

We could remove the force=true call from the google_translation service, and just let the listing_service be responsible for updating the listing cache. Perhaps that would simplify things for us.

Maybe we should make the LastModifiedDate comparison have two outcomes: if the listing date is older than the translations -> refresh the listing. If the translation date is older than the listing -> refresh the translations.


Rails.logger.info('Translations cache is up to date')
listing_field_names_salesforce = ServiceHelper.listing_field_names_salesforce
listing_field_names_salesforce.each do |field_name|
return false unless listing_translations.key?(field_name.to_sym)
Expand Down
12 changes: 8 additions & 4 deletions app/services/google_translation_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@ def translate(text, to)
end
end

def cache_listing_translations(listing_id, keys, translations)
translations = transform_translations_for_caching(listing_id, keys, translations)
def cache_listing_translations(listing_id, keys, translations, last_modified)
Force::Request.new(parse_response: true).cached_get(
"/ListingDetails/#{CGI.escape(listing_id)}", nil, true
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: It's not entirely clear to me why cached_get was added here. Don't we do that earlier in the chain of method calls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding it here would guarantee that the listing is refreshed anytime we run translations for a listing. (Either by event trigger or a full process call).

This solves the issue of https://sfgovdt.jira.com/browse/DAH-2575, as well as ensures the last modified date of the listing is immediately correct for when the listing service is hit.

Testing through the different cases in the review instructions may provide some clarity to my above open question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on our convo in the translations sync, can we remove the force update listing call here?

translations = transform_translations_for_caching(listing_id, keys, translations,
last_modified)
if @cache.write("/ListingDetails/#{listing_id}/translations", translations)
google_translation_logger(
"Successfully cached listing translations for listing id: #{listing_id}",
Expand All @@ -49,13 +53,13 @@ def parse_translations(results)
results.map(&:text)
end

def transform_translations_for_caching(listing_id, keys, translations)
def transform_translations_for_caching(listing_id, keys, translations, last_modified)
prev_cached_translations = @cache.read("/ListingDetails/#{listing_id}/translations")

# keys can come from updated_values.keys in the event_subscriber_translate_service
# they will be in the same order as the translations because the translation service
# uses the values from that object and the api returns 1 for each key
return_value = {}
return_value = { LastModifiedDate: last_modified }
translations.each do |target|
target[:translation].each_with_index do |value, i|
field = keys[i].to_sym
Expand Down
11 changes: 9 additions & 2 deletions spec/services/google_translation_service_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@
expected_result = [{ to: 'ES', translation: %w[Hello World] }]
listing_id = 'a0W0P00000F8YG4UAN'
let(:mock_translate) { instance_double(Google::Cloud::Translate::V2::Api) }
let(:mock_request) { instance_double(Force::Request) }

before do
allow(Google::Cloud::Translate::V2).to receive(:new).and_return(mock_translate)
allow(Force::Request).to receive(:new).and_return(mock_request)
allow(mock_request).to receive(:cached_get).and_return(true)
end

it 'translates text to the target language' do
Expand All @@ -32,9 +35,11 @@
listing_id,
fields,
expected_result,
'2021-07-01T00:00:00Z',
)

expect(result).to eq({ Hello: { ES: 'Hello' }, World: { ES: 'World' } })
expect(result).to eq({ LastModifiedDate: '2021-07-01T00:00:00Z',
Hello: { ES: 'Hello' }, World: { ES: 'World' } })
end

it 'updates translation object for a listing' do
Expand All @@ -51,9 +56,11 @@
listing_id,
fields_updated,
translations,
'2021-07-01T00:00:00Z',
)

expect(result).to eq({ Hello: { ES: 'Hello' }, World: { ES: 'Mundo' } })
expect(result).to eq({ LastModifiedDate: '2021-07-01T00:00:00Z',
Hello: { ES: 'Hello' }, World: { ES: 'Mundo' } })
end
end
end
Loading