Fixes remaining bugs with multi slack handlers #3482
Conversation
bthesorceror
force-pushed
the
fixes/continue-multiple-slack
branch
from
b35124a to
1fa445c
Compare
| const {error} = data | ||
| if (!error) { | ||
| this.props.notify( | ||
| notifyAlertEndpointSaveFailed(section, 'Cannot save configuration') |
There was a problem hiding this comment.
any reason why it can't save? this is more of a ux thing, but i could see a user not understanding why it couldn't save, so if there's any further specificity here possible, what do you think about adding it?
There was a problem hiding this comment.
we just get a 502 back from Kapa with no additional information.
| </li> | ||
| ))} | ||
| {handlersOnThisAlert.map(endpoint => { | ||
| return ( |
There was a problem hiding this comment.
word. i'm preferring this, much like getting rid of ternaries.
| key={uuid.v4()} | ||
| className={classnames('endpoint-tab', { | ||
| active: | ||
| endpoint.alias === (selectedHandler && selectedHandler.alias), |
There was a problem hiding this comment.
i appreciate this clarification of the var name.
kapacitor.go
Outdated
| @@ -117,6 +117,7 @@ type Slack struct { | |||
| Channel string `json:"channel"` // Slack channel in which to post messages. | |||
| Username string `json:"username"` // Username of the Slack bot. | |||
| IconEmoji string `json:"iconEmoji"` // IconEmoji is an emoji name surrounded in ':' characters; The emoji image will replace the normal user icon for the slack bot. | |||
| Workspace string `json:"workspace"` | |||
There was a problem hiding this comment.
possibly a comment to contextualize?
|
|
||
| constructor(props) { | ||
| super(props) | ||
| this.state = { | ||
| testEnabled: this.props.enabled, | ||
| enabled: _.get(this.props, 'config.options.enabled', false), | ||
| workspace: _.get(this.props, 'config.options.workspace', '') || '', |
There was a problem hiding this comment.
the go struct for this seems to imply without omitempty that it will always be present as the field type's zero-value (''), so is it possible for there to not be a workspace?) could assurances be made on the backend to prevent the need for this _.get and || '' and the _.get on enabled?
There was a problem hiding this comment.
we get back a object that has {workspace: null} at this point so the _.get will just return the null and not the default since the key is present.
There was a problem hiding this comment.
you get null back from the server when it's a string type?
| if (isNewConfig) { | ||
| properties.workspace = this.workspace.value | ||
| properties.workspace = workspace |
There was a problem hiding this comment.
- a bit strange seeing a value on an object being assigned directly -- could a spread operator be used here?
- also in the context of all of these getters, this reads a bit weird to me -- what do you think about
workspace-->this.workspaceand putting workspace behind a getter?
There was a problem hiding this comment.
we only want to send the workspace on create, for updates we want to omit the key altogether.
| if (success) { | ||
| this.setState({testEnabled: true}) | ||
| } | ||
| } | ||
|
|
||
| private handleDelete = async e => { | ||
| e.preventDefault() | ||
| await this.props.onDelete(this.workspace.value, this.workspaceID) | ||
| const {workspace} = this.state |
There was a problem hiding this comment.
see question above about a possible getter
| @@ -73,7 +74,7 @@ class SlackConfigs extends PureComponent<Props, State> { | |||
| /> | |||
| ) | |||
| })} | |||
| {isMultipleConfigsSupported && ( | |||
| {this.isAddAllowed && ( | |||
There was a problem hiding this comment.
it's not clear to me what isAddAllowed would mean, semantically -- would you consider preserving some of the self-awareness around config, such as isConfigAddAllowed?
| private get isAddAllowed(): boolean { | ||
| const {isMultipleConfigsSupported} = this.props | ||
| const {configs} = this.state | ||
| const allExisting = _.every(configs, c => !this.isNewConfig(c)) |
There was a problem hiding this comment.
could you clarify what allExisting refers to, such as allExistingConfigs?
| @@ -99,7 +108,13 @@ class SlackConfigs extends PureComponent<Props, State> { | |||
| private getWorkspace = (config: Config): string => { | |||
| const {isMultipleConfigsSupported} = this.props | |||
| if (isMultipleConfigsSupported) { | |||
| return get(config, 'options.workspace', 'new') | |||
| const workspace = _.get(config, 'options', {workspace: null}).workspace | |||
bthesorceror
force-pushed
the
fixes/continue-multiple-slack
branch
from
1fa445c to
d0ff7a4
Compare
- Alerts not persisting workspace - Error when saving handler with empty nickname - Allow users to add multiple empty rules Co-authored-by: Iris Scholten <[email protected]>
bthesorceror
force-pushed
the
fixes/continue-multiple-slack
branch
from
d0ff7a4 to
0c55409
Compare
Closes #2855