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/02 01:24:26 UTC
[GitHub] [incubator-devlake] e2corporation opened a new pull request, #2928: fix: config-ui: refactor connection manager save workflow + enhancements
e2corporation opened a new pull request, #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928
### ⚠️ Config-UI / Data Integrations / Connection Manager
- [ ] `Fix` Connection Save Handling and Remove extra `savePromise` added during recent refactor
- [ ] `Fix` Restore Successful Toast Notifications on Connection Save
- [ ] `Fix` Remove `setTimeout` Handler
- [ ] `Fix` Remove legacy inlined connection test post-save
- [ ] `Feat` Add **Connection** _Data Model_ (`models/Connection.js`)
- [ ] `Refactor` Change legacy uppercase Connection `ID` prop to `id`
- [ ] `Refactor` change Connection Prop `rateLimit` to `rateLimitPerHour` for future consistency
- [ ] `Fix` Resolve Lint warnings with missing React Effect Dependencies
### Description
This PR performs refactoring enhancements to **Connection Manager Hook** that restore the desired save handling workflow for data connections. After a connection is successfully saved, the user is redirected back to the Manage Connections screen _only_ when Adding a New Connection. When Modifying an existing Connection, the user will remain on the Configure Connection screen after saving.
### Does this close any open issues?
Closes #2923
--
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r962735829
##########
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:
This isn't a problem, the `Finally` catch block could be implemented, for now controlling fetch state is handled directly within try/catch.
--
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r962708231
##########
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:
These are controlled properties for a detailed connection form, if you don't understand something it doesn't make it terrible.
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r962733332
##########
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:
Not needed here, notifications are being dispatched on-demand.
--
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r963592245
##########
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:
@mintsweet I wonder if you'd be the best judge to decide what dictates "good code"
--
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r962727178
##########
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 feedback. It's a memo with no compute dependencies at the moment, so the const can be defined in the component.
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r963200064
##########
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:
I am talking about the problem of setTimeout. If the response time is greater than the set time, the pop-up prompt is wrong.
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r963200680
##########
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:
I see that there are duplicate notifyConnectionSaveFailure functions, my task is unnecessary.
At the same time error handling is unified.
--
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r962706707
##########
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:
Have you given thought or used callbacks before at all prior to making comments? This is reusable notification success helper. No it's not meaningless, any funcs that are internal to the class are to be wrapped in callbacks to prevent unwanted re-renders.
--
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
[GitHub] [incubator-devlake] e2corporation commented on pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#issuecomment-1238029101
@mintsweet Your tone comes across like that of a disgruntled community developer member blurting out random comments as you struggle try to find something wrong to post about. None of the issues you mention cause any usability bugs or have much to do with the intent of this PR. Please avoid polluting my pull request with senseless feedback.
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r965507518
##########
config-ui/src/models/GithubProject.js:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+/**
+ * @typedef {object} GitHubProject
+ * @property {number|string?} id
+ * @property {number?} key
+ * @property {string|number?} value
+ * @property {string|number?} name
+ * @property {string|number?} title
+ * @property {string?} owner
+ * @property {string?} repo
+ * @property {boolean?} useApi
+ * @property {project|board?} variant
+ */
+class GitHubProject {
+ constructor (data = {}) {
Review Comment:
```js
data = {
id: null,
key: null,
...
}
this.id = data.id;
```
do you think it would be better？
--
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r965946264
##########
config-ui/src/models/GithubProject.js:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+/**
+ * @typedef {object} GitHubProject
+ * @property {number|string?} id
+ * @property {number?} key
+ * @property {string|number?} value
+ * @property {string|number?} name
+ * @property {string|number?} title
+ * @property {string?} owner
+ * @property {string?} repo
+ * @property {boolean?} useApi
+ * @property {project|board?} variant
+ */
+class GitHubProject {
+ constructor (data = {}) {
Review Comment:
this default `data` object will get get replaced when the model is constructed, so these preserved keys initialized as null within this data obj be wiped. The current behavior of `this.id = data?.id || null` should be sufficient for now.
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r963199673
##########
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:
Well, I didn't look closely.
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r963201139
##########
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:
Good code is easy to understand.
--
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
[GitHub] [incubator-devlake] mintsweet commented on pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#issuecomment-1238237395
@e2corporation sorry, my English is not very good, and my expression may be too abrupt.
It's my fault to make you feel uncomfortable. I sincerely hope devlake becomes better.
--
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r962712554
##########
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:
This `fetch` here is an internal helper method not the HTTP Fetch API.
--
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
[GitHub] [incubator-devlake] e2corporation commented on pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#issuecomment-1241118717
> @e2corporation sorry, my English is not very good, and my expression may be too abrupt. It's my fault to make you feel uncomfortable. I sincerely hope devlake becomes better.
@mintsweet Thanks for taking the time to clarify, I couldn't tell I thought you were just being direct. Had you introduced yourself as one of our new colleagues I would have been more receptive to your feedback and found it less invasive, though code review is not the really the ideal place to start as you are just getting adjusted to the codebase. There will be opportunities going forward for you to apply some of your suggestions and ideas such as implementing the `Finally` block on try/catch statements that could benefit from them etc. I will make our collaboration a smoother process for you going forward.
For now, focusing your efforts on Feature development tickets for DORA would be ideal for this milestone.
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r963199391
##########
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:
I think the defaultTokenStore should be placed outside the component so that it will only be declared when the app is initialized.useMemo brings performance issues.
--
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
[GitHub] [incubator-devlake] klesh merged pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
klesh merged PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928
--
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
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r962710907
##########
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:
Because the default tokenStore is reused in several places why would I want to re-create a generic object each time when I can create a default token store instead? Please read the code before making baseless comments.
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r963198696
##########
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:
https://www.developerway.com/posts/how-to-use-memo-use-callback
This article roughly explains
--
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
[GitHub] [incubator-devlake] mintsweet commented on a diff in pull request #2928: fix: config-ui: refactor connection manager save workflow + enhancements
Posted by GitBox <gi...@apache.org>.
mintsweet commented on code in PR #2928:
URL: https://github.com/apache/incubator-devlake/pull/2928#discussion_r963196984
##########
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:
overuse affects more than re-rendering, I don't see any dependencies preventing re-rendering.
--
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