-
Notifications
You must be signed in to change notification settings - Fork 267
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
Implemented the handleTestNotification function. #1904
Changes from 19 commits
7b1a60d
845b769
9aa0041
b811fb9
f7b247f
798a680
8e026ed
f735b08
de8edbf
b1194eb
00700c8
8994b4b
182c513
910defb
4cbcebb
13b8bb6
caab2c3
c84a547
3136bdb
840690d
100c5d3
9462632
a0920f9
51da6b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import { useState, useMemo } from "react"; | ||
import { useTranslation } from "react-i18next"; | ||
|
||
import { | ||
Dialog, | ||
DialogContent, | ||
|
@@ -8,11 +9,29 @@ import { | |
Typography, | ||
Box, | ||
Tabs, | ||
Tab | ||
Tab, | ||
CircularProgress | ||
} from "@mui/material"; | ||
import { useTheme } from "@emotion/react"; | ||
import TabPanel from "./TabPanel"; | ||
import TabComponent from "./TabComponent"; | ||
import useNotifications from "../Hooks/useNotification"; | ||
|
||
// Define constants for notification types to avoid magic values | ||
const NOTIFICATION_TYPES = { | ||
SLACK: 'slack', | ||
DISCORD: 'discord', | ||
TELEGRAM: 'telegram', | ||
WEBHOOK: 'webhook' | ||
}; | ||
|
||
// Define constants for field IDs | ||
const FIELD_IDS = { | ||
WEBHOOK: 'webhook', | ||
TOKEN: 'token', | ||
CHAT_ID: 'chatId', | ||
URL: 'url' | ||
}; | ||
|
||
const NotificationIntegrationModal = ({ | ||
open, | ||
|
@@ -26,60 +45,75 @@ const NotificationIntegrationModal = ({ | |
const theme = useTheme(); | ||
const [tabValue, setTabValue] = useState(0); | ||
|
||
const [loading, _, sendTestNotification] = useNotifications(); | ||
|
||
// Helper to get the field state key with error handling | ||
const getFieldKey = (typeId, fieldId) => { | ||
if (typeof typeId !== 'string' || typeId === '') { | ||
throw new Error('Invalid typeId provided to getFieldKey'); | ||
} | ||
|
||
if (typeof fieldId !== 'string' || fieldId === '') { | ||
throw new Error('Invalid fieldId provided to getFieldKey'); | ||
} | ||
|
||
return `${typeId}${fieldId.charAt(0).toUpperCase() + fieldId.slice(1)}`; | ||
}; | ||
|
||
// Define notification types | ||
const DEFAULT_NOTIFICATION_TYPES = [ | ||
{ | ||
id: 'slack', | ||
id: NOTIFICATION_TYPES.SLACK, | ||
label: t('notifications.slack.label'), | ||
description: t('notifications.slack.description'), | ||
fields: [ | ||
{ | ||
id: 'webhook', | ||
id: FIELD_IDS.WEBHOOK, | ||
label: t('notifications.slack.webhookLabel'), | ||
placeholder: t('notifications.slack.webhookPlaceholder'), | ||
type: 'text' | ||
} | ||
] | ||
}, | ||
{ | ||
id: 'discord', | ||
id: NOTIFICATION_TYPES.DISCORD, | ||
label: t('notifications.discord.label'), | ||
description: t('notifications.discord.description'), | ||
fields: [ | ||
{ | ||
id: 'webhook', | ||
id: FIELD_IDS.WEBHOOK, | ||
label: t('notifications.discord.webhookLabel'), | ||
placeholder: t('notifications.discord.webhookPlaceholder'), | ||
type: 'text' | ||
} | ||
] | ||
}, | ||
{ | ||
id: 'telegram', | ||
id: NOTIFICATION_TYPES.TELEGRAM, | ||
label: t('notifications.telegram.label'), | ||
description: t('notifications.telegram.description'), | ||
fields: [ | ||
{ | ||
id: 'token', | ||
id: FIELD_IDS.TOKEN, | ||
label: t('notifications.telegram.tokenLabel'), | ||
placeholder: t('notifications.telegram.tokenPlaceholder'), | ||
type: 'text' | ||
}, | ||
{ | ||
id: 'chatId', | ||
id: FIELD_IDS.CHAT_ID, | ||
label: t('notifications.telegram.chatIdLabel'), | ||
placeholder: t('notifications.telegram.chatIdPlaceholder'), | ||
type: 'text' | ||
} | ||
] | ||
}, | ||
{ | ||
id: 'webhook', | ||
id: NOTIFICATION_TYPES.WEBHOOK, | ||
label: t('notifications.webhook.label'), | ||
description: t('notifications.webhook.description'), | ||
fields: [ | ||
{ | ||
id: 'url', | ||
id: FIELD_IDS.URL, | ||
label: t('notifications.webhook.urlLabel'), | ||
placeholder: t('notifications.webhook.urlPlaceholder'), | ||
type: 'text' | ||
|
@@ -101,7 +135,7 @@ const NotificationIntegrationModal = ({ | |
|
||
// Add state for each field in the notification type | ||
type.fields.forEach(field => { | ||
const fieldKey = `${type.id}${field.id.charAt(0).toUpperCase() + field.id.slice(1)}`; | ||
const fieldKey = getFieldKey(type.id, field.id); | ||
state[fieldKey] = monitor?.notifications?.find(n => n.type === type.id)?.[field.id] || ""; | ||
}); | ||
}); | ||
|
@@ -129,11 +163,26 @@ const NotificationIntegrationModal = ({ | |
})); | ||
}; | ||
|
||
const handleTestNotification = (type) => { | ||
console.log(`Testing ${type} notification`); | ||
//implement the test notification functionality | ||
const handleTestNotification = async (type) => { | ||
// Get the notification type details | ||
const notificationType = activeNotificationTypes.find(t => t.id === type); | ||
|
||
if (typeof notificationType === "undefined") { | ||
return; | ||
} | ||
|
||
// Prepare config object based on notification type | ||
const config = {}; | ||
|
||
// Add each field value to the config object | ||
notificationType.fields.forEach(field => { | ||
const fieldKey = getFieldKey(type, field.id); | ||
config[field.id] = integrations[fieldKey]; | ||
}); | ||
|
||
await sendTestNotification(type, config); | ||
}; | ||
|
||
const handleSave = () => { | ||
//notifications array for selected integrations | ||
const notifications = [...(monitor?.notifications || [])]; | ||
|
@@ -155,7 +204,7 @@ const NotificationIntegrationModal = ({ | |
|
||
// Add each field value to the notification object | ||
type.fields.forEach(field => { | ||
const fieldKey = `${type.id}${field.id.charAt(0).toUpperCase() + field.id.slice(1)}`; | ||
const fieldKey = getFieldKey(type.id, field.id); | ||
notificationObject[field.id] = integrations[fieldKey]; | ||
}); | ||
|
||
|
@@ -240,6 +289,7 @@ const NotificationIntegrationModal = ({ | |
handleIntegrationChange={handleIntegrationChange} | ||
handleInputChange={handleInputChange} | ||
handleTestNotification={handleTestNotification} | ||
isLoading={loading} | ||
/> | ||
</TabPanel> | ||
))} | ||
|
@@ -257,13 +307,22 @@ const NotificationIntegrationModal = ({ | |
variant="contained" | ||
color="accent" | ||
onClick={handleSave} | ||
disabled={loading} | ||
sx={{ | ||
width: 'auto', | ||
minWidth: theme.spacing(60), | ||
px: theme.spacing(8) | ||
}} | ||
> | ||
{t('common.save', 'Save')} | ||
{loading ? | ||
<CircularProgress | ||
size={theme.spacing(12)} | ||
sx={{ | ||
color: theme.palette.accent.main | ||
}} | ||
/> | ||
: t('common.save', 'Save') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can remove the second string I believe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup! You are right. I also needed to add the common save to the translation JSON. Thanks! |
||
} | ||
</Button> | ||
</DialogActions> | ||
</Dialog> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,19 +2,21 @@ import React from "react"; | |
import { | ||
Typography, | ||
Box, | ||
Button | ||
Button, | ||
CircularProgress | ||
} from "@mui/material"; | ||
import { useTranslation } from "react-i18next"; | ||
import { useTheme } from "@emotion/react"; | ||
import TextInput from "../../../src/Components/Inputs/TextInput"; | ||
import Checkbox from "../../../src/Components/Inputs/Checkbox"; | ||
import TextInput from "../../../Components/Inputs/TextInput"; | ||
import Checkbox from "../../../Components/Inputs/Checkbox"; | ||
|
||
const TabComponent = ({ | ||
type, | ||
integrations, | ||
handleIntegrationChange, | ||
handleInputChange, | ||
handleTestNotification | ||
handleTestNotification, | ||
isLoading | ||
Comment on lines
+18
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainNew isLoading prop added to component signature You've added the isLoading prop to the component parameters, which is used throughout the component to disable interactions during async operations. Consider adding PropTypes validation for all props to satisfy the ESLint warnings: 🏁 Script executed: #!/bin/bash
# Check if PropTypes is already imported in the file
grep -q "import PropTypes from 'prop-types'" src/Components/NotificationIntegrationModal/Components/TabComponent.jsx
if [ $? -ne 0 ]; then
echo "PropTypes import not found, consider adding:"
echo "import PropTypes from 'prop-types';"
echo ""
echo "And add prop validation at the bottom of the file before the export:"
echo "TabComponent.propTypes = {"
echo " type: PropTypes.object.isRequired,"
echo " integrations: PropTypes.object.isRequired,"
echo " handleIntegrationChange: PropTypes.func.isRequired,"
echo " handleInputChange: PropTypes.func.isRequired,"
echo " handleTestNotification: PropTypes.func.isRequired,"
echo " isLoading: PropTypes.bool"
echo "};"
fi Length of output: 1148 Action Required: Add missing PropTypes validation in TabComponent.jsx The new isLoading prop is now being used correctly, but our verification shows that the PropTypes import and corresponding prop types definition are absent. To satisfy ESLint warnings, please add the following changes:
His palms are sweaty, knees weak, arms are heavy—let’s fix this before we call it good to go! 🧰 Tools🪛 ESLint[error] 18-18: 'handleTestNotification' is missing in props validation (react/prop-types) [error] 19-19: 'isLoading' is missing in props validation (react/prop-types) |
||
}) => { | ||
const theme = useTheme(); | ||
const { t } = useTranslation(); | ||
|
@@ -55,6 +57,7 @@ const TabComponent = ({ | |
label={t('notifications.enableNotifications', { platform: type.label })} | ||
isChecked={integrations[type.id]} | ||
onChange={(e) => handleIntegrationChange(type.id, e.target.checked)} | ||
disabled={isLoading} | ||
/> | ||
</Box> | ||
|
||
|
@@ -77,7 +80,7 @@ const TabComponent = ({ | |
placeholder={field.placeholder} | ||
value={integrations[fieldKey]} | ||
onChange={(e) => handleInputChange(fieldKey, e.target.value)} | ||
disabled={!integrations[type.id]} | ||
disabled={!integrations[type.id] || isLoading} | ||
/> | ||
</Box> | ||
); | ||
|
@@ -88,8 +91,14 @@ const TabComponent = ({ | |
variant="text" | ||
color="info" | ||
onClick={() => handleTestNotification(type.id)} | ||
disabled={!integrations[type.id] || !areAllFieldsFilled()} | ||
disabled={!integrations[type.id] || !areAllFieldsFilled() || isLoading} | ||
> | ||
{isLoading ? ( | ||
<CircularProgress | ||
size={theme.spacing(8)} | ||
sx={{ mr: theme.spacing(1), color: theme.palette.accent.main}} | ||
/> | ||
) : null} | ||
{t('notifications.testNotification')} | ||
</Button> | ||
</Box> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
import { useState } from 'react'; | ||
import { toast } from 'react-toastify'; | ||
import { useTranslation } from "react-i18next"; | ||
import { networkService } from '../../../Utils/NetworkService'; | ||
|
||
// Define constants for notification types to avoid magic values | ||
const NOTIFICATION_TYPES = { | ||
SLACK: 'slack', | ||
DISCORD: 'discord', | ||
TELEGRAM: 'telegram', | ||
WEBHOOK: 'webhook' | ||
}; | ||
|
||
// Define constants for field IDs | ||
const FIELD_IDS = { | ||
WEBHOOK: 'webhook', | ||
TOKEN: 'token', | ||
CHAT_ID: 'chatId', | ||
URL: 'url' | ||
}; | ||
|
||
/** | ||
* Custom hook for notification-related operations | ||
*/ | ||
const useNotifications = () => { | ||
const [loading, setLoading] = useState(false); | ||
const [error, setError] = useState(undefined); | ||
const { t } = useTranslation(); | ||
|
||
/** | ||
* Send a test notification | ||
* @param {string} type - The notification type (slack, discord, telegram, webhook) | ||
* @param {object} config - Configuration object with necessary params | ||
*/ | ||
const sendTestNotification = async (type, config) => { | ||
setLoading(true); | ||
setError(undefined); | ||
|
||
// Validation based on notification type | ||
let payload = { platform: type }; | ||
let isValid = true; | ||
let errorMessage = ''; | ||
|
||
switch(type) { | ||
case NOTIFICATION_TYPES.SLACK: | ||
payload.webhookUrl = config.webhook; | ||
if (typeof payload.webhookUrl === 'undefined' || payload.webhookUrl === '') { | ||
isValid = false; | ||
errorMessage = t('notifications.slack.webhookRequired'); | ||
} | ||
break; | ||
|
||
case NOTIFICATION_TYPES.DISCORD: | ||
payload.webhookUrl = config.webhook; | ||
if (typeof payload.webhookUrl === 'undefined' || payload.webhookUrl === '') { | ||
isValid = false; | ||
errorMessage = t('notifications.discord.webhookRequired'); | ||
} | ||
break; | ||
|
||
case NOTIFICATION_TYPES.TELEGRAM: | ||
payload.botToken = config.token; | ||
payload.chatId = config.chatId; | ||
if (typeof payload.botToken === 'undefined' || payload.botToken === '' || | ||
typeof payload.chatId === 'undefined' || payload.chatId === '') { | ||
isValid = false; | ||
errorMessage = t('notifications.telegram.fieldsRequired'); | ||
} | ||
break; | ||
|
||
case NOTIFICATION_TYPES.WEBHOOK: | ||
payload.webhookUrl = config.url; | ||
payload.platform = NOTIFICATION_TYPES.SLACK; | ||
if (typeof payload.webhookUrl === 'undefined' || payload.webhookUrl === '') { | ||
isValid = false; | ||
errorMessage = t('notifications.webhook.urlRequired'); | ||
} | ||
break; | ||
|
||
default: | ||
isValid = false; | ||
errorMessage = t('notifications.unsupportedType'); | ||
} | ||
|
||
// If validation fails, show error and return | ||
if (isValid === false) { | ||
toast.error(errorMessage); | ||
setLoading(false); | ||
return; | ||
} | ||
|
||
try { | ||
const response = await networkService.testNotification({ | ||
platform: type, | ||
payload: payload | ||
}); | ||
|
||
if (response.data.success === true) { | ||
toast.success(t('notifications.testSuccess')); | ||
} else { | ||
throw new Error(response.data.msg || t('notifications.testFailed')); | ||
} | ||
} catch (error) { | ||
const errorMsg = error.response?.data?.msg || error.message || t('notifications.networkError'); | ||
toast.error(`${t('notifications.testFailed')}: ${errorMsg}`); | ||
setError(errorMsg); | ||
} finally { | ||
setLoading(false); | ||
} | ||
}; | ||
|
||
return [ | ||
loading, | ||
error, | ||
sendTestNotification | ||
]; | ||
}; | ||
|
||
export default useNotifications; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to do this manually, the
Button
has a loading state. We use that elsewhere so it should be used here tooThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Thanks!