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

[deliver] fetch live app info if no edit info is present, fixing scenario of having both macOS and iOS apps present #21472

Merged
merged 21 commits into from Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 54 additions & 13 deletions deliver/lib/deliver/upload_metadata.rb
Expand Up @@ -89,7 +89,8 @@ def upload(options)
enabled_languages = detect_languages(options)

app_store_version_localizations = verify_available_version_languages!(options, app, enabled_languages) unless options[:edit_live]
app_info_localizations = verify_available_info_languages!(options, app, enabled_languages) unless options[:edit_live]
app_info = fetch_edit_app_info(app)
app_info_localizations = verify_available_info_languages!(options, app, app_info, enabled_languages) unless options[:edit_live] || !updating_localized_app_info?(options, app, app_info)

if options[:edit_live]
# not all values are editable when using live_version
Expand Down Expand Up @@ -211,18 +212,19 @@ def upload(options)
store_version_worker.start

# Update app info localizations
app_info_worker = FastlaneCore::QueueWorker.new do |app_info_localization|
attributes = localized_info_attributes_by_locale[app_info_localization.locale]
if attributes
UI.message("Uploading metadata to App Store Connect for localized info '#{app_info_localization.locale}'")
app_info_localization.update(attributes: attributes)
if app_info_localizations
app_info_worker = FastlaneCore::QueueWorker.new do |app_info_localization|
attributes = localized_info_attributes_by_locale[app_info_localization.locale]
if attributes
UI.message("Uploading metadata to App Store Connect for localized info '#{app_info_localization.locale}'")
app_info_localization.update(attributes: attributes)
end
end
app_info_worker.batch_enqueue(app_info_localizations)
app_info_worker.start
end
app_info_worker.batch_enqueue(app_info_localizations)
app_info_worker.start
rogerluan marked this conversation as resolved.
Show resolved Hide resolved

# Update categories
app_info = fetch_edit_app_info(app)
loremattei marked this conversation as resolved.
Show resolved Hide resolved
if app_info
category_id_map = {}

Expand Down Expand Up @@ -437,6 +439,12 @@ def fetch_edit_app_info(app, wait_time: 10)
end
end

def fetch_live_app_info(app, wait_time: 10)
retry_if_nil("Cannot find live app info", wait_time: wait_time) do
app.fetch_live_app_info
end
end

def retry_if_nil(message, tries: 5, wait_time: 10)
loop do
tries -= 1
Expand All @@ -451,12 +459,45 @@ def retry_if_nil(message, tries: 5, wait_time: 10)
end
end

# Finding languages to enable
def verify_available_info_languages!(options, app, languages)
app_info = fetch_edit_app_info(app)
# Checking if the metadata to update includes localised App Info
def updating_localized_app_info?(options, app, app_info)
app_info ||= fetch_live_app_info(app)
rogerluan marked this conversation as resolved.
Show resolved Hide resolved
unless app_info
UI.important("Can't find edit or live App info. Skipping upload.")
return false
end
localizations = app_info.get_app_info_localizations
Copy link
Contributor

Choose a reason for hiding this comment

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

[Unrelated to this PR]

@rogerluan we definitively need to improve our YARD doc on many of our model methods. This isn't clear at all from the definition of the AppInfo#get_app_info_localizations what that method is supposed to return, which makes it harder to read code that uses it like here and why Lore needed to use localizations.find below.

(After some digging, the answer is that it returns Array<Spaceship::ConnectAPI::AppInfo>; which makes sense in retrospect I guess, but was't obvious to me at first sight, and RubyMine isn't able to deduce it automatically)


LOCALISED_APP_VALUES.keys.each do |key|
rogerluan marked this conversation as resolved.
Show resolved Hide resolved
current = options[key]
next unless current

unless current.kind_of?(Hash)
UI.error("Error with provided '#{key}'. Must be a hash, the key being the language.")
rogerluan marked this conversation as resolved.
Show resolved Hide resolved
next
end

current.each do |language, value|
strip_value = value.to_s.strip
next if strip_value.empty?

app_info_locale = localizations.find { |l| l.locale == language }
next if app_info_locale.nil?

current_value = app_info_locale.public_send(key.to_sym)

return true if current_value != strip_value
end
end

UI.message('No changes to localized App Info detected. Skipping upload.')
Copy link
Contributor

@owjsub owjsub Nov 21, 2023

Choose a reason for hiding this comment

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

I believe that skipping upload here is causing a regression with editing the privacy url #21654

Is there a way we can force upload so that we can edit the privacy url even though none of the other localized app info will change?

Copy link
Contributor

Choose a reason for hiding this comment

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

return false
end

# Finding languages to enable
def verify_available_info_languages!(options, app, app_info, languages)
unless app_info
UI.user_error!("Cannot update languages - could not find an editable info")
UI.user_error!("Cannot update languages - could not find an editable 'App Info'. Verify that your app is in one of the editable states in App Store Connect")
return
end

