You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by GitBox <gi...@apache.org> on 2022/09/05 07:20:45 UTC
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r962554154
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -42,15 +43,13 @@ function useConnectionManager (
const [provider, setProvider] = useState(activeProvider)
const [name, setName] = useState()
+ // @todo: refactor to endpoint and setEndpoint
const [endpointUrl, setEndpointUrl] = useState()
const [proxy, setProxy] = useState()
- const [rateLimit, setRateLimit] = useState(0)
+ const [rateLimitPerHour, setRateLimitPerHour] = useState(0)
const [token, setToken] = useState()
- const [initialTokenStore, setInitialTokenStore] = useState({
- 0: '',
- 1: '',
- 2: ''
- })
+ const defaultTokenStore = useMemo(() => ({ 0: '', 1: '', 2: '' }), [])
+ const [initialTokenStore, setInitialTokenStore] = useState(defaultTokenStore)
Review Comment:
why `useState(['', '', ''])`?
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -42,15 +43,13 @@ function useConnectionManager (
const [provider, setProvider] = useState(activeProvider)
const [name, setName] = useState()
+ // @todo: refactor to endpoint and setEndpoint
const [endpointUrl, setEndpointUrl] = useState()
const [proxy, setProxy] = useState()
- const [rateLimit, setRateLimit] = useState(0)
+ const [rateLimitPerHour, setRateLimitPerHour] = useState(0)
const [token, setToken] = useState()
- const [initialTokenStore, setInitialTokenStore] = useState({
- 0: '',
- 1: '',
- 2: ''
- })
+ const defaultTokenStore = useMemo(() => ({ 0: '', 1: '', 2: '' }), [])
Review Comment:
meaningless useMemo
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -141,70 +154,66 @@ function useConnectionManager (
[provider?.id, connectionTestPayload]
)
- const saveConnection = (configurationSettings = {}) => {
- setIsSaving(true)
- let connectionPayload = { ...configurationSettings }
- switch (provider.id) {
- case Providers.JIRA:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.TAPD:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.GITHUB:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- token: token,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.JENKINS:
- // eslint-disable-next-line max-len
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.GITLAB:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- token: token,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
+ const notifyConnectionSaveSuccess = useCallback((message = 'Connection saved successfully.') => {
Review Comment:
meaningless useCallback
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -141,70 +154,66 @@ function useConnectionManager (
[provider?.id, connectionTestPayload]
)
- const saveConnection = (configurationSettings = {}) => {
- setIsSaving(true)
- let connectionPayload = { ...configurationSettings }
- switch (provider.id) {
- case Providers.JIRA:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.TAPD:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.GITHUB:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- token: token,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.JENKINS:
- // eslint-disable-next-line max-len
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.GITLAB:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- token: token,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
+ const notifyConnectionSaveSuccess = useCallback((message = 'Connection saved successfully.') => {
+ ToastNotification.show({
+ message: message,
+ intent: 'success',
+ icon: 'small-tick',
+ })
+ }, [])
+
+ const notifyConnectionSaveFailure = useCallback((message = 'Connection failed to save, please try again.') => {
+ ToastNotification.show({
+ message: message,
+ intent: 'danger',
+ icon: 'error',
+ })
+ }, [])
+
+ const fetchConnection = useCallback(
+ (silent = false, notify = false, cId = null) => {
+ console.log(`>> FETCHING CONNECTION [PROVIDER = ${provider?.id}]....`)
+ try {
+ setIsFetching(!silent)
+ setErrors([])
+ console.log('>> FETCHING CONNECTION SOURCE')
+ const fetch = async () => {
+ const f = await request.get(
+ `${DEVLAKE_ENDPOINT}/plugins/${provider?.id}/connections/${cId || connectionId}`
+ )
+ const connectionData = f.data
+ console.log('>> RAW CONNECTION DATA FROM API...', connectionData)
+ setActiveConnection(new Connection({
+ ...connectionData
+ }))
+ setTimeout(() => {
Review Comment:
why `try {} catch {} finall { setIsFetching(false) }`?
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -141,70 +154,66 @@ function useConnectionManager (
[provider?.id, connectionTestPayload]
)
- const saveConnection = (configurationSettings = {}) => {
- setIsSaving(true)
- let connectionPayload = { ...configurationSettings }
- switch (provider.id) {
- case Providers.JIRA:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.TAPD:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.GITHUB:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- token: token,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.JENKINS:
- // eslint-disable-next-line max-len
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- username: username,
- password: password,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
- }
- break
- case Providers.GITLAB:
- connectionPayload = {
- name: name,
- endpoint: endpointUrl,
- token: token,
- proxy: proxy,
- rateLimitPerHour: rateLimit,
- ...connectionPayload,
+ const notifyConnectionSaveSuccess = useCallback((message = 'Connection saved successfully.') => {
+ ToastNotification.show({
+ message: message,
+ intent: 'success',
+ icon: 'small-tick',
+ })
+ }, [])
+
+ const notifyConnectionSaveFailure = useCallback((message = 'Connection failed to save, please try again.') => {
+ ToastNotification.show({
+ message: message,
+ intent: 'danger',
+ icon: 'error',
+ })
+ }, [])
+
+ const fetchConnection = useCallback(
+ (silent = false, notify = false, cId = null) => {
+ console.log(`>> FETCHING CONNECTION [PROVIDER = ${provider?.id}]....`)
+ try {
+ setIsFetching(!silent)
+ setErrors([])
+ console.log('>> FETCHING CONNECTION SOURCE')
+ const fetch = async () => {
+ const f = await request.get(
+ `${DEVLAKE_ENDPOINT}/plugins/${provider?.id}/connections/${cId || connectionId}`
+ )
+ const connectionData = f.data
+ console.log('>> RAW CONNECTION DATA FROM API...', connectionData)
+ setActiveConnection(new Connection({
+ ...connectionData
+ }))
+ setTimeout(() => {
+ setIsFetching(false)
+ }, 500)
}
- break
- }
+ fetch()
Review Comment:
`await`?
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -546,69 +493,45 @@ function useConnectionManager (
setUsername('')
setPassword('')
setToken('')
- setInitialTokenStore({
- 0: '',
- 1: '',
- 2: ''
- })
+ setInitialTokenStore(defaultTokenStore)
setProxy('')
- setRateLimit(0)
- }, [])
+ setRateLimitPerHour(0)
+ }, [defaultTokenStore])
useEffect(() => {
if (activeConnection && activeConnection.id !== null) {
- const connectionToken = activeConnection.auth || activeConnection.token || activeConnection.basicAuthEncoded
- setName(activeConnection.name)
- setEndpointUrl(activeConnection.endpoint)
- switch (provider.id) {
- case Providers.JENKINS:
- setUsername(activeConnection.username)
- setPassword(activeConnection.password)
- setProxy(activeConnection.Proxy || activeConnection.proxy)
- setRateLimit(activeConnection.rateLimitPerHour)
- break
- case Providers.GITLAB:
- setToken(activeConnection.basicAuthEncoded || activeConnection.token || activeConnection.auth)
- setProxy(activeConnection.Proxy || activeConnection.proxy)
- setRateLimit(activeConnection.rateLimitPerHour)
- break
- case Providers.GITHUB:
- setToken(connectionToken)
- setInitialTokenStore(connectionToken?.split(',')?.reduce((tS, cT, id) => ({ ...tS, [id]: cT }), {}))
- setProxy(activeConnection.Proxy || activeConnection.proxy)
- setRateLimit(activeConnection.rateLimitPerHour)
- break
- case Providers.JIRA:
- // setToken(activeConnection.basicAuthEncoded || activeConnection.token)
- setUsername(activeConnection.username)
- setPassword(activeConnection.password)
- setProxy(activeConnection.Proxy || activeConnection.proxy)
- setRateLimit(activeConnection.rateLimitPerHour)
- break
- case Providers.TAPD:
- setUsername(activeConnection.username)
- setPassword(activeConnection.password)
- setProxy(activeConnection.Proxy || activeConnection.proxy)
- setRateLimit(activeConnection.rateLimitPerHour)
- break
- }
- ToastNotification.clear()
- // ToastNotification.show({ message: `Fetched settings for ${activeConnection.name}.`, intent: 'success', icon: 'small-tick' })
+ setName(activeConnection?.name)
+ setEndpointUrl(activeConnection?.endpoint)
+ setRateLimitPerHour(activeConnection?.rateLimitPerHour)
+ setProxy(activeConnection?.proxy)
+ setUsername(activeConnection?.username)
+ setPassword(activeConnection?.password)
+ setToken(activeConnection?.token)
+ setInitialTokenStore(activeConnection?.token
Review Comment:
terrible variable definition, why not in an object?
##########
config-ui/src/components/actions/DeleteAction.jsx:
##########
@@ -28,21 +28,25 @@ import {
const DeleteAction = (props) => {
const {
- id, connection, showConfirmation = () => {}, onConfirm = () => {}, onCancel = () => {},
+ id,
+ connection,
+ showConfirmation = () => {},
+ onConfirm = () => {},
+ onCancel = () => {},
isDisabled = false,
isLoading = false,
text = 'Delete',
children
} = props
return (
<Popover
- key={`delete-popover-key-${connection.ID}`}
+ key={`delete-popover-key-${connection.id}`}
className='trigger-delete-connection'
popoverClassName='popover-delete-connection'
position={Position.RIGHT}
autoFocus={false}
enforceFocus={false}
- isOpen={id === connection.ID}
+ isOpen={id !== null && id === connection.id}
Review Comment:
`id && id === connection.id`?
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -225,9 +234,15 @@ function useConnectionManager (
connection: { ...s.data },
errors: s.isAxiosError ? [s.message] : [],
}
+ setIsSaving(false)
+ setSaveComplete(saveResponse.success ? new Connection(s.data) : false)
+ if (!saveResponse.success) { notifyConnectionSaveFailure(s.data || s.message) }
Review Comment:
throw a custom error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org