[supply] introduce a new synchronization logic for screenshots

supply/lib/supply.rb

@@ -3,6 +3,7 @@
 require 'supply/client'
 require 'supply/listing'
 require 'supply/apk_listing'
+require 'supply/image_listing'
 require 'supply/generated_universal_apk'
 require 'supply/release_listing'
 require 'supply/uploader'

supply/lib/supply/client.rb

@@ -532,6 +532,11 @@ def update_obb(apk_version_code, expansion_file_type, references_version, file_s
     # @!group Screenshots
     #####################################################
 
+    # @param [String] image_type Typically one of the elements of either Supply::IMAGES_TYPES or Supply::SCREENSHOT_TYPES
+    # @param [String] language Localization code (a BCP-47 language tag; for example, "de-AT" for  Austrian German).
+    #
+    # @return [Array<ImageListing>] A list of PlayStoreImage instances describing each image
+    #
     def fetch_images(image_type: nil, language: nil)
       ensure_active_edit!
 
@@ -544,29 +549,14 @@ def fetch_images(image_type: nil, language: nil)
         )
       end
 
-      urls = (result.images || []).map(&:url)
-      images = urls.map do |url|
-        uri = URI.parse(url)
-        clean_url = [
-          uri.scheme,
-          uri.userinfo,
-          uri.host,
-          uri.port,
-          uri.path
-        ].join
-
-        UI.verbose("Initial URL received: '#{url}'")
-        UI.verbose("Removed params ('#{uri.query}') from the URL")
-        UI.verbose("URL after removing params: '#{clean_url}'")
-
-        full_url = "#{url}=s0" # '=s0' param ensures full image size is returned (https://github.com/fastlane/fastlane/pull/14322#issuecomment-473012462)
-        full_url
+      (result.images || []).map do |row|
+        full_url = "#{row.url}=s0" # '=s0' param ensures full image size is returned (https://github.com/fastlane/fastlane/pull/14322#issuecomment-473012462)
+        ImageListing.new(row.id, row.sha1, row.sha256, full_url)
       end
-
-      return images
     end
 
-    # @param image_type (e.g. phoneScreenshots, sevenInchScreenshots, ...)
+    # @param [String] image_type (e.g. phoneScreenshots, sevenInchScreenshots, ...)
+    # @param [String] language localization code (i.e. BCP-47 language tag as in `pt-BR`)
     def upload_image(image_path: nil, image_type: nil, language: nil)
       ensure_active_edit!
 
@@ -595,6 +585,20 @@ def clear_screenshots(image_type: nil, language: nil)
       end
     end
 
+    def clear_screenshot(image_type: nil, language: nil, image_id: nil)
+      ensure_active_edit!
+
+      call_google_api do
+        client.delete_edit_image(
+          current_package_name,
+          current_edit.id,
+          language,
+          image_type,
+          image_id
+        )
+      end
+    end
+
     def upload_obb(obb_file_path: nil, apk_version_code: nil, expansion_file_type: nil)
       ensure_active_edit!
 

supply/lib/supply/image_listing.rb

@@ -0,0 +1,15 @@
+module Supply
+  class ImageListing
+    attr_reader :id
+    attr_reader :sha1
+    attr_reader :sha256
+    attr_reader :url
+
+    def initialize(id, sha1, sha256, url)
+      @id = id
+      @sha1 = sha1
+      @sha256 = sha256
+      @url = url
+    end
+  end
+end

supply/lib/supply/options.rb

@@ -210,6 +210,12 @@ def self.available_options
                                      description: "Whether to skip uploading SCREENSHOTS",
                                      type: Boolean,
                                      default_value: false),
