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

feat: DAH-2556 check translations cache LastModifiedDate #2283

Merged
merged 15 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
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
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
74 changes: 55 additions & 19 deletions app/services/force/listing_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
module Force
# encapsulate all Salesforce Listing querying functions
class ListingService
CACHE_KEY_PREFIX = '/ListingDetails/'
DATE_FORMAT = '%Y-%m-%dT%H:%M:%SZ'
# get all open listings or specific set of listings by id
# `ids` is a comma-separated list of ids
# `type` designates "ownership" or "rental" listings
Expand All @@ -29,36 +31,70 @@ def self.eligible_listings(filters)
# get one detailed listing result by id
def self.listing(id, opts = {})
@cache = Rails.cache
endpoint = "/ListingDetails/#{CGI.escape(id)}"
endpoint = "#{CACHE_KEY_PREFIX}#{CGI.escape(id)}"
force = opts[:force] || false
Rails.logger.info("Calling self.listing for #{id} with force: #{force}")

results = Request.new(parse_response: true).cached_get(endpoint, nil, force)
listing = process_listing_images(results)

listing['translations'] = get_listing_translations(listing) || {}
cade-exygy marked this conversation as resolved.
Show resolved Hide resolved
listing
end

def self.process_listing_images(results)
results_with_cached_listing_images = add_cloudfront_urls_for_listing_images(results)
listing = add_image_urls(results_with_cached_listing_images).first
add_image_urls(results_with_cached_listing_images).first
end

if ::UNLEASH.is_enabled? 'GoogleCloudTranslate'
listing_translations = @cache.fetch("/ListingDetails/#{id}/translations") do
Rails.logger.info("Nothing in cache for Listing #{id} translations")
{}
end
def self.get_listing_translations(listing)
return unless ::UNLEASH.is_enabled? 'GoogleCloudTranslate'
cade-exygy marked this conversation as resolved.
Show resolved Hide resolved

unless translations_valid?(listing_translations)
Rails.logger.info("Translations are not valid for #{id}")
results = Request.new(parse_response: true).cached_get(endpoint, nil, true)
listing_translations = CacheService.new.process_translations(results.first)
end
listing_id = listing['Id']
listing_translations = fetch_listing_translations_from_cache(listing_id)
if translations_invalid?(listing_translations) || translations_are_outdated?(
listing_translations[:LastModifiedDate], listing['LastModifiedDate']
)
Rails.logger.info("Translations are not valid for #{listing_id}")
return CacheService.new.process_translations(listing)
end

listing['translations'] = listing_translations || {}
if listing_is_outdated?(listing_translations[:LastModifiedDate],
listing['LastModifiedDate'])
Rails.logger.info("Listing is outdated for #{listing_id}")
refresh_listing_cache(listing_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we discussed not refreshing the listing cache in the translations flow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We removed it from the translation service. We do want to refresh the listing when we know that the listing is outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

That wasn't my understanding. I thought we wanted to avoid updating the listing cache on page load - if we update on page load than the cache is moot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I discussed this above:

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.

But it is a fair question. Relates to this ticket as well: https://sfgovdt.jira.com/browse/DAH-2575

We can wait for the 10 minutes: which means english values will be out of date until a listing recache if we update translations via an event. If we have an event miss, then we would have to wait the 10 minutes to resolve those differences as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed in a meeting and decided to move forward with updating the listing cache!

end
listing
listing_translations
end

def self.translations_invalid?(listing_translations)
ServiceHelper.listing_field_names_salesforce.any? do |field_name|
!listing_translations.key?(field_name.to_sym)
end
end

def self.translations_are_outdated?(cache_last_modified, listing_last_modified)
parse_time(cache_last_modified) < parse_time(listing_last_modified)
end

def self.listing_is_outdated?(cache_last_modified, listing_last_modified)
parse_time(cache_last_modified) > parse_time(listing_last_modified)
end

def self.translations_valid?(listing_translations)
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)
def self.fetch_listing_translations_from_cache(listing_id)
@cache.fetch("#{CACHE_KEY_PREFIX}#{listing_id}/translations") do
Rails.logger.info("Nothing in cache for Listing #{listing_id} translations")
{}
end
true
end