Expand Down
95 changes: 91 additions & 4 deletions deliver/spec/upload_metadata_spec.rb
rogerluan marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -162,6 +162,7 @@ def create_metadata(path, text)
locale: 'en-US')
end
let(:app_info) { double('app_info') }
let(:live_app_info) { nil }
let(:app_info_localization_en) do
double('app_info_localization_en',
locale: 'en-US')
Expand Down Expand Up @@ -237,15 +238,18 @@ def create_metadata(path, text)
# Verify available languages
expect(app).to receive(:id).and_return(id)
expect(app).to receive(:get_edit_app_store_version).and_return(version)
expect(app).to receive(:fetch_edit_app_info).and_return(app_info)
expect(uploader).to receive(:fetch_edit_app_info).and_return(app_info)

# Get versions
expect(app).to receive(:get_edit_app_store_version).and_return(version)
expect(version).to receive(:get_app_store_version_localizations).and_return([version_localization_en])

# Get app infos
expect(app).to receive(:fetch_edit_app_info).and_return(app_info)
expect(app_info).to receive(:get_app_info_localizations).and_return([app_info_localization_en])
if app_info
expect(app_info).to receive(:get_app_info_localizations).and_return([app_info_localization_en])
else
expect(live_app_info).to receive(:get_app_info_localizations).and_return([app_info_localization_en])
end
end

context "normal metadata" do
Expand All @@ -257,11 +261,17 @@ def create_metadata(path, text)
description: { "en-US" => "App description" }
}

# Get number of verions (used for if whats_new should be sent)
# Get number of versions (used for if whats_new should be sent)
expect(Spaceship::ConnectAPI).to receive(:get_app_store_versions).and_return(app_store_versions)

expect(version).to receive(:update).with(attributes: {})

# Get app info
expect(app_info).to receive(:get_app_info_localizations).and_return([app_info_localization_en])

# Get app info localization English (Used to compare with data to upload)
expect(app_info_localization_en).to receive(:name).and_return('App Name')

# Update version localization
expect(version_localization_en).to receive(:update).with(attributes: {
"description" => options[:description]["en-US"]
Expand Down Expand Up @@ -412,6 +422,83 @@ def create_metadata(path, text)
uploader.upload(options)
end
end

context "with no editable app info" do
let(:live_app_info) { double('app_info') }
let(:app_info) { nil }
it 'no new app info provided by user' do
options = {
platform: "ios",
metadata_path: metadata_path,
}

# Get live app info
expect(app).to receive(:fetch_live_app_info).and_return(live_app_info)

# Get number of versions (used for if whats_new should be sent)
expect(Spaceship::ConnectAPI).to receive(:get_app_store_versions).and_return(app_store_versions)
expect(version).to receive(:update).with(attributes: {})

uploader.upload(options)
end

it 'same app info as live version' do
options = {
platform: "ios",
metadata_path: metadata_path,
name: { "en-US" => "App name" }
}

# Get live app info
expect(app).to receive(:fetch_live_app_info).and_return(live_app_info)

# Get number of versions (used for if whats_new should be sent)
loremattei marked this conversation as resolved.
Show resolved Hide resolved
expect(Spaceship::ConnectAPI).to receive(:get_app_store_versions).and_return(app_store_versions)

expect(version).to receive(:update).with(attributes: {})

# Get app info localization in English (used to compare with data to upload)
expect(app_info_localization_en).to receive(:name).and_return('App name')

uploader.upload(options)
end
end
end

context "fail when not allowed to update" do
let(:live_app_info) { double('app_info') }
let(:app_info) { nil }
it 'different app info than live version' do
options = {
platform: "ios",
metadata_path: metadata_path,
name: { "en-US" => "New app name" }
}

allow(Deliver).to receive(:cache).and_return({ app: app })

allow(uploader).to receive(:set_review_information)
allow(uploader).to receive(:set_review_attachment_file)
allow(uploader).to receive(:set_app_rating)

# Get app info
expect(uploader).to receive(:fetch_edit_app_info).and_return(app_info)
expect(app).to receive(:fetch_live_app_info).and_return(live_app_info)
expect(live_app_info).to receive(:get_app_info_localizations).and_return([app_info_localization_en])

# Get versions
expect(app).to receive(:get_edit_app_store_version).and_return(version)
expect(version).to receive(:get_app_store_version_localizations).and_return([version_localization_en])

# Get app info localization in English (used to compare with data to upload)
expect(app_info_localization_en).to receive(:name).and_return('App name')

# Fail because app info can't be updated
expect(FastlaneCore::UI).to receive(:user_error!).with("Cannot update languages - could not find an editable 'App Info'. Verify that your app is in one of the editable states in App Store Connect").and_call_original

# Get app info localization in English (used to compare with data to upload)
expect { uploader.upload(options) }.to raise_error(FastlaneCore::Interface::FastlaneError)
end
end
end

Expand Down