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

loremattei · 2023-08-21T22:29:29Z

Checklist

I've run bundle exec rspec from the root directory to see all new and existing tests pass
I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
I've read the Contribution Guidelines
I've updated the documentation if necessary.

Motivation and Context

When an app has an iOS and a macOS implementation, Deliver fails to upload one of the apps if the other one is under review:

ASC returns no edit_app_info if one of the app is submitted for review and this makes Deliver fail.
Uploading only platform specific info for the other platform is actually a valid operation in ASC. Deliver fails because it can't fetch edit_app_info data, not because it can't upload the new data.
Since uploading only platform specific info is a valid flow in ASC, Deliver should allow to perform it.

Resolves #17321
Resolves #21003

Description

This PR updates update_metadata so that it fetches live_app_info if it can't find edit_app_info.

If no app is under review, edit_app_info will be fetched, so no changes from the current behaviour for most cases.
If one platform is under review, fetching live_app_info as a backup solution will allow the upload flow to continue. The flow will succeed if the user uploads only platform specific metadata, or fail later if they try to upload something that can't be modified in this state.

Testing Steps

To test this specific scenario:

Create an app with an iOS and a macOS implementation.
Submit a new version of the iOS app for review.
When the iOS app is waiting for review, upload metadata (you can also try and upload binary and screenshots as well, but the issue is specific to metadata, so no need to do it) for a new version of the macOS app:

bundle exec fastlane deliver run --force \
  --platform osx \
  --skip_binary_upload true \
  --skip_screenshots true \
  --submit_for_review false \
  --skip_metadata false \
  --automatic_release false \
  --username <your username> \
  --app_identifier <your app identifier>

and verify that it works.

Minimum regression testing:

Test that upload a single iOS app still works.
Test that trying to upload new metadata for an app/platform that has been submitted for review fails.

google-cla · 2023-08-21T22:29:33Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

rogerluan · 2023-08-25T03:57:37Z

Hi @loremattei 👋 could you merge master into your branch (or rebase it)? There was a fix to our CI that is in master that should make your PR pass all checks 🙏

Thank you!

loremattei · 2023-08-25T08:42:37Z

Thanks @rogerluan! I rebased and now CI checks are all green :-)

rogerluan

Hi @loremattei 😊 Thanks for fixing this!

Could you add comments to the code explaining why we fallback to the live version, and why it is ok to use the live version? This edge case of having both iOS + macOS apps is not straightforward to understand when reading the code 😅

Also, could you test the case where all apps (macOS + iOS) are in non-edit mode? What message is it shown to the user? 🤔 If no useful error message is displayed, are we able to handle this scenario in our code and display a more user-friendly message?

loremattei · 2023-08-28T22:03:56Z

Hi @rogerluan!
Thanks for the feedback! Good points!
I've added some comments to the code.

I think there are different cases where all apps (macOS + iOS) are in non-edit mode and the failure likely depends on the requested flow (i.e. on the options). As long as the fetch step passes, the failure depends on whether the requested action is allowed.

I've tested the case where no apps is in edit mode and the user tries to upload new metadata without passing any specific version as it looked the one which could be troublesome and it returns Failed to upload metadata: The provided entity includes an attribute with a value that has already been used - The version number has been previously used. which looks pretty explanatory.

rogerluan

Thanks for adding those comments! 🙌

However, I don't think this message:

The provided entity includes an attribute with a value that has already been used - The version number has been previously used.

is intuitive enough 😬 working with fastlane for a few years and knowing the specific jargon Apple's APIs use, I kinda understand why it's failing, but not what I need to do to fix it, or what's wrong, if it's an issue on my side or not, etc... now if this was presented to a user with less experience, they'd probably be lost 😢

Any chances we can catch this error and provide a custom-generated message to the user? Either before the request is actually made (validate before sending), or after (handling the error)?

If we can find a hook to do that, then a good message candidate would be e.g.:

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.

This would be a helpful and user-friendly message, which is what I'd expect as a user 😃

The challenge isn't the message though, it's finding the right hook to add this to 🙈

rogerluan · 2023-08-29T04:16:52Z

deliver/lib/deliver/upload_metadata.rb

+      # 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.


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.

Same for the comment you left below

loremattei · 2023-08-30T17:03:11Z

Thanks @rogerluan!

I don't think this message is intuitive enough

