fix: error toast message while configuring trigger while creating a monitor #1178
Conversation
…onitor Signed-off-by: vikhy-aws <191836418+vikhy-aws@users.noreply.github.com>
Signed-off-by: vikhy-aws <191836418+vikhy-aws@users.noreply.github.com>
vikhy-aws
requested review from
lezzago,
AWSHurneyt,
amsiglan,
sbcd90,
eirsep,
getsaurabh02,
praveensameneni,
bowenlan-amzn,
rishabhmaurya and
riysaxen-amzn
as code owners
package.json
Outdated
| @@ -34,7 +34,7 @@ | |||
| "@babel/plugin-transform-modules-commonjs": "^7.22.9", | |||
| "@elastic/elastic-eslint-config-kibana": "link:../../packages/opensearch-eslint-config-opensearch-dashboards", | |||
| "@elastic/eslint-import-resolver-kibana": "link:../../packages/osd-eslint-import-resolver-opensearch-dashboards", | |||
| "cypress": "9.5.4", | |||
| "cypress": "12.17.4", | |||
There was a problem hiding this comment.
why are we changing this? Have other plugins updated it as well?
There was a problem hiding this comment.
So, the version of cypress in OpenSearch-Dashboards and the version of cypress in all the plugins should match for the frontend to run. So, set them to the same value.
There was a problem hiding this comment.
LMK if this needs to be updated or not?
| @@ -183,7 +183,10 @@ class ConfigureActions extends React.Component { | |||
| description: '', | |||
| })); | |||
| } else { | |||
| backendErrorNotification(notifications, 'load', 'destinations', response.err); | |||
| // If the config index is not created, don't show the notification | |||
| if (response.totalMonitors !== 0) { | |||
| totalMonitors: 0, | ||
| monitors: [], | ||
| message: "Config index will be created automatically when the monitor is created" |
There was a problem hiding this comment.
I would rather keep the schema same and return a known error message which we can check for in the frontend instead of introducing a new field totalMonitors
There was a problem hiding this comment.
totalMonitors is not a new field, it's been already used in some places. I'm just reusing the same to handle exceptions.
|
|
||
| beforeEach(() => { | ||
| mockClient = jest.fn(); | ||
|
|
There was a problem hiding this comment.
Generally we don't add empty lines between initializations
Signed-off-by: vikhy-aws <191836418+vikhy-aws@users.noreply.github.com>
Signed-off-by: vikhy-aws <191836418+vikhy-aws@users.noreply.github.com>
| @@ -182,8 +182,9 @@ class ConfigureActions extends React.Component { | |||
| type: toChannelType(destination.type), | |||
| description: '', | |||
| })); | |||
| } else { | |||
| backendErrorNotification(notifications, 'load', 'destinations', response.err); | |||
| } else if (response.totalMonitors !== 0) { | |||
There was a problem hiding this comment.
Am hazy on my JS but are you intending to check response.totalMonitor > 0
if yes, why are we checking for >0 and then code comment says config index not created?
There was a problem hiding this comment.
So, the config index will be created automatically when the monitor is created. So, when the totalMonitors is 0 it is expected that the config index has not been created, and we don't want to show that error popup to the user.
Description
This PR fixes the issue of an unnecessary error popup appearing while creation of a monitor. The error popup is raised because the config index is not been before a monitor is created, but is created automatically when the monitor is created. So, the error popup is redundant when creating a monitor.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.