Skip to content

fix(ui-components): adds skip_load_on_open field and adds migration#20078

Merged
scefali merged 8 commits intomasterfrom
fix/ui-component-async/API-1169
Aug 12, 2020
Merged

fix(ui-components): adds skip_load_on_open field and adds migration#20078
scefali merged 8 commits intomasterfrom
fix/ui-component-async/API-1169

Conversation

@scefali
Copy link
Copy Markdown
Contributor

@scefali scefali commented Jul 29, 2020

It turns out that the async field we have for UI components does not behave in the way that the docs explain. It turns out the only thing it actually does is skip loading the options when the page first loads (instead, it waits until the user starts typing). This PR fixes this issue by doing the following:

  1. Implements the async field properly in that it controls whether we fetch more options when the user types. In order to maintain legacy behavior, the default value is true.
  2. Creating a new option skip_load_on_open which acts the same was as async did before.
  3. Migrate all instances of async to skip_load_on_open in order to maintain the current form behavior for existing applications.

As far as the migration goes, the SentryAppComponent table is pretty small.

Docs PR to go with this: getsentry/sentry-docs#1930

@scefali scefali marked this pull request as ready for review July 29, 2020 22:04
@scefali scefali requested a review from a team as a code owner July 29, 2020 22:04
@scefali scefali requested a review from MeredithAnya July 29, 2020 22:04
"type": {"type": "string", "enum": ["select"]},
"label": {"type": "string"},
"name": {"type": "string"},
"async": {"type": "boolean"},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you think we could add these options in the test_issue_link test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@MeredithAnya Which test is this? can't find it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

validators/sentry_apps/test_issue_link.py p sure

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@MeredithAnya My bad, I didn't realize it was a file. Yes, I will update it.

@MeredithAnya
Copy link
Copy Markdown
Member

If someone from @getsentry/owners-migrations could give this some 👀 that would be good, I think overall this looks good to me tho

Stephen Cefali added 2 commits July 29, 2020 15:59
Copy link
Copy Markdown
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

I don't know the logic here so can't comment on that, but the migration looks safe, only 64 rows and so should be fine to run on deploy.

@scefali scefali merged commit 308a65a into master Aug 12, 2020
@scefali scefali deleted the fix/ui-component-async/API-1169 branch August 12, 2020 16:11
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants