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
Changes from 1 commit
c976e9a
23ea7c0
9453b77
d57b974
d62af0d
7d42af5
c1bdd94
f5187ff
dfbf80d
d04fb08
aae5bdd
5c161ef
66470dc
e016f01
896c292
89086aa
52bbce7
09f89e3
104ecdb
0cc2bfc
0e62085
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
|
@@ -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 = {} | ||||||
|
||||||
|
@@ -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 | ||||||
|
@@ -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) | ||||||
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") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd use either quoted an capitalized There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've pushed a commit to change to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
What I think we could use now, after the latest changes to the logic:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WFM 👍 |
||||||
|
There was a problem hiding this comment.
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:
updating_app_info?
is the Ruby naming convention for functions that return boolean values (instead of e.g.is…(…)
) 😃There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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 ❤️