Merge pull request #732 from nowcommunity/button-for-changing-time-bound-inside-time-unit

Bug fix in editingComponents error checking function, and changed the way time bounds are displayed/selected in time unit view.
Bypassing the rules and merging.

cypress/e2e/timeUnit.cy.js

@@ -12,9 +12,9 @@ describe('Creating a time unit', () => {
     cy.get('[id=tu_display_name-textfield]').type('Testing Time Unit Creation')
     cy.get('[id=sequence-tableselection]').first().click() // sequence field
     cy.get('[data-cy=detailview-button-ALMAAsianlandmammalage]').first().click()
-    cy.get('[id=up_bnd-tableselection]').first().click() // New Upper Bound Id field
+    cy.get('[id=up_bnd-tableselection]').first().click() // Select Upper Bound Button
     cy.get('[data-cy=detailview-button-11]').first().click()
-    cy.get('[id=low_bnd-tableselection]').first().click() // New Lower Bound Id field
+    cy.get('[id=low_bnd-tableselection]').first().click() // Select Lower Bound Button
     cy.get('[data-cy=detailview-button-14]').first().click()
     cy.get('[id=write-button]').click()
     cy.get('[id=write-button]').should('be.disabled')
@@ -27,6 +27,7 @@ describe('Creating a time unit', () => {
     cy.contains('ALMAAsianlandmammalage')
     cy.contains('C2N-o')
     cy.contains('C2N-y')
+    cy.contains('Creating new time-unit').should('not.exist')
   })
 
   it('without bounds does not work', () => {
@@ -34,14 +35,38 @@ describe('Creating a time unit', () => {
     cy.get('[id=tu_display_name-textfield]').type('Testing Time Unit Creation')
     cy.get('[id=sequence-tableselection]').first().click()
     cy.get('[data-cy=detailview-button-ALMAAsianlandmammalage]').first().click()
-    cy.contains('This field is required')
+    cy.contains('Upper Bound: This field is required')
+    cy.contains('Lower Bound: This field is required')
     cy.get('[id=write-button]').should('be.disabled')
-    cy.contains('2 Invalid fields')
+  })
+
+  it('with incorrect bounds does not work', () => {
+    cy.visit(`/time-unit/new`)
+    cy.get('[id=tu_display_name-textfield]').type('Testing Time Unit Creation')
+    cy.get('[id=sequence-tableselection]').first().click()
+    cy.get('[data-cy=detailview-button-ALMAAsianlandmammalage]').first().click()
+    cy.get('[id=up_bnd-tableselection]').click()
+    cy.get('[data-cy=detailview-button-14]').first().click()
+    cy.get('[id=low_bnd-tableselection]').click()
+    cy.get('[data-cy=detailview-button-11]').first().click()
+    cy.contains('Upper Bound: Upper bound age has to be lower than lower bound age')
+    cy.contains('Lower Bound: Lower bound age has to be higher than upper bound age')
+    cy.get('[id=write-button]').should('be.disabled')
+
+    cy.get('[id=up_bnd-tableselection]').click()
+    cy.get('[data-cy=detailview-button-11]').first().click()
+    cy.contains('Upper Bound: Upper bound age cannot be the same as lower bound age')
+    cy.contains('Lower Bound: Lower bound age cannot be the same as upper bound age')
+    cy.get('[id=write-button]').should('be.disabled')
+
+    cy.get('[id=low_bnd-tableselection]').click()
+    cy.get('[data-cy=detailview-button-14]').first().click()
+    cy.get('[id=write-button]').should('not.be.disabled')
   })
 
   it('but returning from staging view before saving works', () => {
     cy.visit(`/time-unit/new`)
-    cy.get('[id=tu_display_name-textfield]').type('Testing Time Unit Creation part 2')
+    cy.get('[id=tu_display_name-textfield]').type('Testing Time Unit Creation')
     cy.get('[id=sequence-tableselection]').first().click()
     cy.get('[data-cy=detailview-button-ALMAAsianlandmammalage]').first().click()
     cy.get('[id=up_bnd-tableselection]').first().click()
@@ -52,10 +77,10 @@ describe('Creating a time unit', () => {
     cy.get('[id=return-to-editing-button]').click()
     cy.contains('Creating new time-unit')
     cy.get('[id=tu_display_name-textfield]').should('not.be.disabled')
-    cy.get('[id=tu_display_name-textfield]').should('have.value', 'Testing Time Unit Creation part 2')
+    cy.get('[id=tu_display_name-textfield]').should('have.value', 'Testing Time Unit Creation')
     cy.get('[id=sequence-tableselection]').should('have.value', 'ALMAAsianlandmammalage')
-    cy.get('[id=up_bnd-tableselection]').should('have.value', '11')
-    cy.get('[id=low_bnd-tableselection]').should('have.value', '14')
+    cy.contains('C2N-o')
+    cy.contains('C2N-y')
     cy.get('[id=write-button]').click()
     cy.get('[id=write-button]').should('be.disabled')
     cy.contains('button', 'Add existing reference').click()
@@ -89,6 +114,8 @@ describe('Creating a time unit', () => {
     cy.get('[id=edit-button]').click()
     cy.get('[id=sequence-tableselection]').first().click()
     cy.get('[data-cy=detailview-button-CalatayudTeruellocalbiozone]').first().click()
+    cy.get('[id=low_bnd-tableselection]').first().click()
+    cy.get('[data-cy=detailview-button-49]').first().click()
     cy.get('[id=write-button]').click()
     cy.get('[id=write-button]').should('be.disabled')
     cy.contains('button', 'Add existing reference').click()
@@ -97,6 +124,7 @@ describe('Creating a time unit', () => {
     cy.get('[id=write-button]').should('not.be.disabled')
     cy.get('[id=write-button]').click()
     cy.contains('CalatayudTeruellocalbiozone')
+    cy.contains('Langhian/Serravallian')
   })
 })
 
@@ -128,10 +156,19 @@ describe('Editing a time unit', () => {
     cy.get('[id=edit-button]').click()
     cy.get('[id=low_bnd-tableselection]').click()
     cy.get('[data-cy=detailview-button-11]').first().click()
-    cy.contains('​Upper bound age has to be lower than lower bound age')
-    cy.contains('Lower bound age has to be higher than upper bound age')
+    cy.contains('Upper Bound: Upper bound age has to be lower than lower bound age')
+    cy.contains('Lower Bound: Lower bound age has to be higher than upper bound age')
     cy.get('[id=write-button]').should('be.disabled')
-    cy.contains('2 Invalid fields')
+
+    cy.get('[id=up_bnd-tableselection]').click()
+    cy.get('[data-cy=detailview-button-11]').first().click()
+    cy.contains('Upper Bound: Upper bound age cannot be the same as lower bound age')
+    cy.contains('Lower Bound: Lower bound age cannot be the same as upper bound age')
+    cy.get('[id=write-button]').should('be.disabled')
+
+    cy.get('[id=low_bnd-tableselection]').click()
+    cy.get('[data-cy=detailview-button-14]').first().click()
+    cy.get('[id=write-button]').should('not.be.disabled')
   })
 
   it('but returning from staging view before saving works', () => {
@@ -148,8 +185,8 @@ describe('Editing a time unit', () => {
     cy.get('[id=tu_display_name-textfield]').should('be.disabled')
     cy.get('[id=tu_display_name-textfield]').should('have.value', 'Bahean')
     cy.get('[id=sequence-tableselection]').should('have.value', 'CalatayudTeruellocalbiozone')
-    cy.get('[id=up_bnd-tableselection]').should('have.value', '11')
-    cy.get('[id=low_bnd-tableselection]').should('have.value', '20213')
+    cy.contains('C2N-y')
+    cy.contains('MioceneLate-low')
     cy.get('[id=write-button]').click()
     cy.get('[id=write-button]').should('be.disabled')
     cy.contains('button', 'Add existing reference').click()
@@ -158,8 +195,8 @@ describe('Editing a time unit', () => {
     cy.get('[id=write-button]').should('not.be.disabled')
     cy.get('[id=write-button]').click()
     cy.contains('CalatayudTeruellocalbiozone')
-    cy.contains('MioceneLate-low')
     cy.contains('C2N-y')
+    cy.contains('MioceneLate-low')
   })
 
   it('and editing again after saving works', () => {
@@ -176,10 +213,13 @@ describe('Editing a time unit', () => {
     cy.get('[id=write-button]').should('not.be.disabled')
     cy.get('[id=write-button]').click()
     cy.contains('Bahean')
+    cy.contains('easternparatethys')
     cy.get('[id=edit-button]').click()
     cy.get('[id=tu_display_name-textfield]').should('be.disabled')
     cy.get('[id=sequence-tableselection]').first().click()
     cy.get('[data-cy=detailview-button-CalatayudTeruellocalbiozone]').first().click()
+    cy.get('[id=up_bnd-tableselection]').first().click()
+    cy.get('[data-cy=detailview-button-14]').first().click()
     cy.get('[id=write-button]').click()
     cy.get('[id=write-button]').should('be.disabled')
     cy.contains('button', 'Add existing reference').click()
@@ -187,7 +227,7 @@ describe('Editing a time unit', () => {
     cy.contains('button', 'Close').click()
     cy.get('[id=write-button]').should('not.be.disabled')
     cy.get('[id=write-button]').click()
-    cy.contains('Bahean')
     cy.contains('CalatayudTeruellocalbiozone')
+    cy.contains('C2N-o')
   })
 })

frontend/src/components/DetailView/common/checkFieldErrors.tsx

@@ -9,7 +9,11 @@ export const checkFieldErrors = (
 ) => {
   const fieldAsString = String(field)
   if (errorObject.error) {
-    if (!(fieldAsString in fieldsWithErrors)) {
+    if (
+      !(fieldAsString in fieldsWithErrors) ||
+      fieldsWithErrors[fieldAsString].name !== errorObject.name ||
+      fieldsWithErrors[fieldAsString].error !== errorObject.error
+    ) {
       setFieldsWithErrors(prevFieldsWithErrors => {
         return { ...prevFieldsWithErrors, [fieldAsString]: errorObject }
       })

frontend/src/components/DetailView/common/editingComponents.tsx

@@ -19,9 +19,10 @@ import { RegisterOptions, FieldValues, UseFormRegisterReturn, FieldErrors } from
 import { useDetailContext } from '../Context/DetailContext'
 import { DataValue } from './tabLayoutHelpers'
 import { modalStyle } from './misc'
-import { EditDataType, TimeUnitDetailsType, LocalityDetailsType, TimeUnit } from '@/shared/types'
+import { EditDataType, TimeUnitDetailsType, LocalityDetailsType, TimeUnit, TimeBoundDetailsType } from '@/shared/types'
 import { calculateLocalityMinAge, calculateLocalityMaxAge } from '@/util/ageCalculator'
 import { checkFieldErrors } from './checkFieldErrors'
+import { TimeBoundTable } from '@/components/TimeBound/TimeBoundTable'
 
 const fieldWidth = '14em'
 
@@ -391,30 +392,26 @@ export const FieldWithTableSelection = <T extends object, ParentType extends obj
   return <DataValue<ParentType> field={targetField as keyof EditDataType<ParentType>} EditElement={editingComponent} />
 }
 
-export const TimeBoundSelection = <T extends object, ParentType extends object>({
+export const TimeBoundSelection = ({
   targetField,
-  sourceField,
-  selectorTable,
   disabled,
 }: {
-  targetField: keyof ParentType
-  sourceField: keyof T
-  selectorTable: ReactElement
+  targetField: keyof TimeUnitDetailsType
   disabled?: boolean
 }) => {
-  const { editData, setEditData, validator, fieldsWithErrors, setFieldsWithErrors } = useDetailContext<ParentType>()
+  const { editData, setEditData, validator, fieldsWithErrors, setFieldsWithErrors } =
+    useDetailContext<TimeUnitDetailsType>()
   const errorObject = validator(
     editData,
-    (targetField === 'up_bnd' ? 'up_bound' : 'low_bound') as keyof EditDataType<ParentType>
+    (targetField === 'up_bnd' ? 'up_bound' : 'low_bound') as keyof EditDataType<TimeUnitDetailsType>
   )
-  const { error } = errorObject
   const [open, setOpen] = useState(false)
 
-  const selectorFn = (selected: T) => {
+  const selectorFn = (selected: TimeBoundDetailsType) => {
     if (targetField === 'up_bnd') {
-      setEditData({ ...editData, [targetField]: selected[sourceField], ['up_bound']: selected })
+      setEditData({ ...editData, [targetField]: selected['bid'], ['up_bound']: selected })
     } else if (targetField === 'low_bnd') {
-      setEditData({ ...editData, [targetField]: selected[sourceField], ['low_bound']: selected })
+      setEditData({ ...editData, [targetField]: selected['bid'], ['low_bound']: selected })
     }
     setOpen(false)
   }
@@ -424,7 +421,7 @@ export const TimeBoundSelection = <T extends object, ParentType extends object>(
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [errorObject])
 
-  const selectorTableWithFn = cloneElement(selectorTable, { selectorFn })
+  const selectorTableWithFn = cloneElement(<TimeBoundTable showBid />, { selectorFn })
   if (open)
     return (
       <Box>
@@ -442,21 +439,16 @@ export const TimeBoundSelection = <T extends object, ParentType extends object>(
         </Modal>
       </Box>
     )
-  const editingComponent = (
-    <TextField
+  return (
+    <Button
       id={`${String(targetField)}-tableselection`}
-      variant="outlined"
-      size="small"
-      error={!!error}
-      helperText={error ?? ''}
-      value={editData[targetField as keyof EditDataType<ParentType>] ?? undefined}
       onClick={() => setOpen(true)}
+      variant="contained"
       disabled={disabled}
-      sx={{ backgroundColor: disabled ? 'grey' : '' }}
-      inputProps={{ readOnly: true }}
-    />
+    >
+      Select Time Bound
+    </Button>
   )
-  return <DataValue<ParentType> field={targetField as keyof EditDataType<ParentType>} EditElement={editingComponent} />
 }
 
 export const BasisForAgeSelection = ({

frontend/src/components/TimeUnit/Tabs/TimeUnitTab.tsx

@@ -1,13 +1,13 @@
-import { SequenceDetailsType, TimeBoundDetailsType, TimeUnitDetailsType } from '@/shared/types'
+import { SequenceDetailsType, TimeUnitDetailsType } from '@/shared/types'
 import { FieldWithTableSelection, TimeBoundSelection } from '@/components/DetailView/common/editingComponents'
 import { emptyOption } from '@/components/DetailView/common/misc'
-import { ArrayFrame } from '@/components/DetailView/common/tabLayoutHelpers'
+import { ArrayFrame, Grouped } from '@/components/DetailView/common/tabLayoutHelpers'
 import { useDetailContext } from '@/components/DetailView/Context/DetailContext'
 import { SequenceTable } from '@/components/Sequence/SequenceTable'
-import { TimeBoundTable } from '@/components/TimeBound/TimeBoundTable'
+import { Box } from '@mui/material'
 
 export const TimeUnitTab = () => {
-  const { textField, dropdown, data, editData, mode } = useDetailContext<TimeUnitDetailsType>()
+  const { textField, dropdown, data, editData, fieldsWithErrors, mode } = useDetailContext<TimeUnitDetailsType>()
 
   const rankOptions = [
     'Age',
@@ -44,15 +44,6 @@ export const TimeUnitTab = () => {
   const up_bcomment = editData.up_bound ? editData.up_bound.b_comment : data.up_bound ? data.up_bound.b_comment : ''
 
   const upBound = [
-    [
-      'Select New Upper Bound',
-      <TimeBoundSelection<TimeBoundDetailsType, TimeUnitDetailsType>
-        key="up_bnd"
-        sourceField="bid"
-        targetField="up_bnd"
-        selectorTable={<TimeBoundTable showBid />}
-      />,
-    ],
     ['Id', up_id],
     ['Name', up_bname],
     ['Age', up_age],
@@ -69,48 +60,32 @@ export const TimeUnitTab = () => {
       : ''
 
   const lowBound = [
-    [
-      'Select New Lower Bound',
-      <TimeBoundSelection<TimeBoundDetailsType, TimeUnitDetailsType>
-        key="low_bnd"
-        sourceField="bid"
-        targetField="low_bnd"
-        selectorTable={<TimeBoundTable showBid />}
-      />,
-    ],
     ['Id', low_id],
     ['Name', low_bname],
     ['Age', low_age],
     ['Comment', low_bcomment],
   ]
 
-  let upperBoundTitle: string
-  let lowerBoundTitle: string
-  if (!editData.up_bound || !data.up_bound) {
-    upperBoundTitle = 'Upper bound'
-  } else {
-    upperBoundTitle = editData.up_bound.bid === data.up_bound.bid ? 'Upper bound' : 'Upper bound (edited)'
-  }
-  if (!editData.low_bound || !data.low_bound) {
-    lowerBoundTitle = 'Lower bound'
-  } else {
-    lowerBoundTitle = editData.low_bound.bid === data.low_bound.bid ? 'Lower bound' : 'Lower bound (edited)'
-  }
-
   return (
     <>
       <ArrayFrame array={timeUnit} title="Time Unit" />
       <>
-        <ArrayFrame
-          array={upBound}
-          title={upperBoundTitle}
-          warning={upperBoundTitle === 'Upper bound (edited)' ? true : false}
-        />
-        <ArrayFrame
-          array={lowBound}
-          title={lowerBoundTitle}
-          warning={lowerBoundTitle === 'Lower bound (edited)' ? true : false}
-        />
+        <Grouped title="Upper Bound" error={'up_bnd' in fieldsWithErrors}>
+          {!mode.read && (
+            <Box display="flex" gap={1} marginBottom={'15px'}>
+              <TimeBoundSelection key="up_bnd" targetField="up_bnd" />
+            </Box>
+          )}
+          <ArrayFrame array={upBound} title={'Selected Upper Bound'} />
+        </Grouped>
+        <Grouped title="Lower Bound" error={'low_bnd' in fieldsWithErrors}>
+          {!mode.read && (
+            <Box display="flex" gap={1} marginBottom={'15px'}>
+              <TimeBoundSelection key="low_bnd" targetField="low_bnd" />
+            </Box>
+          )}
+          <ArrayFrame array={lowBound} title={'Selected Lower Bound'} />
+        </Grouped>
       </>
     </>
   )