I see what you mean and I agree with your proposal. My point was mostly that we are not introducing any big regression here as the current error message ("Cannot update languages - could not find an editable info") is not drastically more intuitive when users don't have any app in an editable state.

Anyway, I'm happy to try and improve this solution. I took a deeper look at the API and I think I've found a suitable approach, but I'll need some more time to figure it our properly. I'll be back asap.

rogerluan · 2023-08-30T18:32:53Z

the current error message ("Cannot update languages - could not find an editable info") is not drastically more intuitive when users don't have any app in an editable state.

Fully agree 💯 😅 the point isn't which custom message we show though, but the fact that we show a custom one, instead of the API's response. If we replace Cannot update languages - could not find an editable info with a 🌷, how can we show 🌷 to user, on error? That's the challenge here I think, since we pretty much removed the hook that we were using to display such message.

I hope your approach works 🙌 looking forward to reviewing it!

loremattei · 2023-08-30T22:20:50Z

@rogerluan I updated this branch with a first draft. I haven't fully tested it yet, but I wanted to validate the idea with you before committing more time to it.

While investigating better the API and the current Fastlane implementation, I found that Version info handling per platform is already working correctly and the problem is with App Info. App Info can't be actually edited when one platform is under review, so I think the correct behaviour in that scenario is that the upload fails if the user tries to update App Info, but succeeds otherwise.

The current implementation of update_metadata.rb always tries to fetch App Info in verify_available_info_languages, even if it doesn't need it later. fetch_edit_app_info fails if one platform has been submitted, as expected given that it's platform independent.

I think that the right solution here is to only execute verify_available_info_languages if the user is trying to update App Info.

The rest of update_metadata.rb already handles the case where fetch_edit_app_info returns nil without failing, so it looks like the only point that need to be fixed is verify_available_info_languages.

Wdyt? Does this solution look correct to you and worth more investigation and testing?

rogerluan

Love these findings @loremattei !! I think you're definitely on the right track now, but there are some open questions that need more digging so we can cover scenarios where users aren't explicitly passing those arguments 😬

Good stuff so far! 🙌

deliver/lib/deliver/upload_metadata.rb

rogerluan · 2023-08-31T00:22:20Z

deliver/lib/deliver/upload_metadata.rb

@@ -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)


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…(…)) 😃

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

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

…mode.

loremattei · 2023-09-01T20:42:30Z

@rogerluan this should be ready for another look.
I've tested it in a few scenarios and it looks good, though there are actually a bunch of different ones that are possible given all the options that fastlane offers 😅

rogerluan

Good job you did here so far! 🙌 Your algo looks solid to me!
Just left some questions and minor suggestions :) but I think we could use some more rspecs 👀

deliver/lib/deliver/upload_metadata.rb

deliver/spec/upload_metadata_spec.rb

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

deliver/lib/deliver/upload_metadata.rb

deliver/spec/upload_metadata_spec.rb

rogerluan · 2023-09-05T02:57:56Z

deliver/spec/upload_metadata_spec.rb

+
+          expect(version).to receive(:update).with(attributes: {})
+
+          # Get app info localization English (Used to compare with data to upload)


Does the new code work as expected if I change the info in just 1 language and that language happens to not be English or the primary language?

Suggested change

# Get app info localization English (Used to compare with data to upload)

# Get app info localization in English (used to compare with data to upload)

Yes, it does. I used English in tests as it's the language used in the other tests, but the new code goes through all languages and doesn't rely on default or English in particular for anything.

deliver/spec/upload_metadata_spec.rb

rogerluan

Latest changes look good @loremattei ! just left a some feedbacks and a question 😊 once those are addressed I think this will be clear from my side! Good hustle! 💪

rogerluan · 2023-09-05T03:06:08Z

This PR could use a second pair of 👀 as it touches a critical code path 🙇 @AliSoftware @getaaron

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

loremattei · 2023-09-05T07:58:24Z

Thank you for all your support with this @rogerluan!

AliSoftware

Hey Lorenzo! Small world 😄 Fancy seeing you here!

The changes in your PR make sense to me, all looking good, thanks for the contribution! 👍

I just left some nitpicky suggestions about wording, but nothing blocking 🙂

deliver/lib/deliver/upload_metadata.rb

AliSoftware · 2023-09-05T09:03:09Z

deliver/lib/deliver/upload_metadata.rb

