fix(appstore-connect): Add a new endpoint that returns the status of all configured apps [NATIVE-294] #29728

relaxolotl · 2021-11-03T01:51:32Z

@ potential reviewers: i would really recommend reviewing this commit-by-commit, or to at least review the code without 3116b92 included as it performs a rename that only clutters the diff

This creates a new endpoint appstoreconnect/status meant to replace appstoreconnect/validate/{id}/.

This new endpoint does the following things differently from its predecessor:

Returns more detailed diagnostics if API keys are invalid
Returns statuses of all apps(/sources) configured for a project if multiple apps(/sources) are found for the project

The following frontend changes have been made in response:

Parses the aforementioned more detailed diagnostics and shows more specific messages accordingly during update
Handles lists of statuses being returned properly
Since since the detailed diagnostics are going to be reused when validating the API keys when creating a new app(/source), some preliminary work has been done to use the same code to handle those diagnostics responses for both the /validate and the /apps endpoints.

Screenshots

When submitting invalid credentials (changed from "An unexpected error occurred while configuring...")

Inline warning

Context shown after clicking on inline warning

github-actions · 2021-11-03T01:53:44Z

size-limit report

Path	Base Size (61ccbff)	Current Size	Change
src/sentry/static/sentry/dist/entrypoints/app.js	52.77 KB	52.8 KB	+0.06% 🔺
src/sentry/static/sentry/dist/entrypoints/sentry.css	70.89 KB	70.89 KB	0%

static/app/types/debugFiles.tsx

src/sentry/api/endpoints/project_app_store_connect_credentials.py

relaxolotl · 2021-11-03T01:58:28Z

i need to get a few test api keys set up to see what apple returns in a few easy-to-reproduce situations.

relaxolotl · 2021-11-04T06:17:07Z

src/sentry/api/endpoints/project_app_store_connect_credentials.py

