Use browse listings fields for individual listing cache #167409242 (#…

…1241)

* [#167409242] Use browse listings fields for individual listing cache

* [#167409242] Some rewording in CacheService for clarity

app/services/cache_service.rb

@@ -1,42 +1,42 @@
 # service class for pre-fetching + caching salesforce data
 class CacheService
   def prefetch_listings(opts = {})
-    # refresh oauth_token before beginning
+    # Refresh OAuth token, to avoid unauthorized errors in case it has expired
     Force::Request.new.refresh_oauth_token
-    @old_listings = Force::ListingService.raw_listings
-    @new_listings = Force::ListingService.raw_listings(refresh_cache: true)
+    @prev_cached_listings = Force::ListingService.listings(subset: 'browse')
+    @fresh_listings = Force::ListingService.listings(subset: 'browse', force: true)
+
     if opts[:refresh_all]
-      cache_listings
+      cache_all_listings
     else
-      check_listings
+      cache_only_updated_listings
     end
   end
 
   private
 
-  attr_accessor :old_listings, :new_listings
+  attr_accessor :prev_cached_listings, :fresh_listings
 
-  def cache_listings
-    new_listings.each { |listing| cache_single_listing(listing) }
+  def cache_all_listings
+    fresh_listings.each { |l| cache_single_listing(l) }
   end
 
-  def check_listings
-    new_listings.each do |listing|
-      id = listing['Id']
-      old_listing = old_listings.find { |l| l['Id'] == id }
-      unchanged = false
-      if old_listing.present?
-        sorted_old_listing = Force::ListingService.array_sort!(old_listing)
-        sorted_listing = Force::ListingService.array_sort!(listing)
-        # NOTE: This comparison isn't perfect, as the browse listings API endpoint doesn't
-        # contain some relational data e.g. some individual unit/preference details.
-        # That's why we more aggressively re-cache open listings.
-        unchanged = HashDiff.diff(sorted_old_listing, sorted_listing).empty?
+  def cache_only_updated_listings
+    fresh_listings.each do |fresh_listing|
+      prev_cached_listing = prev_cached_listings.find do |l|
+        l['Id'] == fresh_listing['Id']
       end
-      cache_single_listing(listing) unless unchanged
+
+      cache_single_listing(fresh_listing) unless
+        listing_unchanged?(prev_cached_listing, fresh_listing)
     end
   end
 
+  def listing_unchanged?(prev_cached_listing, fresh_listing)
+    prev_cached_listing.present? &&
+      (prev_cached_listing['LastModifiedDate'] == fresh_listing['LastModifiedDate'])
+  end
+
   def cache_single_listing(listing)
     id = listing['Id']
     # cache this listing from API

app/services/force/listing_service.rb

@@ -13,13 +13,8 @@ def self.listings(attrs = {})
       params[:type] = attrs[:type] if attrs[:type].present?
       params[:ids] = attrs[:ids] if attrs[:ids].present?
       params[:subset] = attrs[:subset] if attrs[:subset].present?
-      get_listings(params)
-    end
-
-    def self.raw_listings(opts = {})
-      force = opts[:refresh_cache] || false
-      Request.new(parse_response: true)
-             .cached_get('/ListingDetails', nil, force)
+      force = attrs[:force].present? ? attrs[:force] : false
+      get_listings(params, force)
     end
 
     # get listings with eligibility matches applied
@@ -104,8 +99,9 @@ def self.array_sort!(listing)
       end
     end
 
-    private_class_method def self.get_listings(params = nil)
-      results = Request.new(parse_response: true).cached_get('/ListingDetails', params)
+    private_class_method def self.get_listings(params = nil, force = false)
+      results = Request.new(parse_response: true)
+                       .cached_get('/ListingDetails', params, force)
       add_image_urls(results)
     end
 

spec/controllers/api/v1/listings_controller_spec.rb

@@ -26,7 +26,7 @@
       before do
         allow(Force::ListingService)
           .to receive(:get_listings)
-          .with(type: 'rental')
+          .with({ type: 'rental' }, false)
           .and_return(rental_listings)
       end
 
@@ -41,7 +41,7 @@
       before do
         allow(Force::ListingService)
           .to receive(:get_listings)
-          .with(type: 'ownership')
+          .with({ type: 'ownership' }, false)
           .and_return(sale_listings)
       end
 

spec/services/cache_service_spec.rb

@@ -4,8 +4,8 @@
 
 describe CacheService do
   let(:cached_listings) do
-    listings = VCR.use_cassette('listings/all_listings') do
-      Force::ListingService.raw_listings
+    listings = VCR.use_cassette('listings/all_listings_browse') do
+      Force::ListingService.listings(subset: 'browse')
     end
     # Trim down, we only need one listing for testing
     listings.take(1)
@@ -15,21 +15,21 @@
     # map from cached listings to prevent listing properties from
     # being passed by reference (and thus updated in both places)
     updated_listings = cached_listings.map(&:clone)
-    updated_listings.first['Name'] = 'The Newest Listing on the Block'
+    updated_listings.first['LastModifiedDate'] = 'Very recent date time'
     updated_listings
   end
 
   let(:updated_listing) { updated_listings.first }
 
-  let(:updated_listing_id) { updated_listing['listingID'] }
+  let(:updated_listing_id) { updated_listing['Id'] }
 
   let(:listing_image_service) { instance_double(ListingImageService) }
 
   before do
-    allow(Force::ListingService).to receive(:raw_listings)
-      .with(no_args).and_return(cached_listings)
-    allow(Force::ListingService).to receive(:raw_listings)
-      .with(refresh_cache: true).and_return(updated_listings)
+    allow(Force::ListingService).to receive(:listings)
+      .with(subset: 'browse').and_return(cached_listings)
+    allow(Force::ListingService).to receive(:listings)
+      .with(subset: 'browse', force: true).and_return(updated_listings)
     allow(Force::ListingService).to receive(:listing)
     allow(Force::ListingService).to receive(:units)
     allow(Force::ListingService).to receive(:preferences)
@@ -41,7 +41,7 @@
 
   shared_examples 'cacher of listings' do
     # expects `prefetch_args` to be a hash of options passed to `prefetch_listings`
-    it 'refreshes the listing cache' do
+    it 'refreshes the listing cache for updated listing' do
       expect(Force::ListingService).to receive(:listing)
         .with(updated_listing_id, force: true)
       expect(Force::ListingService).to receive(:units)
@@ -99,8 +99,8 @@
     context 'listing is updated' do
       before do
         # simulate an updated listing
-        allow(Force::ListingService).to receive(:raw_listings)
-          .with(refresh_cache: true).and_return(updated_listings)
+        allow(Force::ListingService).to receive(:listings)
+          .with(subset: 'browse', force: true).and_return(updated_listings)
       end
 
       it_behaves_like 'cacher of listings' do

spec/services/listing_service_spec.rb

@@ -26,11 +26,6 @@
       Force::ListingService.send :get_listings, type: 'ownership', subset: 'browse'
     end
   end
-  let(:all_listings) do
-    VCR.use_cassette('listings/all_listings') do
-      Force::ListingService.send :get_listings
-    end
-  end
   let(:all_listings_browse) do
     VCR.use_cassette('listings/all_listings_browse') do
       Force::ListingService.send :get_listings, subset: 'browse'
@@ -45,37 +40,28 @@
     it 'should pass type down to Salesforce request for ownership listing' do
       expect_any_instance_of(Force::Request).to receive(:cached_get)
         .with('/ListingDetails',
-              type: 'ownership',
-              subset: 'browse').and_return(sale_listings)
+              { type: 'ownership', subset: 'browse' }, false).and_return(sale_listings)
       Force::ListingService.listings(type: 'ownership', subset: 'browse')
     end
     it 'should pass type down to Salesforce request for rental listings' do
       expect_any_instance_of(Force::Request).to receive(:cached_get)
         .with('/ListingDetails',
-              type: 'rental',
-              subset: 'browse').and_return(rental_listings)
+              { type: 'rental', subset: 'browse' }, false).and_return(rental_listings)
       Force::ListingService.listings(type: 'rental', subset: 'browse')
     end
     it 'should pass id down to Salesforce request if part of params' do
       expect_any_instance_of(Force::Request).to receive(:cached_get)
-        .with('/ListingDetails', ids: listing_ids).and_return(listings_by_ids)
+        .with('/ListingDetails', { ids: listing_ids }, false).and_return(listings_by_ids)
       Force::ListingService.listings(ids: listing_ids)
     end
     it 'should pass subset down to Salesforce request if part of params' do
       expect_any_instance_of(Force::Request).to receive(:cached_get)
-        .with('/ListingDetails', subset: 'browse').and_return(all_listings_browse)
+        .with('/ListingDetails',
+              { subset: 'browse' }, false).and_return(all_listings_browse)
       Force::ListingService.listings(subset: 'browse')
     end
   end
 
-  describe '.raw_listings' do
-    it 'should return both rental and ownership listings' do
-      expect_any_instance_of(Force::Request).to receive(:cached_get)
-        .with('/ListingDetails', nil, false).and_return(all_listings)
-      Force::ListingService.raw_listings
-    end
-  end
-
   describe '.listing' do
     it 'should return details of a listing' do
       endpoint = '/ListingDetails/' + listing_id