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
Changes from 1 commit
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
31 changes: 12 additions & 19 deletions deliver/lib/deliver/upload_metadata.rb
Expand Up @@ -89,7 +89,7 @@ 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_localizations = verify_available_info_languages!(options, app, enabled_languages) unless options[:edit_live] or !is_updating_app_info(options)

if options[:edit_live]
# not all values are editable when using live_version
Expand Down Expand Up @@ -222,12 +222,7 @@ def upload(options)
app_info_worker.start

# Update categories
# When there are iOS and macOS implementations of the same app,
# fetch_edit_app_info will fail when trying to fetch data for one platform
# if the other is in not editable state.
# We still keep fetch_edit_app_info as first option to make sure
# latest data is fetched when apps are in an editable state.
app_info = fetch_edit_app_info(app) || fetch_live_app_info(app)
app_info = fetch_edit_app_info(app)
if app_info
category_id_map = {}

Expand Down Expand Up @@ -442,12 +437,6 @@ 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 @@ -462,14 +451,18 @@ def retry_if_nil(message, tries: 5, wait_time: 10)
end
end

# Checking if the metadata to update includes App Info
def is_updating_app_info(options)
Copy link
Member

Choose a reason for hiding this comment

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

This function could look more Ruby-y 😊 simply:

def updating_app_info?(options)
  LOCALISED_APP_VALUES.keys.any? { |key| options[key] }
end

updating_app_info? is the Ruby naming convention for functions that return boolean values (instead of e.g. is…(…)) 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Clearly, I'm not a ruby-ist :-)

Copy link
Member

Choose a reason for hiding this comment

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

I just know a thing or two! Always happy to share knowledge and help others ❤️

LOCALISED_APP_VALUES.keys.each do |key|
rogerluan marked this conversation as resolved.
Show resolved Hide resolved
return true unless options[key].nil?
rogerluan marked this conversation as resolved.
Show resolved Hide resolved
end

return false
end

# Finding languages to enable
def verify_available_info_languages!(options, app, languages)
# When there are iOS and macOS implementations of the same app,
# fetch_edit_app_info will fail when trying to fetch data for one platform
# if the other is in not editable state.
# We still keep fetch_edit_app_info as first option to make sure
# latest data is fetched when apps are in an editable state.
app_info = fetch_edit_app_info(app) || fetch_live_app_info(app)
app_info = fetch_edit_app_info(app)

unless app_info
UI.user_error!("Cannot update languages - could not find an editable info")
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit outside of the scope of this PR, but while we're updating the code around that logic, I feel we could use message that could be a tad more helpful. Wdyt?

Suggested change
UI.user_error!("Cannot update languages - could not find an editable info")
UI.user_error!("Cannot update languages - could not find an editable version. Verify that your app is in one of the editable states in App Store Connect")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!
I wonder if "version" would be confusing, as in this case the user may have an editable version (on one platform), but not editable App info (e.g. because the other platform is under review).
I used your wording, but switched 'version' to 'App info'.
Let me know if it doesn't look good to you.

Copy link
Member

Choose a reason for hiding this comment

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

I'd use either quoted an capitalized 'App Info' or simple nouns (thus all lowercase) app info @loremattei 😊

Copy link
Member

Choose a reason for hiding this comment

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

I've pushed a commit to change to 'App Info' so we can move forward with this PR, I hope you don't mind 😊

Copy link
Member

Choose a reason for hiding this comment

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

One last thing! What do you guys think about the message I suggested earlier? User-friendlier and more specific?

What I suggested earlier:

Unable to upload metadata: It seems that the app you're attempting to upload metadata for isn't currently in an editable state across any platform. Please ensure that the app you're working with falls into one of these states: Prepare for Submission, Developer Rejected, Rejected, Metadata Rejected, Waiting for Review, or Invalid Binary.

What I think we could use now, after the latest changes to the logic:

Unable to upload metadata: It seems that the app you're attempting to upload metadata for isn't currently in an editable state. Please ensure that the app you're working with falls into one of these states: Prepare for Submission, Developer Rejected, Rejected, Metadata Rejected, Waiting for Review, or Invalid Binary.

@loremattei @AliSoftware

Copy link
Contributor

Choose a reason for hiding this comment

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

WFM 👍

Expand Down