+            except appstore_connect.UnauthorizedError:
+                asc_credentials = {
+                    "status": "invalid",
+                    "code": AppConnectAuthenticationError().code,


I plan to throw these errors directly in /app so the FE can use the same code to handle the validation messages returned by this endpoint as well as /status

I'm not sure you need to instantiate the class here, can you not access the code attribute without instantiating?

you're right about that, thanks for catching this!

relaxolotl · 2021-11-04T06:19:38Z

static/app/types/debugFiles.tsx

@@ -1,3 +1,5 @@
+import {ValidationErrorDetailed} from 'app/components/modals/debugFileCustomRepository/appStoreConnect/utils';


this doesn't introduce a circular import but i'm not particularly confident about this import. maybe it's better to just duplicate the type here instead?

should we move the ValidationErrorDetailed type and maybe add some AppStoreConnect prefix or rename it?

do you have any ideas on where ValidationErrorDetailed could be moved to? i'm not too familiar with the front end's file structure so hints here would be very helpful.

please do not worry... I worked on this in the PR #29843

static/app/components/projects/appStoreConnectContext/utils.tsx

relaxolotl · 2021-11-04T06:21:50Z

@ potential reviewers: i would really recommend reviewing this commit-by-commit, or to at least review the code without 3116b92 included as it performs a rename that only clutters the diff

flub

Looks nice!

(but there are no tests :( )

flub · 2021-11-04T09:54:41Z

src/sentry/api/endpoints/project_app_store_connect_credentials.py

+    ```json
+    {
+        "appstoreCredentialsValid": true,
+        "pendingDownloads": 123,


It seems we're just hiding the fact that pendingDownloads can be undefined right after a new source is created. The implementation here queries the database and will return 0 pending downloads if we never ran the task. This could be a tiny bit odd if our workers are a bit slow. But probably not important enough to worry about so I don't mind if we keep it like this.

good catch, i've got something written down right now to investigate this as i've been seeing this behaviour after the switch to using the app store connect API to download dSYMs. i'm currently unsure what the best solution for this would be as it would probably need coordination from the front end, but i'm thinking we can maybe defer the work to address this to later on in the quarter unless we're getting a lot of customer feedback on this.

flub · 2021-11-04T10:06:43Z

src/sentry/api/endpoints/project_app_store_connect_credentials.py

+            except appstore_connect.UnauthorizedError:
+                asc_credentials = {
+                    "status": "invalid",
+                    "code": AppConnectAuthenticationError().code,


I'm not sure you need to instantiate the class here, can you not access the code attribute without instantiating?

src/sentry/api/endpoints/project_app_store_connect_credentials.py

relaxolotl · 2021-11-04T18:32:25Z

fyi @priscilawebdev @flub i've rebased this off of master to resolve the conflicts and to prune some duplicate commits due to a recent merge of a dependency. i moved the rename commit back to the bottom of the commit stack to make it easier to omit it from the diff during review.

relaxolotl · 2021-11-08T18:43:24Z

status endpoint is currently throwing 405s for some reason, will investigate later after lunch

…all configured apps

relaxolotl · 2021-11-09T01:48:12Z

@priscilawebdev i believe i've addressed all of your feedback, so this should be good for a second look. if you're unable to find time to take a second look by tomorrow, i'll merge this in first and let's follow up on any remaining feedback you've got in another PR.

priscilawebdev

looks good to me

priscilawebdev · 2021-11-09T08:06:34Z

static/app/types/debugFiles.tsx

@@ -1,3 +1,5 @@
+import {ValidationErrorDetailed} from 'app/components/modals/debugFileCustomRepository/appStoreConnect/utils';


please do not worry... I worked on this in the PR #29843

static/app/types/debugFiles.tsx

priscilawebdev · 2021-11-09T08:11:34Z

static/app/components/modals/debugFileCustomRepository/appStoreConnect/utils.tsx

+  switch (error.code) {
+    case 'app-connect-authentication-error':
+      return t(
+        'Credentials are invalid or missing. Check the entered App Store Connect credentials are correct.'


vercel bot deployed to Preview – storybook November 3, 2021 01:52 View deployment

relaxolotl commented Nov 3, 2021

View reviewed changes

static/app/types/debugFiles.tsx Show resolved Hide resolved

relaxolotl commented Nov 3, 2021

View reviewed changes

src/sentry/api/endpoints/project_app_store_connect_credentials.py Show resolved Hide resolved

relaxolotl force-pushed the appconnect/extended-validate branch from f66e72c to 54afb07 Compare November 4, 2021 05:59

vercel bot deployed to Preview – storybook November 4, 2021 05:59 View deployment

vercel bot deployed to Preview – sentry November 4, 2021 05:59 View deployment

vercel bot deployed to Preview – sentry November 4, 2021 06:04 View deployment

vercel bot deployed to Preview – storybook November 4, 2021 06:04 View deployment

relaxolotl commented Nov 4, 2021

View reviewed changes

relaxolotl marked this pull request as ready for review November 4, 2021 06:17

relaxolotl requested a review from a team as a code owner November 4, 2021 06:17

relaxolotl requested review from a team and priscilawebdev November 4, 2021 06:17

relaxolotl commented Nov 4, 2021

View reviewed changes

static/app/components/projects/appStoreConnectContext/utils.tsx Outdated Show resolved Hide resolved

flub approved these changes Nov 4, 2021

View reviewed changes

Base automatically changed from appconnect/stop-asking-for-credentials to master November 4, 2021 10:54

vercel bot deployed to Preview – sentry November 4, 2021 18:29 View deployment

vercel bot deployed to Preview – storybook November 4, 2021 18:29 View deployment

relaxolotl force-pushed the appconnect/extended-validate branch from 2fb9eb0 to 549d66b Compare November 4, 2021 18:30

vercel bot deployed to Preview – storybook November 4, 2021 18:32 View deployment

vercel bot deployed to Preview – sentry November 4, 2021 18:32 View deployment

relaxolotl force-pushed the appconnect/extended-validate branch from 549d66b to 3116b92 Compare November 5, 2021 00:10

vercel bot deployed to Preview – storybook November 5, 2021 00:10 View deployment

vercel bot deployed to Preview – sentry November 5, 2021 00:10 View deployment

relaxolotl mentioned this pull request Nov 5, 2021

fix(appstore-connect): Return more detailed errors when API credentials are unusable during setup #29808

Merged

vercel bot deployed to Preview – sentry November 9, 2021 00:09 View deployment

vercel bot deployed to Preview – storybook November 9, 2021 00:09 View deployment

relaxolotl added 15 commits November 8, 2021 20:45

fix(appstore-connect): Add a new endpoint that returns the status of …

30b4997

…all configured apps

return a list of statuses for every config associated with a project

a4b3a5e

frontend support for multiple statuses being returned

7d9492b

complete backend implementation

9bd72a0

full frontend support for new status endpoint

bb58318

no need to instantiate

1027735

this was test code oops

058d684

return a map instead of a list

e008a26

scope list of pending downloads to a specific app/config

f5c4832

s/appstoreconnectvalidationdata/appstoreconnectstatusdata/

85b2b9d

add trailing slash

da31f16

address feedback

8473a41

remove nested ID in payload

d8e494e

just directly check for validity

cc96284

fix endpoint

1e7fa19

relaxolotl force-pushed the appconnect/extended-validate branch from 142c12d to 1e7fa19 Compare November 9, 2021 01:45

vercel bot deployed to Preview – sentry November 9, 2021 01:45 View deployment

vercel bot deployed to Preview – storybook November 9, 2021 01:45 View deployment

priscilawebdev approved these changes Nov 9, 2021

View reviewed changes

priscilawebdev reviewed Nov 9, 2021

View reviewed changes

Swatinem approved these changes Nov 9, 2021

View reviewed changes

relaxolotl merged commit bcfd90a into master Nov 9, 2021

relaxolotl deleted the appconnect/extended-validate branch November 9, 2021 15:17

relaxolotl changed the title ~~fix(appstore-connect): Add a new endpoint that returns the status of all configured apps~~ fix(appstore-connect): Add a new endpoint that returns the status of all configured apps [NATIVE-294] Nov 9, 2021

github-actions bot locked and limited conversation to collaborators Nov 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(appstore-connect): Add a new endpoint that returns the status of all configured apps [NATIVE-294] #29728

fix(appstore-connect): Add a new endpoint that returns the status of all configured apps [NATIVE-294] #29728

relaxolotl commented Nov 3, 2021 •

edited

Loading

github-actions bot commented Nov 3, 2021 •

edited

Loading

relaxolotl commented Nov 3, 2021

relaxolotl Nov 4, 2021

flub Nov 4, 2021

relaxolotl Nov 4, 2021

relaxolotl Nov 4, 2021

priscilawebdev Nov 7, 2021

relaxolotl Nov 8, 2021

priscilawebdev Nov 9, 2021

relaxolotl commented Nov 4, 2021 •

edited

Loading

flub left a comment

flub Nov 4, 2021

relaxolotl Nov 4, 2021 •

edited

Loading

flub Nov 4, 2021

relaxolotl commented Nov 4, 2021

relaxolotl commented Nov 8, 2021

relaxolotl commented Nov 9, 2021

priscilawebdev left a comment

priscilawebdev Nov 9, 2021

priscilawebdev Nov 9, 2021

		@@ -1,3 +1,5 @@
		import {ValidationErrorDetailed} from 'app/components/modals/debugFileCustomRepository/appStoreConnect/utils';

fix(appstore-connect): Add a new endpoint that returns the status of all configured apps [NATIVE-294] #29728

fix(appstore-connect): Add a new endpoint that returns the status of all configured apps [NATIVE-294] #29728

Conversation

relaxolotl commented Nov 3, 2021 • edited Loading

Screenshots

When submitting invalid credentials (changed from "An unexpected error occurred while configuring...")

Inline warning

Context shown after clicking on inline warning

github-actions bot commented Nov 3, 2021 • edited Loading

size-limit report

relaxolotl commented Nov 3, 2021

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

Choose a reason for hiding this comment

relaxolotl commented Nov 4, 2021 • edited Loading

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relaxolotl Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relaxolotl commented Nov 4, 2021

relaxolotl commented Nov 8, 2021

relaxolotl commented Nov 9, 2021

priscilawebdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relaxolotl commented Nov 3, 2021 •

edited

Loading

github-actions bot commented Nov 3, 2021 •

edited

Loading

relaxolotl commented Nov 4, 2021 •

edited

Loading

relaxolotl Nov 4, 2021 •

edited

Loading