[WIP][spaceship] add initial App Clips support #19043
Conversation
|
CI failure might be related to this 🤔 |
|
|
||
| module Spaceship | ||
| class ConnectAPI | ||
| class AppClipHeaderImage |
There was a problem hiding this comment.
This class's implementation is almost identical to AppScreenshot and AppPreview but I didn't go refactoring to extract common interfaces for now.
I personally feel like -
- Having the list of
attr_accesorin each class should be fine awaiting_upload?or other helpers functions can be extracted to a module to be shared- Asset uploading logic should be extracted to a class to enforce exactly the same flow for each and write tests but that requires a bit of abstraction since APIs used are different
| def get_app_clip_version_release_with_app_store_version(app_clip_version_id:, filter: {}, includes: nil, limit: nil, sort: nil) | ||
| params = tunes_request_client.build_params(filter: filter, includes: includes, limit: limit, sort: sort) | ||
| tunes_request_client.get("appClipVersions/#{app_clip_version_id}/relationships/releaseWithAppStoreVersion", params) | ||
| end | ||
|
|
||
| def get_app_clip_app_store_review_detail(app_clip_version_id:, filter: {}, includes: nil, limit: nil, sort: nil) | ||
| params = tunes_request_client.build_params(filter: filter, includes: includes, limit: limit, sort: sort) | ||
| tunes_request_client.get("appClipVersions/#{app_clip_version_id}/relationships/appClipAppStoreReviewDetail", params) | ||
| end |
There was a problem hiding this comment.
There are those resources related to appClipVersions but didn't add models for them since I have no idea how to use them. (releaseWithAppStoreVersion is appStoreVersion type)
| def get_app_clip_version_localizations(client: nil, filter: {}, includes: Spaceship::ConnectAPI::AppClipVersionLocalization::ESSENTIAL_INCLUDES, limit: nil, sort: nil) | ||
| client ||= Spaceship::ConnectAPI |
There was a problem hiding this comment.
| def get_app_clip_version_localizations(client: nil, filter: {}, includes: Spaceship::ConnectAPI::AppClipVersionLocalization::ESSENTIAL_INCLUDES, limit: nil, sort: nil) | |
| client ||= Spaceship::ConnectAPI | |
| def get_app_clip_version_localizations(client: nil, filter: {}, includes: nil, limit: nil, sort: nil) | |
| client ||= Spaceship::ConnectAPI | |
| includes ||= Spaceship::ConnectAPI::AppClipVersionLocalization::ESSENTIAL_INCLUDES |
There was a problem hiding this comment.
Ah, well, yes. I know what you are saying here.
But I think the default value of keyword arguments is much simpler than ||= idiom to do this same thing honestly😅 client ||= Spaceship::ConnectAPI appears everywhere right now so I just followed it for the sake of consistency but it should have been consistent with using "default value", IMHO. I may clean it up at some point 🤔
(Moreover, I don't fully understand the point to pass a client class type to each method 🤔 )
There was a problem hiding this comment.
@ainame There are people who use these methods with multiple teams at the same time (which means multiple clients). These users are not able to just use the single session on the Spaceship::ConnectAPI for this reason. So these users create individual Spaceship::ConnectAPI::Client instances and pass them into the models to use. It's a very niche feature but something we need to support 🤷♂️
There was a problem hiding this comment.
@joshdholtz Yes, I know that but was just wondering what's the reason behind using multiple sessions🤔 I saw the PR changing it this way but it didn't contain much information, unfortunately.
|
@ainame Thanks a lot for this PR! 🚀 We are currently in the release process, but I can test your branch until the end of the week. |
|
@EmDee Thank you for that! Please note this🙂
|
Thanks, I'll keep that in mind. Will keep you posted. |
|
Hey @ainame, I'm currently trying to test our your implementation. I initially thought that this has already been integrated into the My setup is the following:
lane :adjustAppClip do
app = Spaceship::ConnectAPI::App.find('com.foo.bar')
app_clip = app.get_app_clips.first
app_clip_version = app_clip.get_app_clip_versions.first
app_clip_version.update(attributes: { invocationVerb: Spaceship::ConnectAPI::AppClipVersion::InvocationVerb::OPEN })
app_clip_version.create_app_clip_version_localization(attributes: { locale: 'en', subtitle: 'Test' } )
localizations = app_clip_version.get_app_clip_version_localizations
localizations[0].update(attributes: { subtitle: 'Foo' })
localizations[1].update(attributes: { subtitle: 'Bar' })
app_clip_header_image = Spaceship::ConnectAPI::AppClipHeaderImage.create(app_clip_version_localization_id: localizations[0].id, path: 'metadata/preview.png')
app_clip_header_image.delete!
endI think I'm missing the web session, therefore no apps can be found, but I'm not entirely sure how to initialize the session here. Pointers? |
|
@EmDee Thank you for giving it a try🙂
Well, that's the future plan. This PR hasn't covered that yet.
Could you please share the stack trace (backtrace) of that error? Isn't that "undefined method get_app_clips for nil:NilClass"...? If so make sure you get the right value from |
I tried prepending the lane with Do you see any other problems in my lane script? Or how did you test the retrieval of the app clips information? |
|
@EmDee |
Just to clarify, the I feel like I'm doing something fundamentally wrong here 🙈 |
|
Hmm🤔 This page is a bit old but you can test the pieces of code I shared on the playground opened by "bundle exec fastlane spaceship". You may want to give it a try? https://github.com/fastlane/fastlane/blob/master/spaceship/README.md#playground Either way your issue feels like it's not related to my change though. |
Awesome! I only had a quick chance to play around with the playground, but was already able to retrieve the App Clips. I'm currently ob the road, but will try out later today again. But already looks promising :) |
|
@ainame I was able to test this further. With the spaceship playground I was able to run all of your commands. The response seems a bit strange though. When I call For example this is what the API is returning: But ASC is showing the following:
Note the German subtitle for example. The API is returning "@subtitle="Jetzt mobil bezahlen">,", whereas ASC shows "Jetzt hier mobil bezahlen". So I don't even know where the API is getting that information from? But it sounds like an old version of an advanced app clip that we once had (maybe). Other than that, I'm running into the same problem as you. In case an advanced experience is set, updating some (or all) property won't work. |
|
@EmDee Thank you for your additional report! Much appreciated.
My guess is that |
Just in case you're missing a domain to host the AASA file, I will gladly host it for you! |
Oh, thanks. But I did host it by myself but didn't work. I feel like I need to go through all of the related WWDC sessions😄 |
@ainame Assuming your ID is correct, I think the problem lies in your AASA's content-type. It should be In case you didn't know, Apple has a tool to verify your AASA: https://search.developer.apple.com/appsearch-validation-tool/
|
|
@EmDee Thanks. I fixed Content-Type header but it's still invalid. It looks like my AASA file won't be valid unless my test app is published in App Store. That won't happen after all as I used Apple's Fruta demo app for testing😂 Unfortunately, I don't have any public app personally right now 😞 |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
Just rebased my branch🙂 At my first look, I think I need to change the plan in this PR to support the latest and public App Store Connect APIs. This PR's change did depend on the internal APIs but the API design has been completely changed on the published APIs. Those endpoints are now diverged to support both "default experience" and "advanced experience". So some additions in this PR don't make sense any more. Since the specs are revealed now, my plan is to add supports for new endpoints little by little over a couple of PRs so that each PR review will be easy. I'll leave only things survived and remove rest in this PR to get merged! We'll get complete APIs support at some point and then might integrate spaceship-level App Clips support with deliver or something like a new action hopefully 🤞 |
ainame
changed the title
|
@ainame You can hit me up, if you need someone to test your implementation |
|
I'm going to revise |
|
@ainame Hello. Any news here..?( |
Checklist
bundle exec rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validMotivation and Context
#17639
We got no App Clips support in the official App Store Connect API from WWDC21 so I came to start supporting it with the web session one.
Description
I have added the following four new models to support the basic settings of App Clips' metadata on App Store Connect console, which represents the information on the screenshot.
JFYI: I haven't looked into things that can be edited within "Edit Advanced Experiences" pages.
Testing Steps
As the above says, to see the App Clip section enabled on App Store Connect, you would need to follow these.
Then you'll see the section enabled, which means
appClipsandappClipVersionsresources are created internally already. You can play APIs with the spaceship playground command, like below.Note that this currently works with only the web session. App Store Connect API key wont' work for this.
FYI: Apparently, once you change things within "Edit Adavanced Experiences", you won't be able to modify some attributes via
AppClipVersion#update🤔