Skip to content

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

Merged
merged 6 commits into from
Jul 5, 2019

Conversation

MeredithAnya
Copy link
Member

Depends on #13377

Screen Shot 2019-05-24 at 10 35 22 AM

@MeredithAnya MeredithAnya force-pushed the app-platform/integration-features-modal branch from a50ae9f to 144968b Compare June 5, 2019 18:09
@MeredithAnya MeredithAnya force-pushed the app-platform/integration-features-modal branch 4 times, most recently from d732b1a to 10db914 Compare June 21, 2019 18:32
@MeredithAnya MeredithAnya requested a review from mnoble June 21, 2019 20:05
Copy link
Contributor

@mnoble mnoble left a 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.


const {FeatureList, IntegrationFeatures} = featureListHooks[0]();
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

@scefali scefali Jul 3, 2019

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.

Copy link
Member

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

@MeredithAnya MeredithAnya requested a review from scefali July 2, 2019 16:56
@MeredithAnya MeredithAnya force-pushed the app-platform/integration-features-modal branch from 10db914 to eacf461 Compare July 2, 2019 17:17
@MeredithAnya MeredithAnya force-pushed the app-platform/integration-features-modal branch from eacf461 to 260770d Compare July 2, 2019 23:10
Copy link
Contributor

@mnoble mnoble left a 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.

@MeredithAnya MeredithAnya merged commit e873279 into master Jul 5, 2019
@MeredithAnya MeredithAnya deleted the app-platform/integration-features-modal branch July 5, 2019 17:56
jan-auer added a commit that referenced this pull request Jul 9, 2019
* 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)
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 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.

4 participants