+        FastlaneCore::ConfigItem.new(key: :sync_image_upload,
+                                     env_name: "SUPPLY_SYNC_IMAGE_UPLOAD",
+                                     optional: true,
+                                     description: "Weather to use sha256 comparison to skip upload of images and screenshots that are already in Play Store",
+                                     type: Boolean,
+                                     default_value: false),
         FastlaneCore::ConfigItem.new(key: :track_promote_to,
                                      env_name: "SUPPLY_TRACK_PROMOTE_TO",
                                      optional: true,

supply/lib/supply/setup.rb

@@ -66,7 +66,7 @@ def download_images(listing)
 
           UI.message("Downloading `#{image_type}` for #{listing.language}...")
 
-          urls = client.fetch_images(image_type: image_type, language: listing.language)
+          urls = client.fetch_images(image_type: image_type, language: listing.language).map(&:url)
           next if urls.nil? || urls.empty?
 
           image_counter = 1 # Used to prefix the downloaded files, so order is preserved.

supply/lib/supply/uploader.rb

@@ -270,7 +270,17 @@ def upload_images(language)
         path = Dir.glob(search, File::FNM_CASEFOLD).last
         next unless path
 
-        UI.message("Uploading image file #{path}...")
+        if Supply.config[:sync_image_upload]
+          UI.message("🔍 Checking #{image_type} checksum...")
+          existing_images = client.fetch_images(image_type: image_type, language: language)
+          sha256 = Digest::SHA256.file(path).hexdigest
+          if existing_images.map(&:sha256).include?(sha256)
+            UI.message("🟰 Skipping upload of screenshot #{path} as remote sha256 matches.")
+            next
+          end
+        end
+
+        UI.message("⬆️ Uploading image file #{path}...")
         client.upload_image(image_path: File.expand_path(path),
                             image_type: image_type,
                               language: language)
@@ -280,13 +290,30 @@ def upload_images(language)
     def upload_screenshots(language)
       Supply::SCREENSHOT_TYPES.each do |screenshot_type|
         search = File.join(metadata_path, language, Supply::IMAGES_FOLDER_NAME, screenshot_type, "*.#{IMAGE_FILE_EXTENSIONS}")
-        paths = Dir.glob(search, File::FNM_CASEFOLD)
+        paths = Dir.glob(search, File::FNM_CASEFOLD).sort
         next unless paths.count > 0
 
-        client.clear_screenshots(image_type: screenshot_type, language: language)
+        if Supply.config[:sync_image_upload]
+          UI.message("🔍 Checking #{screenshot_type} checksums...")
+          existing_images = client.fetch_images(image_type: screenshot_type, language: language)
+          # Don't keep images that either don't exist locally, or that are out of order compared to the `paths` to upload
+          first_path_checksum = Digest::SHA256.file(paths.first).hexdigest
+          existing_images.each do |image|
+            if image.sha256 == first_path_checksum
+              UI.message("🟰 Skipping upload of screenshot #{paths.first} as remote sha256 matches.")
+              paths.shift # Remove first path from the list of paths to be uploaded
+              first_path_checksum = paths.empty? ? nil : Digest::SHA256.file(paths.first).hexdigest
+            else
+              UI.message("🚮 Deleting #{language} screenshot id ##{image.id} as it does not exist locally or is out of order...")
+              client.clear_screenshot(image_type: screenshot_type, language: language, image_id: image.id)
+            end
+          end
+        else
+          client.clear_screenshots(image_type: screenshot_type, language: language) unless Supply.config[:sync_image_upload]
+        end
 
-        paths.sort.each do |path|
-          UI.message("Uploading screenshot #{path}...")
+        paths.each do |path|
+          UI.message("⬆️  Uploading screenshot #{path}...")
           client.upload_image(image_path: File.expand_path(path),
                               image_type: screenshot_type,
                                 language: language)

supply/spec/client_spec.rb

@@ -54,6 +54,7 @@
         expect(subject.class.method_defined?(:list_edit_images)).to eq(true)
         expect(subject.class.method_defined?(:upload_edit_image)).to eq(true)
         expect(subject.class.method_defined?(:deleteall_edit_image)).to eq(true)
+        expect(subject.class.method_defined?(:delete_edit_image)).to eq(true)
         expect(subject.class.method_defined?(:upload_edit_expansionfile)).to eq(true)
         expect(subject.class.method_defined?(:uploadapk_internalappsharingartifact)).to eq(true)
         expect(subject.class.method_defined?(:uploadbundle_internalappsharingartifact)).to eq(true)

supply/spec/uploader_spec.rb

@@ -221,5 +221,200 @@ def find_obbs
         uploader.perform_upload
       end
     end
+
+    context 'when sync_image_upload is set' do
+      let(:client) { double('client') }
+      let(:language) { 'pt-BR' }
+      let(:config) { { metadata_path: 'spec_metadata', sync_image_upload: true } }
+
+      before do
+        Supply.config = config
+        allow(Supply::Client).to receive(:make_from_config).and_return(client)
+      end
+
+      describe '#upload_images' do
+        it 'should upload and replace image if sha256 does not match remote image' do
+          allow(Digest::SHA256).to receive(:file) { |file| instance_double(Digest::SHA256, hexdigest: "sha256-of-#{file}") }
+          allow(Dir).to receive(:glob).and_return(['image.png'])
+          remote_images = [Supply::ImageListing.new('id123', '_unused_', 'different-remote-sha256', '_unused_')]
+
+          Supply::IMAGES_TYPES.each do |image_type|
+            allow(client).to receive(:fetch_images).with(image_type: image_type, language: language).and_return(remote_images)
+            expect(client).to receive(:upload_image).with(image_path: File.expand_path('image.png'), image_type: image_type, language: language)
+          end
+
+          uploader = Supply::Uploader.new
+          uploader.upload_images(language)
+        end
+
+        it 'should skip image upload if sha256 matches remote image' do
+          allow(Digest::SHA256).to receive(:file) { |file| instance_double(Digest::SHA256, hexdigest: "sha256-of-#{file}") }
+          allow(Dir).to receive(:glob).and_return(['image.png'])
+          remote_images = [Supply::ImageListing.new('id123', '_unused_', 'sha256-of-image.png', '_unused_')]
+
+          Supply::IMAGES_TYPES.each do |image_type|
+            allow(client).to receive(:fetch_images).with(image_type: image_type, language: language).and_return(remote_images)
+            expect(client).not_to receive(:upload_image).with(image_path: File.expand_path('image.png'), image_type: image_type, language: language)
+          end
+
+          uploader = Supply::Uploader.new
+          uploader.upload_images(language)
+        end
+      end
+
+      describe '#upload_screenshots' do
+        it 'should upload and replace all screenshots if no sha256 matches any remote screenshot' do
+          allow(Digest::SHA256).to receive(:file) { |file| instance_double(Digest::SHA256, hexdigest: "local-sha256-of-#{file}") }
+          local_images = %w[image1.png image2.png image3.png]
+          allow(Dir).to receive(:glob).and_return(local_images)
+          remote_images = [1, 2, 3].map do |idx|
+            Supply::ImageListing.new("id_#{idx}", '_unused_', "remote-sha256-#{idx}", '_unused_')
+          end
+
+          Supply::SCREENSHOT_TYPES.each do |screenshot_type|
+            allow(client).to receive(:fetch_images).with(image_type: screenshot_type, language: language).and_return(remote_images)
+            remote_images.each do |image|
+              expect(client).to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: image.id)
+            end
+            local_images.each do |path|
+              expect(client).to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language)
+            end
+          end
+
+          uploader = Supply::Uploader.new
+          uploader.upload_screenshots(language)
+        end
+
+        it 'should skip all screenshots if all sha256 matches the remote screenshots' do
+          allow(Digest::SHA256).to receive(:file) { |file| instance_double(Digest::SHA256, hexdigest: "common-sha256-of-#{file}") }
+          local_images = %w[image1.png image2.png image3.png]
+          allow(Dir).to receive(:glob).and_return(local_images)
+          remote_images = local_images.map do |path|
+            Supply::ImageListing.new("id_#{path}", '_unused_', "common-sha256-of-#{path}", '_unused_')
+          end
+
+          Supply::SCREENSHOT_TYPES.each do |screenshot_type|
+            allow(client).to receive(:fetch_images).with(image_type: screenshot_type, language: language).and_return(remote_images)
+            remote_images.each do |image|
+              expect(client).not_to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: image.id)
+            end
+            local_images.each do |path|
+              expect(client).not_to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language)
+            end
+          end
+
+          uploader = Supply::Uploader.new
+          uploader.upload_screenshots(language)
+        end
+
+        it 'should delete and re-upload screenshots that changed locally, as long as start of list is in order' do
+          allow(Digest::SHA256).to receive(:file) { |file| instance_double(Digest::SHA256, hexdigest: "sha256-of-#{file}") }
+          local_images = %w[image0.png image1.png new-image2.png new-image3.png]
+          allow(Dir).to receive(:glob).and_return(local_images)
+
+          remote_images = %w[image0.png image1.png old-image2.png old-image3.png].map.with_index do |path, idx|
+            Supply::ImageListing.new("id_#{idx}", '_unused_', "sha256-of-#{path}", '_unused_')
+          end
+
+          Supply::SCREENSHOT_TYPES.each do |screenshot_type|
+            allow(client).to receive(:fetch_images).with(image_type: screenshot_type, language: language).and_return(remote_images)
+            local_images[0..1].each_with_index do |path, idx|
+              expect(client).not_to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: "id_#{idx}")
+              expect(client).not_to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language)
+            end
+            local_images[2..3].each_with_index do |path, idx|
+              expect(client).to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: "id_#{idx + 2}")
+              expect(client).to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language)
+            end
+          end
+
+          uploader = Supply::Uploader.new
+          uploader.upload_screenshots(language)
+        end
+
+        it 'should delete remote screenshots that are no longer present locally' do
+          allow(Digest::SHA256).to receive(:file) { |file| instance_double(Digest::SHA256, hexdigest: "common-sha256-of-#{file}") }
+          local_images = %w[image1.png image2.png image3.png]
+          allow(Dir).to receive(:glob).and_return(local_images)
+
+          same_remote_images = local_images.map do |path|
+            Supply::ImageListing.new("id_#{path}", '_unused_', "common-sha256-of-#{path}", '_unused_')
+          end
+          extra_remote_image = Supply::ImageListing.new("id_extra", '_unused_', "common-sha256-of-extra-image", '_unused_')
+          remote_images = [same_remote_images[0], extra_remote_image, same_remote_images[1], same_remote_images[2]]
+
+          Supply::SCREENSHOT_TYPES.each do |screenshot_type|
+            allow(client).to receive(:fetch_images).with(image_type: screenshot_type, language: language).and_return(remote_images)
+            same_remote_images.each do |image|
+              expect(client).not_to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: image.id)
+            end
+            expect(client).to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: extra_remote_image.id)
+            local_images.each do |path|
+              expect(client).not_to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language)
+            end
+          end
+
+          uploader = Supply::Uploader.new
+          uploader.upload_screenshots(language)
+        end
+
+        it 'should delete screenshots that are out of order and re-upload them in the correct order' do
+          allow(Digest::SHA256).to receive(:file) { |file| instance_double(Digest::SHA256, hexdigest: "sha256-of-#{file}") }
+          local_images = %w[image0.png image1.png image2.png image4.png image3.png image5.png image6.png] # those will be sorted after Dir.glob
+          allow(Dir).to receive(:glob).and_return(local_images)
+
+          # Record the mocked deletions and uploads in list of remote images to check the final state at the end
+          final_remote_images_ids = {}
+          allow(client).to receive(:clear_screenshot) do |**args|
+            image_type = args[:image_id].split('_')[1]
+            final_remote_images_ids[image_type].delete(args[:image_id])
+          end
+          allow(client).to receive(:upload_image) do |**args|
+            path = File.basename(args[:image_path])
+            image_type = args[:image_type]
+            final_remote_images_ids[image_type] << "new-id_#{image_type}_#{path}"
+          end
+
+          Supply::SCREENSHOT_TYPES.each do |screenshot_type|
+            remote_images = local_images.map do |path|
+              Supply::ImageListing.new("id_#{screenshot_type}_#{path}", '_unused_', "sha256-of-#{path}", '_unused_')
+            end # remote images will be in order 0124356 though
+            allow(client).to receive(:fetch_images).with(image_type: screenshot_type, language: language).and_return(remote_images)
+
+            final_remote_images_ids[screenshot_type] = remote_images.map(&:id)
+
+            # We should skip image0, image1, image2 from remote as they are the same as the first local images,
+            # But also skip image3 (which was in-between 2 and 3 in remote listing, but is still present in local images)
+            # While deleting image4 (because it was in-between image2 and image3 in the `remote_images`, so out of order)
+            # And finally deleting image5 and image6, before re-uploading image4, image5 and image6 in the right order
+            local_images.sort[0..3].each do |path|
+              expect(client).not_to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: "id_#{screenshot_type}_#{path}")
+              expect(client).not_to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language)
+            end
+            local_images.sort[4..6].each do |path|
+              expect(client).to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: "id_#{screenshot_type}_#{path}")
+              expect(client).to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language)
+            end
+          end
+
+          uploader = Supply::Uploader.new
+          uploader.upload_screenshots(language)
+
+          # Check the final order of the remote images after the whole skip/delete/upload dance
+          Supply::SCREENSHOT_TYPES.each do |screenshot_type|
+            expected_final_images_ids = %W[
+              id_#{screenshot_type}_image0.png
+              id_#{screenshot_type}_image1.png
+              id_#{screenshot_type}_image2.png
+              id_#{screenshot_type}_image3.png
+              new-id_#{screenshot_type}_image4.png
+              new-id_#{screenshot_type}_image5.png
+              new-id_#{screenshot_type}_image6.png
+            ]
+            expect(final_remote_images_ids[screenshot_type]).to eq(expected_final_images_ids)
+          end
+        end
+      end
+    end
   end
 end