Separated Alert to View / Edit / New page components

[ranbena]

• Refactor

Description

By request in comment for separation of mega file, now Alert.jsx renders:

<div className="container alert-page">
  {mode === MODES.NEW && <AlertNew {...commonProps} />}
  {mode === MODES.VIEW && <AlertView canEdit={canEdit} {...commonProps} />}
  {mode === MODES.EDIT && <AlertEdit {...commonProps} />}
</div>

Also, made abstracted the alert name component into:

<Title alert={alert} name={name} onChange={onNameChange} editMode />

[ranbena]

I keep breaking Github @kravets-levko :)

[arikfr]

Why not have each path render its own component? What's the purpose of having a single top level page component? To share the callback functions?

[ranbena]

Yes. Less code duplication
Also allows alert, query and queryResult to be props instead of state, simplifying the page components.

[ranbena]

Also allows this, which imo is more elegant than redirection to view page:

redash/client/app/pages/alert/Alert.jsx

Lines 68 to 71 in 9aeccb4

	// force view mode if can't edit
	if (!canEdit) {
	this.setState({ mode: MODES.VIEW });
	}

[arikfr]

You could use composition instead:

<AlertPage ...>
    <EditAlertPage ...>
</AlertPage>

But the ability to change states without making API request is a win. In the long run, we should find a way to do this without as this pattern isn't always applicable. But here it's fine.

[ranbena]

But the ability to change states without making API request is a win.

Are you sure that's happening here? Changing Alert states mandates url changes which causes page to reload (navigateTo).

[ranbena]

@arikfr it was an easy modification e865460

[arikfr]

Oops, I misunderstood your previous comment. I thought this was what happens when you switch between view and edit. I don't think that just rendering the view page when you hit the edit url is the thing to do. At the very least we should show a message saying you don't have permission to edit. But we could do all that with composition as well. No need for a single component. No need to change it now though, it's not important enough and better to focus on other aspects of the alert PRs.

…

On Thu, Sep 26, 2019, 05:44 Ran Byron ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In client/app/pages/alert/Alert.jsx <#4183 (comment)>: > @@ -383,14 +194,20 @@ export default function init(ngModule) { ngModule.component('alertPage', react2angular(AlertPage)); return routesToAngularRoutes([ + { + path: '/alerts/new', + title: 'New Alert', + mode: MODES.NEW, *But* the ability to change states without making API request is a win. Are you sure that's happening here? Changing Alert states mandates url changes which causes page to reload (navigateTo). — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#4183?email_source=notifications&email_token=AAAROLD2MWMMLCP7W37JZ5DQLQOZHA5CNFSM4I2CWKM2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCF65D5Y#discussion_r328414709>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAROLFCV2GKQZXYTYTR7FLQLQOZHANCNFSM4I2CWKMQ> .

[ranbena]

I don't think that just rendering the view page when you hit the edit url is the thing to do.

That's not the case. The edit button doesn't get rendered if the user can't edit the alert.
It's for the off-chance that an editing user shares an alert url with a non-editing user.

[arikfr]

It's for the off-chance that an editing user shares an alert url with a non-editing user.

I know and still I think it has the potential to confuse people who did expect to edit the alert using this URL. Even showing a notification saying something like "You do not have permission to edit this Alert." is probably enough. Not a huge priority.

[ranbena]

Was also an ez change eecb6d3

As long as you're ok with the prompt being a notification (cause it's easy):

And instead of hiding action buttons they're disabled with a tooltip:

[arikfr]

Merged.