+        UI.important("Can't find edit or live App info. Skipping upload.")
+        return false
+      end
+      localizations = app_info.get_app_info_localizations


[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)

deliver/lib/deliver/upload_metadata.rb

AliSoftware · 2023-09-05T09:07:01Z

deliver/spec/upload_metadata_spec.rb

+          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("")


💭 rubocop might complain if you use double-quotes while you don't need any interpolation

Suggested change

expect(app_info_localization_en).to receive(:name).and_return("")

expect(app_info_localization_en).to receive(:name).and_return('')

💡 Also, might make it clearer and easier to understand the logic of the spec to provide a dummy name instead? Or was it intentional and important to the test that this was blank?

Suggested change

expect(app_info_localization_en).to receive(:name).and_return("")

expect(app_info_localization_en).to receive(:name).and_return('Old App Name')

💭 rubocop might complain if you use double-quotes while you don't need any interpolation

The rubocop rules we have in fastlane explicitly allows both " and ' to be used whenever 😅

AliSoftware · 2023-09-05T09:19:06Z

deliver/lib/deliver/upload_metadata.rb

+    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")


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")

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.

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

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

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

deliver/lib/deliver/upload_metadata.rb

Co-authored-by: Olivier Halligon <olivier@halligon.net>

loremattei · 2023-09-06T10:44:54Z

Hey @AliSoftware !
How are you doing? Great to see you!
Thanks for your feedback. I made the changes you suggested and left a comment about the last one. Let me know what you think!

loremattei · 2023-09-08T10:18:56Z

@rogerluan I think I've done everything requested here. Please, let me know if there's anything I need to do to move this forward :-)

deliver/lib/deliver/upload_metadata.rb

deliver/spec/upload_metadata_spec.rb

rogerluan · 2023-09-10T20:07:57Z

deliver/spec/upload_metadata_spec.rb

+          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("")


💭 rubocop might complain if you use double-quotes while you don't need any interpolation

The rubocop rules we have in fastlane explicitly allows both " and ' to be used whenever 😅

rogerluan

💪 LGTM!

deliver/spec/upload_metadata_spec.rb

loremattei · 2023-09-27T16:16:49Z

Hey @rogerluan 👋 Is there anything I can do to move this forward?

rogerluan · 2023-09-29T05:18:49Z

Thanks for the ping! Gonna merge this in, but it will only be released whenever we have the next release cut off 🙇

loremattei · 2023-10-02T11:20:08Z

Thanks @rogerluan! Sounds great! Looking forward for it :-)

