Merge pull request #1015 from 3scale/feature/THREESCALE-2646_service_…

…discovery_errors Improve Service Discovery Errors (AKA improve New API form)
3scale · Aug 20, 2019 · b360f37 · b360f37
2 parents 28634fd + 10d4ac9
commit b360f37
Show file tree

Hide file tree

Showing 24 changed files with 385 additions and 172 deletions.
diff --git a/.flowconfig b/.flowconfig
@@ -12,6 +12,7 @@
 [lints]
 
 [options]
+module.system.node.resolve_dirname=node_modules
 module.name_mapper.extension='scss' -> 'empty/object'
 module.system=haste
 module.name_mapper='\(Applications\|Dashboard\|LoginPage\|Navigation\|Onboarding\|Policies\|services\|Stats\|Types\|Users\|utilities\|NewService\)\(.*\)$' -> '<PROJECT_ROOT>/app/javascript/src/\1/\2'
diff --git a/app/assets/stylesheets/provider/_forms.scss b/app/assets/stylesheets/provider/_forms.scss
@@ -1,3 +1,5 @@
+$fieldset-margin-bottom: line-height-times(1.5);
+
 form.formtastic ol, form.formtastic ul {
   list-style: none;
 }
@@ -310,7 +312,7 @@ form.formtastic {
     position: relative;
     padding: line-height-times(1) 0 0 0;
     border-top: $border-width solid $border-color;
-    margin-bottom: line-height-times(1.5);
+    margin-bottom: $fieldset-margin-bottom;
     -moz-appearance: none;
     -webkit-appearance: none;
     appearance: none;
@@ -533,10 +535,14 @@ li.hidden {
   display: none;
 }
 
-.errorMessage {
-  color: $error-color;
-}
+#new_service_wrapper {
+  .errorMessage {
+    color: $error-color;
+    margin-bottom: line-height-times(1/2);
+    margin-top: -#{$fieldset-margin-bottom};
+  }
 
-.new-service-source-input {
-  padding-left: line-height-times(5 / 10);
+  .new-service-source-input {
+    padding: 0 line-height-times(5 / 10);
+  }
 }
diff --git a/app/controllers/provider/admin/service_discovery/cluster_projects_controller.rb b/app/controllers/provider/admin/service_discovery/cluster_projects_controller.rb
@@ -2,6 +2,7 @@
 
 class Provider::Admin::ServiceDiscovery::ClusterProjectsController < Provider::Admin::ServiceDiscovery::ClusterBaseController
   def index
-    render json: { projects: cluster.projects_with_discoverables.map(&:to_json) }
+    render json: cluster.projects_with_discoverables.map(&:name)
+                                                    .to_json
   end
 end
diff --git a/app/controllers/provider/admin/service_discovery/cluster_services_controller.rb b/app/controllers/provider/admin/service_discovery/cluster_services_controller.rb
@@ -2,13 +2,20 @@
 
 class Provider::Admin::ServiceDiscovery::ClusterServicesController < Provider::Admin::ServiceDiscovery::ClusterBaseController
   def index
-    render json: { services: cluster.discoverable_services(namespace: params.require(:namespace_id)).map(&:to_json) }
+    render json: cluster.discoverable_services(namespace: namespace_id).map(&:name)
+                                                                       .to_json
   end
 
   def show
-    cluster_service = cluster.find_discoverable_service_by(namespace: params.require(:namespace_id), name: params[:id])
+    cluster_service = cluster.find_discoverable_service_by(namespace: namespace_id, name: params[:id])
     render json: cluster_service.to_json
   rescue ::ServiceDiscovery::ClusterClient::ResourceNotFound => exception
     render_error exception.message, status: :not_found
   end
+
+  private
+
+  def namespace_id
+    params.require(:namespace_id)
+  end
 end
diff --git a/app/javascript/src/NewService/components/FormElements/ErrorMessage.jsx b/app/javascript/src/NewService/components/FormElements/ErrorMessage.jsx
@@ -4,9 +4,11 @@ import React from 'react'
 
 const ErrorMessage = ({fetchErrorMessage}: {
   fetchErrorMessage: string
-}) => <p className='errorMessage'>
-  {`Sorry, your request has failed with the error: ${fetchErrorMessage}`}
-</p>
+}) => (
+  <p className='errorMessage'>
+    {`Sorry, your request has failed with the error: ${fetchErrorMessage}`}
+  </p>
+)
 
 export {
   ErrorMessage

diff --git a/app/javascript/src/NewService/components/FormElements/Select.jsx b/app/javascript/src/NewService/components/FormElements/Select.jsx
@@ -2,34 +2,29 @@
 
 import React from 'react'
 
-type Option = {
-  metadata: {
-    name: string
-  }
-}
-
 type Props = {
   name: string,
   id: string,
+  disabled?: boolean,
   onChange?: (event: SyntheticEvent<HTMLSelectElement>) => void,
-  options: Array<Option>
+  options: Array<string>
 }
 
 const Options = ({options}) => {
-  return options.map(option => {
-    const { name } = option.metadata
-    return <option key={name} value={name}>{name}</option>
+  return options.map((option) => {
+    return <option key={option} value={option}>{option}</option>
   })
 }
 
-const Select = ({name, id, onChange, options}: Props) =>
+const Select = ({name, id, disabled, onChange, options}: Props) =>
   <select
     required="required"
     name={name}
     id={id}
+    disabled={disabled}
     onChange={onChange}
   >
-    {<Options options={options}/>}
+    <Options options={options}/>
   </select>
 
 export {Select}
diff --git a/app/javascript/src/NewService/components/FormElements/ServiceDiscoveryListItems.jsx b/app/javascript/src/NewService/components/FormElements/ServiceDiscoveryListItems.jsx
@@ -1,27 +1,53 @@
 // @flow
 
-import React from 'react'
+import React, {useState, useEffect} from 'react'
 import {Label, Select} from 'NewService/components/FormElements'
+import {fetchData} from 'utilities/utils'
+import {BASE_PATH} from 'NewService'
 
 type Props = {
-  fetchServices: (namespace: string) => Promise<void>,
   projects: string[],
-  services: string[]
+  onError: (err: string) => void
 }
 
 const ServiceDiscoveryListItems = (props: Props) => {
-  const {fetchServices, projects, services} = props
+  const { projects, onError } = props
+
+  const [services, setServices] = useState([])
+  const [loading, setLoading] = useState(true)
+
+  useEffect(() => {
+    if (projects && projects.length) {
+      fetchServices(projects[0])
+    }
+  }, [projects])
+
+  const fetchServices = async (namespace: string) => {
+    setLoading(true)
+    setServices([])
+
+    try {
+      const services = await fetchData<string[]>(`${BASE_PATH}/namespaces/${namespace}/services.json`)
+      setServices(services)
+    } catch (error) {
+      onError(error.message)
+    } finally {
+      setLoading(false)
+    }
+  }
+
   return (
     <React.Fragment>
       <li id="service_name_input" className="string required">
         <Label
-          htmlFor='namespace'
+          htmlFor='service_namespace'
           label='Namespace'
         />
         <Select
+          disabled={loading}
           name='service[namespace]'
           id='service_namespace'
-          onChange={(e) => fetchServices(e.currentTarget.value)}
+          onChange={e => { fetchServices(e.currentTarget.value) }}
           options={projects}
         />
       </li>
@@ -31,6 +57,7 @@ const ServiceDiscoveryListItems = (props: Props) => {
           label='Name'
         />
         <Select
+          disabled={loading}
           name='service[name]'
           id='service_name'
           options={services}

diff --git a/app/javascript/src/NewService/components/FormElements/index.jsx b/app/javascript/src/NewService/components/FormElements/index.jsx
@@ -1,3 +1,5 @@
+// @flow
+
 export {FormWrapper} from 'NewService/components/FormElements/FormWrapper'
 export {HiddenServiceDiscoveryInput} from 'NewService/components/FormElements/HiddenServiceDiscoveryInput'
 export {Label} from 'NewService/components/FormElements/Label'

diff --git a/app/javascript/src/NewService/components/NewServiceForm.jsx b/app/javascript/src/NewService/components/NewServiceForm.jsx
@@ -18,14 +18,15 @@ const NewServiceForm = (props: Props) => {
     providerAdminServiceDiscoveryServicesPath, adminServicesPath} = props
 
   const [formMode, setFormMode] = useState('manual')
+  const [loadingProjects, setLoadingProjects] = useState(false)
 
-  const handleFormsVisibility = (event: SyntheticEvent<HTMLSelectElement>) => {
+  const handleFormsVisibility = (event: SyntheticEvent<HTMLInputElement>) => {
     setFormMode(event.currentTarget.value)
   }
 
   const formToRender = () => formMode === 'manual' || !isServiceDiscoveryAccessible
     ? <ServiceManualForm formActionPath={adminServicesPath}/>
-    : <ServiceDiscoveryForm formActionPath={providerAdminServiceDiscoveryServicesPath}/>
+    : <ServiceDiscoveryForm formActionPath={providerAdminServiceDiscoveryServicesPath} setLoadingProjects={setLoadingProjects} />
 
   return (
     <React.Fragment>
@@ -35,6 +36,7 @@ const NewServiceForm = (props: Props) => {
           isServiceDiscoveryUsable={isServiceDiscoveryUsable}
           serviceDiscoveryAuthenticateUrl={serviceDiscoveryAuthenticateUrl}
           handleFormsVisibility={handleFormsVisibility}
+          loadingProjects={loadingProjects}
         />
       }
       {formToRender()}

diff --git a/app/javascript/src/NewService/components/ServiceDiscoveryForm.jsx b/app/javascript/src/NewService/components/ServiceDiscoveryForm.jsx
@@ -1,47 +1,43 @@
 // @flow
 
-import React from 'react'
-import {useState, useEffect} from 'react'
+import React, {useEffect} from 'react'
 
 import {FormWrapper, ErrorMessage,
   ServiceDiscoveryListItems} from 'NewService/components/FormElements'
 import {fetchData} from 'utilities/utils'
-import type {FormProps} from 'NewService/types'
 
-const BASE_PATH = '/p/admin/service_discovery'
-const PROJECTS_PATH = `${BASE_PATH}/projects.json`
+import {PROJECTS_PATH} from 'NewService'
 
-const ServiceDiscoveryForm = ({formActionPath}: {formActionPath: string}) => {
-  const [projects, setProjects] = useState([])
-  const [services, setServices] = useState([])
-  const [fetchErrorMessage, setFetchErrorMessage] = useState('')
+type Props = {
+  formActionPath: string,
+  setLoadingProjects: boolean => void
+}
+
+const ServiceDiscoveryForm = ({formActionPath, setLoadingProjects}: Props) => {
+  // Don't use named imports so that useState can be mocked in specs
+  const [projects, setProjects] = React.useState([])
+  const [fetchErrorMessage, setFetchErrorMessage] = React.useState('')
 
   const fetchProjects = async () => {
-    try {
-      const data = await fetchData(PROJECTS_PATH)
-      setProjects(data['projects'])
-      fetchServices(data.projects[0].metadata.name)
-    } catch (error) {
-      setFetchErrorMessage(error.message)
-    }
-  }
+    setLoadingProjects(true)
 
-  const fetchServices = async (namespace: string) => {
     try {
-      const data = await fetchData(`${BASE_PATH}/namespaces/${namespace}/services.json`)
-      setServices(data['services'])
+      const projects = await fetchData<string[]>(PROJECTS_PATH)
+      setProjects(projects)
     } catch (error) {
       setFetchErrorMessage(error.message)
+    } finally {
+      setLoadingProjects(false)
     }
   }
 
-  const listItemsProps = {fetchServices, projects, services}
+  const listItemsProps = {projects, onError: setFetchErrorMessage}
 
   useEffect(() => {
     fetchProjects()
   }, [])
 
-  const formProps: FormProps = {
+  const formProps = {
     id: 'service_source',
     formActionPath,
     hasHiddenServiceDiscoveryInput: true,

diff --git a/app/javascript/src/NewService/components/ServiceSourceForm.jsx b/app/javascript/src/NewService/components/ServiceSourceForm.jsx
@@ -5,29 +5,45 @@ import React from 'react'
 type Props = {
   isServiceDiscoveryUsable: boolean,
   serviceDiscoveryAuthenticateUrl: string,
-  handleFormsVisibility: (event: SyntheticEvent<HTMLInputElement>) => void
+  handleFormsVisibility: (event: SyntheticEvent<HTMLInputElement>) => void,
+  loadingProjects: boolean
 }
 
 const ServiceSourceForm = (props: Props) => {
   const {isServiceDiscoveryUsable, serviceDiscoveryAuthenticateUrl,
-    handleFormsVisibility} = props
+    handleFormsVisibility, loadingProjects} = props
   const classNameDisabled = isServiceDiscoveryUsable ? '' : 'disabled'
   return (
     <form className="formtastic" id="new_service_source">
       <fieldset className="inputs">
         <ol>
           <li className="radio">
             <label htmlFor="source_manual">
-              <input type="radio" name="source" id="source_manual" value="manual" defaultChecked="defaultChecked" onChange={handleFormsVisibility}/>
+              <input
+                type="radio"
+                name="source"
+                id="source_manual"
+                value="manual"
+                disabled={loadingProjects}
+                defaultChecked="defaultChecked"
+                onChange={handleFormsVisibility}
+              />
               <span className="new-service-source-input">Define manually</span>
             </label>
           </li>
           <li className="radio">
-            <label htmlFor="source_discover"
-              className={classNameDisabled}>
-              <input type="radio" name="source" id="source_discover" value="discover" disabled={!isServiceDiscoveryUsable} onChange={handleFormsVisibility}/>
+            <label htmlFor="source_discover" className={classNameDisabled}>
+              <input
+                type="radio"
+                name="source"
+                id="source_discover"
+                value="discover"
+                disabled={!isServiceDiscoveryUsable || loadingProjects}
+                onChange={handleFormsVisibility}
+              />
               <span className="new-service-source-input">Import from OpenShift</span>
-              { isServiceDiscoveryUsable ||
+              {loadingProjects && <i className="fa fa-spinner fa-spin" />}
+              {isServiceDiscoveryUsable ||
                 <a href={serviceDiscoveryAuthenticateUrl}>
                   {' (Authenticate to enable this option)'}
                 </a>

diff --git a/app/javascript/src/NewService/index.jsx b/app/javascript/src/NewService/index.jsx
@@ -1,4 +1,9 @@
+// @flow
+
 export {NewServiceForm, NewServiceFormWrapper} from 'NewService/components/NewServiceForm'
 export {ServiceSourceForm} from 'NewService/components/ServiceSourceForm'
 export {ServiceDiscoveryForm} from 'NewService/components/ServiceDiscoveryForm'
 export {ServiceManualForm} from 'NewService/components/ServiceManualForm'
+
+export const BASE_PATH = '/p/admin/service_discovery'
+export const PROJECTS_PATH = `${BASE_PATH}/projects.json`
diff --git a/app/javascript/src/Types/libs/libdefs.js b/app/javascript/src/Types/libs/libdefs.js
@@ -9,7 +9,22 @@ declare var module: {
 
 // TODO: remove these module declarations when not failing
 declare module 'whatwg-fetch' {
-  declare module.exports: fetch
+  declare type Options = {
+    method?: string,
+    body?: string,
+    headers?: Object,
+    credentials?: 'omit' | 'same-origin' | 'include'
+  }
+  declare type Response = {
+    status: number,
+    statusText: string,
+    ok: boolean,
+    headers: any,
+    url: string,
+    text: () => Promise<string>,
+    json: () => Promise<Object>
+  }
+  declare export function fetch (url: string, options: ?Options): Promise<Response>
 }
 
 declare module 'core-js/fn/symbol' {