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

[spaceship] Initial support for updated review submissions flow #19751

Conversation

valerio-castelli
Copy link
Contributor

@valerio-castelli valerio-castelli commented Dec 28, 2021

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've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Apple is introducing a new submission workflow on App Store Connect. Once an account is updated to follow the new flow, Fastlane's current submission methods will not work. This PR contains the initial work done for adding support for new concepts like ReviewSubmission-s and ReviewSubmissionItem-s.

While these endpoints have not been made public from Apple's side, the work done in this PR follows the observed HTTP calls that are made when creating a submission on App Store Connect.

Description

This PR introduces the concepts of ReviewSubmission and ReviewSubmissionItem.

Support is added for:

  • creating a new review submission
  • fetching an existing review submission
  • adding items to an existing review submission (only the App Store version, for now)
  • submitting a review submission
  • cancelling a review submission

The App Store Connect API supports additional concepts, states and operations which have not been included in this PR. In other words, this PR does not aim to provide an exhaustive coverage of the functionalities connected to the new submission flow; rather, its goal is to enable the development of a version of Deliver that is compliant with the new submission flow and that can be used when Apple eventually pulls the trigger and enforces it on all developer accounts.

Note that these new classes have not been integrated in Deliver yet. This was a conscious choice, as the new submission flow allows users to batch contributions of different kinds (an app version update, a new custom product page, in-app events, etc.); assuming that the submission happens instantaneously as it does now would therefore have been a very opinionated choice, which I think should be taken by the Fastlane community at large.

Rubocop is currently failing due to the increased length of the App class caused by the addition of the new submission methods. While I would tentatively argue that such issue could be resolved by re-issuing the rubocop file, I figured it would have been best to hear the community's opinion on the matter.

Testing Steps

The PR includes unit tests that verify the correctness of the methods and that are executed with the usual bundle exec rspec command. In addition, the flow has been tested locally against my company's App Store Connect account using a slightly twisted (and opinionated) version of Deliver.

@valerio-castelli valerio-castelli changed the title feat: initial support for updated review submissions flow [spaceship] Initial support for updated review submissions flow Dec 28, 2021
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This PR is sooo 💯 Really appreciate the tests and fixtures 😍

It looks like the linter is failing with...

spaceship/lib/spaceship/connect_api/models/app.rb:6:5: C: Metrics/ClassLength: Class has too many lines. [342/320]
    class App ...
    ^^^^^^^^^

You will need to put # rubocop:disable Metrics/ClassLength above the class and # rubocop:enable Metrics/ClassLength below the class

You can see an example of this in https://github.com/fastlane/fastlane/blob/master/scan/lib/scan/options.rb

Thanks! ❤️

@valerio-castelli
Copy link
Contributor Author

Thanks for the review, @joshdholtz! I added the two comment lines to the App class, the linter should be happy now. 👍

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This is 💯 But also like more than that so its like 💯 💯 💯 Thank you so much for adding this but also adding tests 😊

I left a handful of small comments on changing the named arguments for methods but other than this looks good! I didn't test all of the functionality yet but I did create a submission, added a version, submitted it, and cancelled it and it worked great!

spaceship/lib/spaceship/connect_api/models/app.rb Outdated Show resolved Hide resolved
spaceship/lib/spaceship/connect_api/tunes/tunes.rb Outdated Show resolved Hide resolved
spaceship/lib/spaceship/connect_api/tunes/tunes.rb Outdated Show resolved Hide resolved
spaceship/lib/spaceship/connect_api/tunes/tunes.rb Outdated Show resolved Hide resolved
spaceship/lib/spaceship/connect_api/tunes/tunes.rb Outdated Show resolved Hide resolved
Comment on lines 422 to 424
def get_ready_review_submission(client: nil, platform: nil, includes: nil)
client ||= Spaceship::ConnectAPI
platform ||= Spaceship::ConnectAPI::Platform::IOS
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_ready_review_submission(client: nil, platform: nil, includes: nil)
client ||= Spaceship::ConnectAPI
platform ||= Spaceship::ConnectAPI::Platform::IOS
def get_ready_review_submission(client: nil, platform:, includes: nil)
client ||= Spaceship::ConnectAPI

Thoughts on requiring platform and removing the default? It got me in trouble and confused when testing this on a Mac app 😛 I know we do this in other places in spaceship but I think I'd like to get away from this if possible 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've accepted all your suggestions for mandatory fields! 😀

spaceship/lib/spaceship/connect_api/models/app.rb Outdated Show resolved Hide resolved
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you sooooo much for adding this 😍 Really really love the tests 🚀

@joshdholtz joshdholtz merged commit 5931981 into fastlane:master Jan 13, 2022
Copy link

@fastlane-bot fastlane-bot left a comment

Choose a reason for hiding this comment

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

Congratulations! 🎉 This was released as part of fastlane 2.200.0 🚀

@valerio-castelli
Copy link
Contributor Author

@joshdholtz Just a heads up that Apple plans to start enforcing the new submission flow from January 25th, so Deliver will most likely start to fail (at least for some people) unless we integrate this new logic. This is what they say on App Store Connect:

Last year, we introduced an updated way to submit apps, in-app events, custom product pages, and product page optimization tests for review. Starting January 25, 2022, the submission experience will be automatically updated for all App Store Connect accounts.

I don't know what the best approach would be to support this within Deliver, especially if we consider that the new submission flow makes it possible to have "asynchronous" or "deferred" submissions (you can create the submission, add items to it, and submit the items in bulk) – which may be against the way Deliver has traditionally operated.

I hope that by letting you know in advance (if you're not already aware of it) we may be able to smoothen the transition when January 25th comes 🙂

@joshdholtz
Copy link
Member

@valerio-castelli I have this started in #19838 😊 I'm going to be finishing it up tomorrow and then validate it whenever the App Store Connect API gets the official support for this 💪

@fastlane fastlane locked and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants