Don't use property value for key construction. (#2317)

* Don't use property value for key construction.

* Use value from props as current value in `DateRange` component:
 - this allows to update it properly in the dialog when the value on the server is updated
 - current value is stored in `PropertyValueElement` similar to other property value components
 - fixed stale closure in `Modal` component
 - unit tests for `DateRange` updated (wrapper with current value created)

webapp/src/components/cardDetail/cardDetailProperties.tsx

@@ -135,10 +135,9 @@ const CardDetailProperties = (props: Props) => {
     return (
         <div className='octo-propertylist CardDetailProperties'>
             {board.fields.cardProperties.map((propertyTemplate: IPropertyTemplate) => {
-                const propertyValue = card.fields.properties[propertyTemplate.id]
                 return (
                     <div
-                        key={propertyTemplate.id + '-' + propertyTemplate.type + '-' + propertyValue}
+                        key={propertyTemplate.id + '-' + propertyTemplate.type}
                         className='octo-propertyrow'
                     >
                         {props.readonly && <div className='octo-propertyname octo-propertyname--readonly'>{propertyTemplate.name}</div>}

webapp/src/components/modal.tsx

@@ -1,6 +1,6 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
-import React, {useRef, useEffect} from 'react'
+import React, {useRef, useEffect, useCallback} from 'react'
 
 import IconButton from '../widgets/buttons/iconButton'
 import CloseIcon from '../widgets/icons/close'
@@ -17,20 +17,19 @@ const Modal = (props: Props): JSX.Element => {
 
     const {position, onClose, children} = props
 
-    const closeOnBlur = (e: Event) => {
+    const closeOnBlur = useCallback((e: Event) => {
         if (e.target && node.current?.contains(e.target as Node)) {
             return
         }
-
         onClose()
-    }
+    }, [onClose])
 
     useEffect(() => {
         document.addEventListener('click', closeOnBlur, true)
         return () => {
             document.removeEventListener('click', closeOnBlur, true)
         }
-    }, [])
+    }, [closeOnBlur])
 
     return (
         <div

webapp/src/components/properties/dateRange/dateRange.test.tsx

@@ -1,7 +1,7 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
 
-import React from 'react'
+import React, {useState} from 'react'
 import {render} from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
 import {IntlProvider} from 'react-intl'
@@ -17,6 +17,27 @@ const June15 = new Date(Date.UTC(new Date().getFullYear(), 5, 15, 12))
 const June15Local = new Date(new Date().getFullYear(), 5, 15, 12)
 const June20 = new Date(Date.UTC(new Date().getFullYear(), 5, 20, 12))
 
+type Props = {
+    initialValue?: string
+    showEmptyPlaceholder?: boolean
+    onChange?: (value: string) => void
+}
+
+const DateRangeWrapper = (props: Props): JSX.Element => {
+    const [value, setValue] = useState(props.initialValue || '')
+    return (
+        <DateRange
+            className='octo-propertyvalue'
+            value={value}
+            showEmptyPlaceholder={props.showEmptyPlaceholder}
+            onChange={(newValue) => {
+                setValue(newValue)
+                props.onChange?.(newValue)
+            }}
+        />
+    )
+}
+
 describe('components/properties/dateRange', () => {
     beforeEach(() => {
         // Quick fix to disregard console error when unmounting a component
@@ -26,9 +47,8 @@ describe('components/properties/dateRange', () => {
 
     test('returns default correctly', () => {
         const component = wrapIntl(
-            <DateRange
+            <DateRangeWrapper
-                className='octo-propertyvalue'
+                initialValue=''
-                value={''}
                 onChange={jest.fn()}
             />,
         )
@@ -40,9 +60,8 @@ describe('components/properties/dateRange', () => {
     test('returns local correctly - es local', () => {
         const component = (
             <IntlProvider locale='es'>
-                <DateRange
+                <DateRangeWrapper
-                    className='octo-propertyvalue'
+                    initialValue={June15Local.getTime().toString()}
-                    value={June15Local.getTime().toString()}
                     onChange={jest.fn()}
                 />
             </IntlProvider>
@@ -57,9 +76,8 @@ describe('components/properties/dateRange', () => {
     test('handles calendar click event', () => {
         const callback = jest.fn()
         const component = wrapIntl(
-            <DateRange
+            <DateRangeWrapper
-                className='octo-propertyvalue'
+                initialValue=''
-                value={''}
                 showEmptyPlaceholder={true}
                 onChange={callback}
             />,
@@ -84,9 +102,8 @@ describe('components/properties/dateRange', () => {
     test('handles setting range', () => {
         const callback = jest.fn()
         const component = wrapIntl(
-            <DateRange
+            <DateRangeWrapper
-                className='octo-propertyvalue'
+                initialValue={''}
-                value={''}
                 showEmptyPlaceholder={true}
                 onChange={callback}
             />,
@@ -121,9 +138,8 @@ describe('components/properties/dateRange', () => {
     test('handle clear', () => {
         const callback = jest.fn()
         const component = wrapIntl(
-            <DateRange
+            <DateRangeWrapper
-                className='octo-propertyvalue'
+                initialValue={June15Local.getTime().toString()}
-                value={June15Local.getTime().toString()}
                 onChange={callback}
             />,
         )
@@ -146,9 +162,8 @@ describe('components/properties/dateRange', () => {
     test('set via text input', () => {
         const callback = jest.fn()
         const component = wrapIntl(
-            <DateRange
+            <DateRangeWrapper
-                className='octo-propertyvalue'
+                initialValue={'{"from": ' + June15.getTime().toString() + ',"to": ' + June20.getTime().toString() + '}'}
-                value={'{"from": ' + June15.getTime().toString() + ',"to": ' + June20.getTime().toString() + '}'}
                 onChange={callback}
             />,
         )
@@ -183,9 +198,8 @@ describe('components/properties/dateRange', () => {
 
         const component = (
             <IntlProvider locale='es'>
-                <DateRange
+                <DateRangeWrapper
-                    className='octo-propertyvalue'
+                    initialValue={'{"from": ' + June15.getTime().toString() + ',"to": ' + June20.getTime().toString() + '}'}
-                    value={'{"from": ' + June15.getTime().toString() + ',"to": ' + June20.getTime().toString() + '}'}
                     onChange={callback}
                 />
             </IntlProvider>
@@ -218,9 +232,8 @@ describe('components/properties/dateRange', () => {
     test('cancel set via text input', () => {
         const callback = jest.fn()
         const component = wrapIntl(
-            <DateRange
+            <DateRangeWrapper
-                className='octo-propertyvalue'
+                initialValue={'{"from": ' + June15.getTime().toString() + ',"to": ' + June20.getTime().toString() + '}'}
-                value={'{"from": ' + June15.getTime().toString() + ',"to": ' + June20.getTime().toString() + '}'}
                 onChange={callback}
             />,
         )
@@ -248,9 +261,8 @@ describe('components/properties/dateRange', () => {
     test('handles `Today` button click event', () => {
         const callback = jest.fn()
         const component = wrapIntl(
-            <DateRange
+            <DateRangeWrapper
-                className='octo-propertyvalue'
+                initialValue={''}
-                value={''}
                 showEmptyPlaceholder={true}
                 onChange={callback}
             />,

webapp/src/components/properties/dateRange/dateRange.tsx

@@ -1,6 +1,6 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
-import React, {useState} from 'react'
+import React, {useMemo, useState} from 'react'
 import {useIntl} from 'react-intl'
 import {DateUtils} from 'react-day-picker'
 import MomentLocaleUtils from 'react-day-picker/moment'
@@ -50,6 +50,10 @@ export function createDatePropertyFromString(initialValue: string) : DatePropert
     return dateProperty
 }
 
+function datePropertyToString(dateProperty: DateProperty): string {
+    return dateProperty.from || dateProperty.to ? JSON.stringify(dateProperty) : ''
+}
+
 const loadedLocales: Record<string, moment.Locale> = {}
 
 function DateRange(props: Props): JSX.Element {
@@ -68,7 +72,7 @@ function DateRange(props: Props): JSX.Element {
         return new Date(date).getTimezoneOffset() * 60 * 1000
     }
 
-    const [dateProperty, setDateProperty] = useState<DateProperty>(createDatePropertyFromString(value as string))
+    const dateProperty = useMemo(() => createDatePropertyFromString(value as string), [value])
     const [showDialog, setShowDialog] = useState(false)
 
     // Keep dateProperty as UTC,
@@ -127,7 +131,7 @@ function DateRange(props: Props): JSX.Element {
             rangeUTC.to -= dateProperty.includeTime ? 0 : timeZoneOffset(rangeUTC.to)
         }
 
-        setDateProperty(rangeUTC)
+        onChange(datePropertyToString(rangeUTC))
         setFromInput(getDisplayDate(range.from ? new Date(range.from) : undefined))
         setToInput(getDisplayDate(range.to ? new Date(range.to) : undefined))
     }
@@ -141,16 +145,7 @@ function DateRange(props: Props): JSX.Element {
     }
 
     const onClose = () => {
-        // not actually setting here,
+        onChange(datePropertyToString(dateProperty))
-        // but using to retreive the current state
-        setDateProperty((current) => {
-            if (current && current.from) {
-                onChange(JSON.stringify(current))
-            } else {
-                onChange('')
-            }
-            return {...current}
-        })
         setShowDialog(false)
     }
 

webapp/src/components/propertyValueElement.tsx

@@ -171,7 +171,12 @@ const PropertyValueElement = (props:Props): JSX.Element => {
                 className='octo-propertyvalue'
                 value={value.toString()}
                 showEmptyPlaceholder={showEmptyPlaceholder}
-                onChange={(newValue) => mutator.changePropertyValue(card, propertyTemplate.id, newValue)}
+                onChange={(newValue) => {
+                    if (value !== newValue) {
+                        setValue(newValue)
+                        mutator.changePropertyValue(card, propertyTemplate.id, newValue)
+                    }
+                }}
             />
         )
     } else if (propertyTemplate.type === 'url') {