-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
ref(app-platform): Add integration features to SentryAppDetailsModal #13393
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
Conversation
a50ae9f
to
144968b
Compare
d732b1a
to
10db914
Compare
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.
We should add some tests ensuring the installation button is disabled, based on the IntegrationFeature
.
src/sentry/static/sentry/app/components/modals/sentryAppDetailsModal.jsx
Show resolved
Hide resolved
|
||
const {FeatureList, IntegrationFeatures} = featureListHooks[0](); |
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.
I'm not sure what the above code is doing. It looks like HookStore
returns a list of functions (do you know why?). Then we're adding defaultFeatureGateComponents
to it. Then we're calling the first item in that list? Is the push
in case it returns an empty list?
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.
So the defaultFeatureGateComponent
list for if you are just running sentry to make this compatible for on-prem people.
Is the push in case it returns an empty list?
This is so we use the pretty markdown and plan specific information for sentry.io
I forget exactly know how the HookStore
works so maybe @evanpurkhiser can shed light on the 'returns list of functions' question.
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.
@MeredithAnya I agree with @mnoble that the code makes this a bit confusing to understand what's going on, though I see this is used other places in the code base. I'm wondering if there is a clearer way to get the same behavior. Here is my suggestion:
const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
const {FeatureList, IntegrationFeatures} = featureHook();
This also avoids mutating the array that is stored in the HookStore
with key integrations:feature-gates
. The mutation probably does not have any consequences in this case but in general, it's better to avoid mutation unless that is the explicit goal.
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 was my doing.
Yes we push it onto the end in the case that it's empty.
const featureHook = HookStore.get('integrations:feature-gates')[0] || defaultFeatureGateComponents;
This would need to be written as
const featureHook = HookStore.get('integrations:feature-gates')[0] || (() => defaultFeatureGateComponents);
Since we call the hookstore function to get our components back.
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.
I'm not sure what the above code is doing. It looks like HookStore returns a list of functions
This is historically just the convention that we have typically used for hookstore. I think it's not great considering there are actually a lot of times where we do not want a list. In fact, if you look around the code base, I would say a majority of the times we just pop off the first function and call it.
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.
@evanpurkhiser Does it make sense to make a getFirst
method on the HookStore? Probably outside the scope of this PR but if what you are saying is true then getting the full array is just a hassle.
EDIT:
Maybe even have a getFirstAndCall
method so we can take care of calling the method as well.
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.
It would probably make more sense to re-evaluate how we're using hookstore. Not sure how much you've seen it yet, but it's basically a bridge between sentry and getsentry, a way to add frontend features specific to the SASS product
10db914
to
eacf461
Compare
src/sentry/static/sentry/app/components/modals/sentryAppDetailsModal.jsx
Show resolved
Hide resolved
eacf461
to
260770d
Compare
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.
I'd update that one section to what @scefali suggested, get this on staging again, and then I think we're good.
* master: chore: Fix typo errywhere -> everywhere (#13934) ref: (Django 1.9) Bump djangorestframework to 3.0.5 as an intermediate step to get to 3.3.x feat(issueless events): Test eventstream work without groups (#13888) ref: Improve repr of User (#13896) feat(loader): Make the default for new js projects v5 of js sdk (#13327) build: Remove browser-reload flag (#13918) ref(ui): Consolidate server frontend hydration logic (#13868) ref: Refactor user reports to not use Postgres Event (#13904) Test/integration acceptance tests (#13895) feat: Remove ts-jest (#13846) obs(sentry_apps): Add a small metrics increment on processing resource changes. (#13897) enable testing for all orgs (#13903) ref: Update Python SDK to 0.10.0 (#13911) fix: Do not suggest Sentry.Extensions.Logging when ASP.NET Core is used (#13891) Update license year to 2019 ci(travis): Move docker-sentry builds to Dockes Hub autobuild (#13901) ref(app-platform): Add integration features to SentryAppDetailsModal (#13393) chore(ui) Rename OrganizationStream to IssueList (#13892)
Depends on #13377