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

Remove deprecated attributes from apps requests #21187

Merged
merged 4 commits into from Apr 16, 2023

Conversation

giginet
Copy link
Collaborator

@giginet giginet commented Apr 6, 2023

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

closes #21125

This PR fixes this issue.
Listing apps fails, due to the recent pricing update on App Store? · Issue #21125 · fastlane/fastlane

Description

Since App Store Connect API 2.3(released on March 2023), App.prices has been deprecated.
https://developer.apple.com/documentation/appstoreconnectapi/app/relationships/prices

After this version, the following 409 error is returned when requested with includingprices attributes.

The resource 'appPrices' cannot be viewed, created or updated. Please view and create 'manualPrices' using the resource 'appPriceSchedules'.

However, prices are always included as an essential attribute.

This PR removes prices from ESSENTIAL_INCLUDES.

Testing Steps

  1. Enable scheduled prices for your app on AppStore Connect
  2. Execute the following lane
lane :app_version do
  app_store_connect_api_key 
  app_store_build_number
end

Discussion

This PR is just a hotfix. We need to support the new price API to fetch prices.

The current response mock is old. So we have to update this to follow the latest API spec.

@@ -54,12 +54,12 @@ module EducationDiscountType
"contentRightsDeclaration" => "content_rights_declaration",

"appStoreVersions" => "app_store_versions",
# This attribute is already deprecated. It will be removed in a future release.
"prices" => "prices"
Copy link
Collaborator Author

@giginet giginet Apr 6, 2023

Choose a reason for hiding this comment

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

I have no idea whether we can drop this attribute.

We have to migrate for a latest API

Choose a reason for hiding this comment

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

As I see, old applications continue working with old attributes, so it is better to leave it, for backward compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, this attribute should be removed as with this PR's changes it will always end up being nil. Part of the code seeing the nil might think this app has no price even though it might have prices set up in App Store Connect, ending up with confusion like in #21213.
Not having the attribute would catch possible misuses.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I might have misunderstood part of the code, as it seems app data can come from different locations, some with or without prices. The current state is pretty confusing 😅

@Michelalmeidasilva
Copy link

@joshdholtz

Is this a pr that can be approved?

Apparently fixes the bug mentioned in PR #21187

@tukestolo
Copy link

please guys can you speed up this ? 🙏

@giginet
Copy link
Collaborator Author

giginet commented Apr 16, 2023

As discussed on #21125, this patch is not enough to support all changed APIs (e.g. upload_price_tier).

This patch works correctly for a specific use case. However, we need to support the latest API to fix all issues.
But it takes a much time to implement this.

@joshdholtz How do you think about these issues?

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.

Thanks for the fix! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Listing apps fails, due to the recent pricing update on App Store?
7 participants