-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fix(appstore-connect): Return more detailed errors when API credentials are unusable during setup #29808
Conversation
@priscilawebdev heads up that this doesn't make any changes to the FE, but a previously partially unused case in the FE will now be used once these changes are in. |
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.
Nice!
try: | ||
apps = appstore_connect.get_apps(session, credentials) | ||
except appstore_connect.UnauthorizedError: | ||
raise AppConnectAuthenticationError() |
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.
python oddity, you don't actually have to instantiate the class you want to "raise". If you pass a type to raise
it will instantiate it for you. But there's no harm in doing this either so doesn't really matter.
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 is probably the second or third time i've done this in a PR, thanks for pointing this out 🙃
142c12d
to
1e7fa19
Compare
0f90a60
to
d154ad1
Compare
d154ad1
to
e2a683a
Compare
When setting up credentials the only feedback that's returned when they're wrong (invalid, not enough perms) is this:
This changes things so that the user will see the following two errors instead based on what App Store Connect tells us:
Front end work was already completed as a part of #29728 since there's some shared FE code between these two PRs.
I don't believe there should be any issues deploying this change as the FE already is capable of handling what this endpoint returns prior to and after this change.