Browse Source

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

Stephen Cefali 4 years ago
parent
commit
308a65ac74

+ 1 - 1
migrations_lockfile.txt

@@ -10,7 +10,7 @@ auth: 0008_alter_user_username_max_length
 contenttypes: 0002_remove_content_type_name
 jira_ac: 0001_initial
 nodestore: 0001_initial
-sentry: 0095_ruleactivity
+sentry: 0096_sentry_app_component_skip_load_on_open
 sessions: 0001_initial
 sites: 0002_alter_domain_unique
 social_auth: 0001_initial

+ 2 - 0
src/sentry/api/validators/sentry_apps/schema.py

@@ -44,6 +44,8 @@ SCHEMA = {
                 "type": {"type": "string", "enum": ["select"]},
                 "label": {"type": "string"},
                 "name": {"type": "string"},
+                "async": {"type": "boolean"},
+                "skip_load_on_open": {"type": "boolean"},
                 "uri": {"$ref": "#/definitions/uri"},
                 "options": {"$ref": "#/definitions/options"},
                 "depends_on": {"type": "array", "minItems": 1, "items": {"type": "string"}},

+ 1 - 1
src/sentry/mediators/sentry_app_components/preparer.py

@@ -54,7 +54,7 @@ class Preparer(Mediator):
             field.update({"choices": field["options"]})
 
         if "uri" in field:
-            if "async" not in field:
+            if not field.get("skip_load_on_open"):
                 field.update(self._request(field["uri"]))
 
     def _request(self, uri):

+ 73 - 0
src/sentry/migrations/0096_sentry_app_component_skip_load_on_open.py

@@ -0,0 +1,73 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.29 on 2020-07-28 22:38
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+#update the field with mutation
+def convert_field(field):
+    # even if async if false, we had a bug where we'd treat it the same as true
+    # so to maintain legacy behavior, we have to replicate that same check when setting skip_load_on_open
+    if "async" in field:
+        field["skip_load_on_open"] = True
+        del field["async"]
+
+
+# updates the schema with mutation
+def update_element_schema(schema):
+    # update all the fields in the schema
+    link = schema.get("link", {})
+    create = schema.get("create", {})
+
+    for field in link.get("required_fields", []):
+        convert_field(field)
+
+    for field in link.get("optional_fields", []):
+        convert_field(field)
+
+    for field in create.get("required_fields", []):
+        convert_field(field)
+
+    for field in create.get("optional_fields", []):
+        convert_field(field)
+
+def update_ui_components(apps, schema_editor):
+    SentryAppComponent = apps.get_model("sentry", "SentryAppComponent")
+    for component in SentryAppComponent.objects.filter(type="issue-link").select_related("sentry_app"):
+        # need to update the denormalized data
+        update_element_schema(component.schema)
+        for element in component.sentry_app.schema.get("elements", []):
+            # only update issue link elements
+            if element.get("type") == "issue-link":
+                update_element_schema(element)
+
+        # save the UI component and the sentry app
+        component.save()
+        component.sentry_app.save()
+
+
+
+class Migration(migrations.Migration):
+    # This flag is used to mark that a migration shouldn't be automatically run in
+    # production. We set this to True for operations that we think are risky and want
+    # someone from ops to run manually and monitor.
+    # General advice is that if in doubt, mark your migration as `is_dangerous`.
+    # Some things you should always mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that
+    #   they can be monitored. Since data migrations will now hold a transaction open
+    #   this is even more important.
+    # - Adding columns to highly active tables, even ones that are NULL.
+    is_dangerous = False
+
+    # This flag is used to decide whether to run this migration in a transaction or not.
+    # By default we prefer to run in a transaction, but for migrations where you want
+    # to `CREATE INDEX CONCURRENTLY` this needs to be set to False. Typically you'll
+    # want to create an index concurrently when adding one to an existing table.
+    atomic = False
+
+
+    dependencies = [
+        ('sentry', '0095_ruleactivity'),
+    ]
+
+    operations = [migrations.RunPython(update_ui_components, migrations.RunPython.noop)]

+ 2 - 1
src/sentry/static/sentry/app/components/group/sentryAppExternalIssueForm.tsx

@@ -27,6 +27,7 @@ type FieldFromSchema = Omit<Field, 'choices' | 'type'> & {
   uri?: string;
   depends_on?: string[];
   choices?: Array<[any, string]>;
+  async?: boolean;
 };
 
 type Config = {
@@ -292,7 +293,7 @@ export class SentryAppExternalIssueForm extends React.Component<Props, State> {
     const extraProps = field.uri
       ? {
           loadOptions: (input: string) => this.getOptions(field, input),
-          async: true, //TODO: make configurable
+          async: typeof field.async === 'undefined' ? true : field.async, //default to true
           cache: false,
           onSelectResetsInput: false,
           onCloseResetsInput: false,

+ 23 - 0
tests/sentry/api/validators/sentry_apps/test_issue_link.py

@@ -27,6 +27,19 @@ class TestIssueLinkSchemaValidation(TestCase):
                 "required_fields": [
                     {"type": "text", "name": "title", "label": "Title"},
                     {"type": "text", "name": "description", "label": "Description"},
+                    {
+                        "type": "select",
+                        "uri": "/sentry/tasks/projects",
+                        "name": "project_id",
+                        "label": "Project",
+                    },
+                    {
+                        "depends_on": ["project_id"],
+                        "type": "select",
+                        "uri": "/sentry/tasks/boards",
+                        "name": "board_id",
+                        "label": "Board",
+                    },
                 ],
                 "optional_fields": [{"type": "text", "name": "owner", "label": "Owner"}],
             },
@@ -87,3 +100,13 @@ class TestIssueLinkSchemaValidation(TestCase):
     def test_missing_link_optional_fields(self):
         del self.schema["link"]["optional_fields"]
         validate_component(self.schema)
+
+    @invalid_schema
+    def test_invalid_async_option(self):
+        self.schema["create"]["required_fields"][2]["async"] = "cat"
+        validate_component(self.schema)
+
+    @invalid_schema
+    def test_invalid_skip_load_on_open_option(self):
+        self.schema["create"]["required_fields"][2]["skip_load_on_open"] = "cat"
+        validate_component(self.schema)

+ 11 - 2
tests/sentry/mediators/sentry_app_components/test_preparer.py

@@ -39,7 +39,13 @@ class TestPreparerIssueLink(TestCase):
                     {"type": "select", "name": "bar", "label": "Bar", "uri": "/sentry/bar"}
                 ],
                 "optional_fields": [
-                    {"type": "select", "name": "baz", "label": "Baz", "uri": "/sentry/baz"}
+                    {
+                        "type": "select",
+                        "name": "baz",
+                        "label": "Baz",
+                        "uri": "/sentry/baz",
+                        "skip_load_on_open": True,
+                    }
                 ],
             },
         }
@@ -54,7 +60,10 @@ class TestPreparerIssueLink(TestCase):
 
         assert call(install=self.install, project=self.project, uri="/sentry/bar") in run.mock_calls
 
-        assert call(install=self.install, project=self.project, uri="/sentry/baz") in run.mock_calls
+        assert (
+            not call(install=self.install, project=self.project, uri="/sentry/baz")
+            in run.mock_calls
+        )
 
 
 class TestPreparerStacktraceLink(TestCase):