Fixes an issue when deleting an integration from it's detail page

[elvisisking]

• see issue User isn't redirected after deleting integration from integration detail syndesis#5578
• added WithRouter to WithIntegrationActions
• after a successful delete, the integrations list page is displayed

[riccardo-forina]

What if I delete the integration from the dashboard? Will it go to the integrations list?

[elvisisking]

It does now go to the list page @riccardo-forina:( I need to fix that.

[riccardo-forina]

I dunno it’s not that big of a deal I think. @dongniwang what do you think? Is it a problem if users are redirected to the integrations list page after deleting an integration, regardless of where they were? Eg. deleting from the dashboard will take you to the integrations list page.

[riccardo-forina]

PR Storybook available here

[gashcrumb]

I think the redirect should only occur if you're on the detail page. It'd definitely be weird to delete an integration on the dashboard there and then wind up on the integration list page.

And also the angular app only redirected if you were on the detail page, so there's that 😃

[dongniwang]

Yeah, I think it makes sense to delete and return to the integration list when you're on the details page.

But if delete happens from the dashboard, I would expect to see a confirmation dialog then a toast notification saying that the delete was successful.

We (meaning UX) should be better in documenting these interactions. Thanks for fixing the behavior! And I think it's not a big deal that it takes users back to the list page for now.

[elvisisking]

@riccardo-forina @gashcrumb wdyt of this code that conditionally redirects to the integrations list page only when on the integration details page:

if (history.location.pathname.endsWith("details")) {
  history.push(resolvers.list());
}

And might there be a better way to do this?

[riccardo-forina]

Yeah, please no :D

What about adding a postDeleteHref: H.LocationDescriptionObject property to the WithIntegrationActions component, and let the consumers of that component specify the right URL to use for the redirect?

[elvisisking]

Yeah that is much nicer @riccardo-forina . Will do.

[riccardo-forina]

PR Storybook available here

[elvisisking]

While things are working as expected when an integration is deleted (redirect to list when on details page, otherwise stay on same page), I'm getting the following error in the console when the delete action from the kebab menu on the integration DetailsPage and MetricsPage is used:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
    in Fetch (created by Context.Consumer)
    in SyndesisFetch (created by WithChangeListener)
    in WithChangeListener (created by Context.Consumer)
    in Fetch (created by Context.Consumer)
    in SyndesisFetch (created by WithIntegration)
    in WithIntegration (created by WithMonitoredIntegration)
    in WithMonitoredIntegration (at DetailsPage.tsx:46)
    in Route (created by WithRouteData)
    in WithRouteData (at DetailsPage.tsx:44)
    in Translation (at DetailsPage.tsx:40)
    in DetailsPage (created by Context.Consumer)

It seems like the code in ChangeListener.componentWillUnmount() should not let this happen but not sure. I've tried a few things in the details page to fix this but seems like this could be a bigger issue especially when deleting objects from detail pages.

[gashcrumb]

Raise an issue for that and I'll investigate next week, shouldn't be a problem for getting this pr merged I think. Unless you want to look into it further that is.

…

On Fri, Jun 7, 2019, 5:21 PM Daniel Florian ***@***.***> wrote: While things are working as expected when an integration is deleted (redirect to list when on details page, otherwise stay on same page), I'm getting the following error in the console when the delete action from the kebab menu on the integration DetailsPage and MetricsPage is used: Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method. in Fetch (created by Context.Consumer) in SyndesisFetch (created by WithChangeListener) in WithChangeListener (created by Context.Consumer) in Fetch (created by Context.Consumer) in SyndesisFetch (created by WithIntegration) in WithIntegration (created by WithMonitoredIntegration) in WithMonitoredIntegration (at DetailsPage.tsx:46) in Route (created by WithRouteData) in WithRouteData (at DetailsPage.tsx:44) in Translation (at DetailsPage.tsx:40) in DetailsPage (created by Context.Consumer) It seems like the code in ChangeListener.componentWillUnmount() <https://github.com/syndesisio/syndesis/blob/master/app/ui-react/packages/api/src/WithChangeListener.tsx#L48> should not let this happen but not sure. I've tried a few things in the details page to fix this but seems like this could be a bigger issue especially when deleting objects from detail pages. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#424?email_source=notifications&email_token=AACV3LDMMQ3LSJC4XBH4RZDPZLGMPA5CNFSM4HUDDPV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHAY4I#issuecomment-500042865>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACV3LB5VKJYWBZVVHTSQKDPZLGMPANCNFSM4HUDDPVQ> .

[riccardo-forina]

Ooooh it was the Fetch raising the warning, you didn’t t mention that! That’s the fetch promise that will still trigger even if the Fetch component has been unmounted. The problem is that no browser implements abortable fetches, so there is no way to work around this without replacing the browser’s own fetch with some polyfilled version that’s abortable, like whatwg-fetch (which is basically an xhr request wrapped in a fetch compatible interface). Or drop it in favor of something like axios.

Anyway, this isn’t something urgent, the warning is safe to ignore in this case (but should be ultimately addressed)

[riccardo-forina]

PR Storybook available here

[riccardo-forina]

PR Storybook available here

[paoloantinori]

@riccardo-forina with your last comment, are you saying that we can merge this?

[pure-bot]

Pull request approved by @riccardo-forina - applying approved label