…ing scenario of having both macOS and iOS apps present (fastlane#21472)" This reverts commit c4ed1a4.

owjsub · 2023-11-21T16:13:51Z

deliver/lib/deliver/upload_metadata.rb

+        end
+      end
+
+      UI.message('No changes to localized App Info detected. Skipping upload.')


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?

cc: @loremattei @AliSoftware @rogerluan @kommen

loremattei force-pushed the master branch from 638e096 to fbdfd35 Compare August 22, 2023 07:50

Fetch live app info if no edit info is present

c976e9a

loremattei force-pushed the master branch from fbdfd35 to c976e9a Compare August 25, 2023 08:06

rogerluan requested changes Aug 27, 2023

View reviewed changes

rogerluan changed the title ~~Upload Metadata: Fetch live app info if no edit info is present~~ [deliver] fetch live app info if no edit info is present, fixing scenario of having both macOS and iOS apps present Aug 27, 2023

rogerluan added the tool: deliver label Aug 27, 2023

Add comments to explain the fallback

23ea7c0

loremattei force-pushed the master branch from cb38b28 to 23ea7c0 Compare August 28, 2023 21:50

rogerluan requested changes Aug 29, 2023

View reviewed changes

loremattei added 2 commits August 30, 2023 23:53

Check App Info localization only when new metadata contains App Info

9453b77

Fix lint issues

d57b974

rogerluan requested changes Aug 31, 2023

View reviewed changes

loremattei added 3 commits September 1, 2023 21:51

Only fail upload when trying to update localised app info in no edit …

d62af0d

…mode.

Update tests

7d42af5

Make rubocop happy

c1bdd94

rogerluan requested changes Sep 2, 2023

View reviewed changes

loremattei and others added 6 commits September 3, 2023 22:33

Update deliver/lib/deliver/upload_metadata.rb

f5187ff

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Update deliver/lib/deliver/upload_metadata.rb

dfbf80d

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Update deliver/lib/deliver/upload_metadata.rb

d04fb08

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Update message severity

aae5bdd

Minimize api call to fetch app infos

5c161ef

Add tests

66470dc

rogerluan reviewed Sep 5, 2023

View reviewed changes

Fix simple typos.

e016f01

rogerluan reviewed Sep 5, 2023

View reviewed changes

rogerluan requested review from AliSoftware and getaaron September 5, 2023 03:05

loremattei and others added 2 commits September 5, 2023 09:47

Refactor to more Ruby-like syntax

896c292

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Update tests

89086aa

AliSoftware approved these changes Sep 5, 2023

View reviewed changes

AliSoftware reviewed Sep 5, 2023

View reviewed changes

deliver/lib/deliver/upload_metadata.rb Show resolved Hide resolved

loremattei and others added 3 commits September 6, 2023 12:29

Update spelling for consistency

52bbce7

Co-authored-by: Olivier Halligon <olivier@halligon.net>

Update spelling for consistency

09f89e3

Co-authored-by: Olivier Halligon <olivier@halligon.net>

Address feedback

104ecdb

rogerluan reviewed Sep 10, 2023

View reviewed changes

Update user-facing message.

0cc2bfc

rogerluan approved these changes Sep 10, 2023

View reviewed changes

rogerluan reviewed Sep 10, 2023

View reviewed changes

deliver/spec/upload_metadata_spec.rb Outdated Show resolved Hide resolved

Fix syntax of string quotes.

0e62085

rogerluan merged commit c4ed1a4 into fastlane:master Sep 29, 2023
10 checks passed

kronenthaler pushed a commit to kronenthaler/fastlane that referenced this pull request Oct 5, 2023

Revert "[deliver] fetch live app info if no edit info is present, fix…

1843443

…ing scenario of having both macOS and iOS apps present (fastlane#21472)" This reverts commit c4ed1a4.

joshdholtz mentioned this pull request Nov 15, 2023

Version bump to 2.217.0 #21641

Merged

owjsub reviewed Nov 21, 2023

View reviewed changes

rogerluan mentioned this pull request Nov 22, 2023

[deliver] fix regression where changes made to the privacy URL fail to upload #21657

Merged

6 tasks

lacostej mentioned this pull request Feb 23, 2024

Can't update privacy url using deliver by fastlane #21744

Open

4 tasks


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

		# Get app info localization English (Used to compare with data to upload)

	# Get app info localization English (Used to compare with data to upload)
	# Get app info localization in English (used to compare with data to upload)

	expect(app_info_localization_en).to receive(:name).and_return("")
	expect(app_info_localization_en).to receive(:name).and_return('')

	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")

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

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

Conversation

loremattei commented Aug 21, 2023 • edited by rogerluan

Checklist

Motivation and Context

Description

Testing Steps

google-cla bot commented Aug 21, 2023

rogerluan commented Aug 25, 2023

loremattei commented Aug 25, 2023

rogerluan left a comment

Choose a reason for hiding this comment

loremattei commented Aug 28, 2023

rogerluan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loremattei commented Aug 30, 2023

rogerluan commented Aug 30, 2023

loremattei commented Aug 30, 2023

rogerluan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loremattei commented Sep 1, 2023

rogerluan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerluan left a comment

Choose a reason for hiding this comment

rogerluan commented Sep 5, 2023

loremattei commented Sep 5, 2023

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Sep 5, 2023 • edited

Choose a reason for hiding this comment

rogerluan Sep 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Sep 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loremattei commented Sep 6, 2023

loremattei commented Sep 8, 2023

rogerluan Sep 10, 2023 • edited

Choose a reason for hiding this comment

rogerluan left a comment

Choose a reason for hiding this comment

loremattei commented Sep 27, 2023

rogerluan commented Sep 29, 2023

loremattei commented Oct 2, 2023

owjsub Nov 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loremattei commented Aug 21, 2023 •

edited by rogerluan

rogerluan left a comment •

edited

AliSoftware Sep 5, 2023 •

edited

rogerluan Sep 10, 2023 •

edited

AliSoftware Sep 5, 2023 •

edited

rogerluan Sep 10, 2023 •

edited

owjsub Nov 21, 2023 •

edited