def self.refresh_listing_cache(listing_id)
endpoint = "#{CACHE_KEY_PREFIX}#{CGI.escape(listing_id)}"
Request.new(parse_response: true).cached_get(endpoint, nil, true)
end

def self.parse_time(time_str)
Time.parse(time_str)
end

# get all units for a given listing
Expand Down
9 changes: 5 additions & 4 deletions app/services/google_translation_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ 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)
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 +50,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
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@
"webpack-dev-server": "^4.15.1"
},
"engines": {
"npm": "5.3.0",
"yarn": "1.x.x",
"node": "18.x.x"
},
Expand Down
11 changes: 9 additions & 2 deletions spec/services/google_translation_service_spec.rb
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
56 changes: 44 additions & 12 deletions spec/services/listing_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@
end
end

# before do
# allow_any_instance_of(Force::Request).to receive(:oauth_token)
# end

describe '.listings' do
it 'should pass type down to Salesforce request for ownership listing' do
expect_any_instance_of(Force::Request).to receive(:cached_get)
Expand Down Expand Up @@ -63,29 +59,65 @@
end
end

describe '.listing with GoogleCloudTranslate feature flag enabled' do
describe '.listing with GoogleCloudTranslate feature flag disabled' do
it 'should return details of a listing' do
endpoint = '/ListingDetails/' + listing_id
endpoint = "/ListingDetails/#{listing_id}"
expect_any_instance_of(Force::Request).to receive(:cached_get)
.with(endpoint, nil, false).and_return([single_listing])
Force::ListingService.listing(listing_id)
end
end

describe '.listing with GoogleCloudTranslate feature flag disabled' do
describe '.listing with GoogleCloudTranslate feature flag enabled' do
let(:cache_store) { instance_double(ActiveSupport::Cache::MemoryStore) }
let(:endpoint) { "/ListingDetails/#{listing_id}" }
let(:single_listing) do
{ 'Id' => listing_id, 'LastModifiedDate' => '2024-03-08T16:51:35.000+0000' }
end
let(:request_instance) { instance_double(Force::Request) }
let(:cache_service) { instance_double(CacheService) }

before do
allow(Rails).to receive(:cache).and_return(cache_store)

allow(Force::Request).to receive(:new).and_return(request_instance)
allow(request_instance).to receive(:cached_get).with(endpoint, nil,
false).and_return([single_listing])
allow(::UNLEASH).to receive(:is_enabled?) # rubocop:disable Style/RedundantConstantBase
.with('GoogleCloudTranslate')
.and_return(true)

allow(cache_store).to receive(:fetch)
.with("/ListingDetails/#{listing_id}/translations")
.and_return({ LastModifiedDate: '2024-03-08T16:51:35.000+0000' })

allow(CacheService).to receive(:new).and_return(cache_service)

allow(cache_service).to receive(:process_translations).and_return({ LastModifiedDate: '2024-03-08T16:51:35.000+0000' })
end
after do
allow(::UNLEASH).to receive(:is_enabled?) # rubocop:disable Style/RedundantConstantBase
.and_return(false)
it 'should return details of a listing with cached listing_translations attached' do
endpoint = "/ListingDetails/#{listing_id}"
expect(request_instance).to receive(:cached_get)
.with(endpoint, nil, false).and_return([single_listing])

expect(Force::ListingService).to receive(:translations_invalid?).and_return(false)

expect(cache_store).to receive(:fetch).and_return({ LastModifiedDate: '2024-03-08T16:51:35.000+0000' })

listing = Force::ListingService.listing(listing_id)
expect(listing).to have_key('translations')
end
it 'should return details of a listing with listing_translations attached' do
it 'should return details of a listing with fresh listing_translations attached' do
endpoint = "/ListingDetails/#{listing_id}"
expect_any_instance_of(Force::Request).to receive(:cached_get)
expect(request_instance).to receive(:cached_get)
.with(endpoint, nil, false).and_return([single_listing])

expect(Force::ListingService).to receive(:translations_invalid?).and_return(true)

expect(cache_store).to receive(:fetch).and_return({ LastModifiedDate: '2024-03-08T16:51:35.000+0000' })

expect(cache_service).to receive(:process_translations).and_return({ LastModifiedDate: '2024-03-08T16:51:35.000+0000' })

listing = Force::ListingService.listing(listing_id)
expect(listing).to have_key('translations')
end
Expand Down
Loading