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.

Sign up for GitHub

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

Jump to bottom

[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

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

Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c976e9a
Fetch live app info if no edit info is present
loremattei Aug 21, 2023
23ea7c0
Add comments to explain the fallback
loremattei Aug 28, 2023
9453b77
Check App Info localization only when new metadata contains App Info
loremattei Aug 30, 2023
d57b974
Fix lint issues
loremattei Aug 30, 2023
d62af0d
Only fail upload when trying to update localised app info in no edit …
loremattei Sep 1, 2023
7d42af5
Update tests
loremattei Sep 1, 2023
c1bdd94
Make rubocop happy
loremattei Sep 1, 2023
f5187ff
Update deliver/lib/deliver/upload_metadata.rb
loremattei Sep 3, 2023
dfbf80d
Update deliver/lib/deliver/upload_metadata.rb
loremattei Sep 3, 2023
d04fb08
Update deliver/lib/deliver/upload_metadata.rb
loremattei Sep 3, 2023
aae5bdd
Update message severity
loremattei Sep 3, 2023
5c161ef
Minimize api call to fetch app infos
loremattei Sep 3, 2023
66470dc
Add tests
loremattei Sep 4, 2023
e016f01
Fix simple typos.
rogerluan Sep 5, 2023
896c292
Refactor to more Ruby-like syntax
loremattei Sep 5, 2023
89086aa
Update tests
loremattei Sep 5, 2023
52bbce7
Update spelling for consistency
loremattei Sep 6, 2023
09f89e3
Update spelling for consistency
loremattei Sep 6, 2023
104ecdb
Address feedback
loremattei Sep 6, 2023
0cc2bfc
Update user-facing message.
rogerluan Sep 10, 2023
0e62085
Fix syntax of string quotes.
rogerluan Sep 10, 2023
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
20 changes: 18 additions & 2 deletions deliver/lib/deliver/upload_metadata.rb
View file
Open in desktop
Expand Up @@ -222,7 +222,12 @@ def upload(options)
app_info_worker.start

# Update categories
app_info = fetch_edit_app_info(app)
loremattei marked this conversation as resolved.
Show resolved Hide resolved
# 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.
Copy link
Member

Choose a reason for hiding this comment

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

What do you think?

Suggested change
# 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.
# 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 isn't in an editable state, so we use the live app info.
# We still keep fetch_edit_app_info as first option to make sure
# latest data is fetched when apps are in an editable state.

Copy link
Member

Choose a reason for hiding this comment

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

Same for the comment you left below

app_info = fetch_edit_app_info(app) || fetch_live_app_info(app)
if app_info
category_id_map = {}

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

# Finding languages to enable
def verify_available_info_languages!(options, app, languages)
app_info = fetch_edit_app_info(app)
# 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)

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