Merge pull request #25 from dhis2/fix/keep-selection-context

fix(top-bar): keep selection context in more cases
dhis2 · Jul 14, 2021 · 244bef5 · 244bef5
2 parents 422f72d + 163489b
commit 244bef5
Show file tree

Hide file tree

Showing 11 changed files with 157 additions and 28 deletions.
diff --git a/i18n/en.pot b/i18n/en.pot
@@ -5,8 +5,8 @@ msgstr ""
 "Content-Type: text/plain; charset=utf-8\n"
 "Content-Transfer-Encoding: 8bit\n"
 "Plural-Forms: nplurals=2; plural=(n != 1)\n"
-"POT-Creation-Date: 2021-07-07T12:40:17.704Z\n"
-"PO-Revision-Date: 2021-07-07T12:40:17.704Z\n"
+"POT-Creation-Date: 2021-07-14T10:11:31.114Z\n"
+"PO-Revision-Date: 2021-07-14T10:11:31.114Z\n"
 
 msgid "Not authorized"
 msgstr "Not authorized"
@@ -72,9 +72,6 @@ msgstr "Cannot approve"
 msgid "Clear selections"
 msgstr "Clear selections"
 
-msgid "Choose an organisation unit"
-msgstr "Choose an organisation unit"
-
 msgid "Choose a period first"
 msgstr "Choose a period first"
 
@@ -84,6 +81,9 @@ msgstr "Choose a workflow and period first"
 msgid "Organisation Unit"
 msgstr "Organisation Unit"
 
+msgid "Choose an organisation unit"
+msgstr "Choose an organisation unit"
+
 msgid "January"
 msgstr "January"
 
@@ -177,21 +177,21 @@ msgstr "Financial year (Start July)"
 msgid "Financial year (Start April)"
 msgstr "Financial year (Start April)"
 
-msgid "Choose a period"
-msgstr "Choose a period"
-
 msgid "Period"
 msgstr "Period"
 
+msgid "Choose a period"
+msgstr "Choose a period"
+
 msgid "Choose a workflow first"
 msgstr "Choose a workflow first"
 
-msgid "Choose a workflow"
-msgstr "Choose a workflow"
-
 msgid "Workflow"
 msgstr "Workflow"
 
+msgid "Choose a workflow"
+msgstr "Choose a workflow"
+
 msgid "No workflows found. None may exist, or you may not have access to any."
 msgstr "No workflows found. None may exist, or you may not have access to any."
 

diff --git a/src/top-bar/context-select/context-select.js b/src/top-bar/context-select/context-select.js
@@ -12,6 +12,7 @@ import classes from './context-select.module.css'
 const ContextSelect = ({
     children,
     prefix,
+    placeholder,
     value,
     onClose,
     onOpen,
@@ -29,7 +30,9 @@ const ContextSelect = ({
             disabled={disabled}
         >
             <span className={classes.prefix}>{prefix}</span>
-            {!disabled && <span className={classes.value}>{value}</span>}
+            <span className={classes.value}>
+                {value || (!disabled && placeholder)}
+            </span>
             <Icon color={disabled ? colors.grey600 : undefined} />
         </button>
     )
@@ -68,13 +71,14 @@ const ContextSelect = ({
 
 ContextSelect.propTypes = {
     children: PropTypes.node.isRequired,
+    placeholder: PropTypes.string.isRequired,
     prefix: PropTypes.string.isRequired,
-    value: PropTypes.string.isRequired,
     onClose: PropTypes.func.isRequired,
     onOpen: PropTypes.func.isRequired,
     disabled: PropTypes.bool,
     open: PropTypes.bool,
     requiredValuesMessage: PropTypes.string,
+    value: PropTypes.string,
 }
 
 export { ContextSelect }
diff --git a/src/top-bar/context-select/context-select.module.css b/src/top-bar/context-select/context-select.module.css
@@ -11,7 +11,7 @@
 }
 .prefix {
     color: var(--colors-grey600);
-    padding-right: var(--spacers-dp4);
+    margin-right: var(--spacers-dp8);
 }
 .value {
     flex-grow: 1;

diff --git a/src/top-bar/context-select/context-select.test.js b/src/top-bar/context-select/context-select.test.js
@@ -12,6 +12,7 @@ import { ContextSelect } from './context-select.js'
 describe('<ContextSelect>', () => {
     const baseProps = {
         prefix: 'prefix',
+        placeholder: 'placeholder',
         value: 'value',
         onClose: () => {},
         onOpen: () => {},

diff --git a/src/top-bar/org-unit-select/org-unit-select.js b/src/top-bar/org-unit-select/org-unit-select.js
@@ -19,7 +19,7 @@ const OrgUnitSelect = () => {
         setOpenedSelect,
     } = useSelectionContext()
     const open = openedSelect === ORG_UNIT
-    const value = orgUnit.displayName || i18n.t('Choose an organisation unit')
+    const value = orgUnit.displayName
     const requiredValuesMessage = workflow.id
         ? i18n.t('Choose a period first')
         : i18n.t('Choose a workflow and period first')
@@ -32,6 +32,7 @@ const OrgUnitSelect = () => {
     return (
         <ContextSelect
             prefix={i18n.t('Organisation Unit')}
+            placeholder={i18n.t('Choose an organisation unit')}
             value={value}
             open={open}
             disabled={!(workflow.id && period.id)}

diff --git a/src/top-bar/org-unit-select/org-unit-select.test.js b/src/top-bar/org-unit-select/org-unit-select.test.js
@@ -69,7 +69,61 @@ describe('<OrgUnitSelect>', () => {
         expect(wrapper.find(OrganisationUnitTree)).toHaveLength(1)
     })
 
-    it('renders a placeholder text when no organisation unit is selected', () => {
+    it('is enabled if workflow and period have been set', () => {
+        useSelectionContext.mockImplementation(() => ({
+            workflow: mockWorkflows[0],
+            period: {
+                id: '20120402',
+            },
+            orgUnit: {},
+            openedSelect: '',
+            selectWorkflow: () => {},
+            setOpenedSelect: () => {},
+        }))
+        const wrapper = shallow(<OrgUnitSelect />)
+
+        expect(wrapper.find(ContextSelect).prop('disabled')).toBe(false)
+    })
+
+    it('is disabled if workflow and period have not been set yet', () => {
+        useSelectionContext.mockImplementation(() => ({
+            workflow: {},
+            period: {},
+            orgUnit: {},
+            openedSelect: '',
+            selectWorkflow: () => {},
+            setOpenedSelect: () => {},
+        }))
+        const wrapper = shallow(<OrgUnitSelect />)
+
+        expect(wrapper.find(ContextSelect).prop('disabled')).toBe(true)
+    })
+
+    it('renders a placeholder text when enabled but no organisation unit is selected', () => {
+        useSelectionContext.mockImplementation(() => ({
+            workflow: mockWorkflows[0],
+            period: {
+                id: '20120402',
+            },
+            orgUnit: {},
+            openedSelect: '',
+            selectWorkflow: () => {},
+            setOpenedSelect: () => {},
+        }))
+        const wrapper = shallow(<OrgUnitSelect />)
+        const placeholder = 'Choose an organisation unit'
+
+        expect(wrapper.find(ContextSelect).prop('disabled')).toBe(false)
+        expect(wrapper.find(ContextSelect).prop('value')).toBe(undefined)
+        expect(wrapper.find(ContextSelect).prop('placeholder')).toBe(
+            placeholder
+        )
+        expect(
+            wrapper.find(ContextSelect).shallow().text().includes(placeholder)
+        ).toBe(true)
+    })
+
+    it('does not render placeholder text when disabled and no organisation unit is selected', () => {
         useSelectionContext.mockImplementation(() => ({
             workflow: {},
             period: {},
@@ -79,10 +133,16 @@ describe('<OrgUnitSelect>', () => {
             setOpenedSelect: () => {},
         }))
         const wrapper = shallow(<OrgUnitSelect />)
+        const placeholder = 'Choose an organisation unit'
 
-        expect(wrapper.find(ContextSelect).prop('value')).toBe(
-            'Choose an organisation unit'
+        expect(wrapper.find(ContextSelect).prop('disabled')).toBe(true)
+        expect(wrapper.find(ContextSelect).prop('value')).toBe(undefined)
+        expect(wrapper.find(ContextSelect).prop('placeholder')).toBe(
+            placeholder
         )
+        expect(
+            wrapper.find(ContextSelect).shallow().text().includes(placeholder)
+        ).toBe(false)
     })
 
     it('renders the value when a organisation unit is selected', () => {

diff --git a/src/top-bar/period-select/period-select.js b/src/top-bar/period-select/period-select.js
@@ -12,7 +12,7 @@ const PeriodSelect = () => {
         useSelectionContext()
     const [year, setYear] = useState(period.year || currentYear)
     const open = openedSelect === PERIOD
-    const value = period.displayName || i18n.t('Choose a period')
+    const value = period.displayName
 
     useEffect(() => {
         if (period.year) {
@@ -23,6 +23,7 @@ const PeriodSelect = () => {
     return (
         <ContextSelect
             prefix={i18n.t('Period')}
+            placeholder={i18n.t('Choose a period')}
             value={value}
             open={open}
             disabled={!workflow.id}

diff --git a/src/top-bar/period-select/period-select.test.js b/src/top-bar/period-select/period-select.test.js
@@ -61,7 +61,7 @@ describe('<PeriodSelect>', () => {
         expect(wrapper.find(PeriodMenu)).toHaveLength(1)
     })
 
-    it('renders a placeholder text when no period is selected', () => {
+    it('is enabled if a workflow has been set', () => {
         useSelectionContext.mockImplementation(() => ({
             workflow: mockWorkflows[0],
             period: {},
@@ -71,9 +71,62 @@ describe('<PeriodSelect>', () => {
         }))
         const wrapper = shallow(<PeriodSelect />)
 
-        expect(wrapper.find(ContextSelect).prop('value')).toBe(
-            'Choose a period'
+        expect(wrapper.find(ContextSelect).prop('disabled')).toBe(false)
+    })
+
+    it('is disabled if a workflow has not been set', () => {
+        useSelectionContext.mockImplementation(() => ({
+            workflow: {},
+            period: {},
+            openedSelect: '',
+            selectPeriod: () => {},
+            setOpenedSelect: () => {},
+        }))
+        const wrapper = shallow(<PeriodSelect />)
+
+        expect(wrapper.find(ContextSelect).prop('disabled')).toBe(true)
+    })
+
+    it('renders a placeholder text when enabled and no period is selected', () => {
+        useSelectionContext.mockImplementation(() => ({
+            workflow: mockWorkflows[0],
+            period: {},
+            openedSelect: '',
+            selectPeriod: () => {},
+            setOpenedSelect: () => {},
+        }))
+        const wrapper = shallow(<PeriodSelect />)
+        const placeholder = 'Choose a period'
+
+        expect(wrapper.find(ContextSelect).prop('disabled')).toBe(false)
+        expect(wrapper.find(ContextSelect).prop('value')).toBe(undefined)
+        expect(wrapper.find(ContextSelect).prop('placeholder')).toBe(
+            placeholder
+        )
+        expect(
+            wrapper.find(ContextSelect).shallow().text().includes(placeholder)
+        ).toBe(true)
+    })
+
+    it('does not render a placeholder text when disabled and no period is selected', () => {
+        useSelectionContext.mockImplementation(() => ({
+            workflow: {},
+            period: {},
+            openedSelect: '',
+            selectPeriod: () => {},
+            setOpenedSelect: () => {},
+        }))
+        const wrapper = shallow(<PeriodSelect />)
+        const placeholder = 'Choose a period'
+
+        expect(wrapper.find(ContextSelect).prop('disabled')).toBe(true)
+        expect(wrapper.find(ContextSelect).prop('value')).toBe(undefined)
+        expect(wrapper.find(ContextSelect).prop('placeholder')).toBe(
+            placeholder
         )
+        expect(
+            wrapper.find(ContextSelect).shallow().text().includes(placeholder)
+        ).toBe(false)
     })
 
     it('renders a the value when a period is selected', () => {

diff --git a/src/top-bar/selection-context/selection-provider.js b/src/top-bar/selection-context/selection-provider.js
@@ -30,10 +30,14 @@ const reducer = (state, { type, payload }) => {
             }
         case ACTIONS.SELECT_WORKFLOW:
             return {
+                ...state,
                 openedSelect: '',
                 workflow: payload.workflow,
-                period: {},
-                orgUnit: {},
+                period:
+                    state.workflow &&
+                    state.workflow.periodType === payload.workflow.periodType
+                        ? state.period
+                        : {},
             }
         case ACTIONS.SELECT_PERIOD:
             return {
@@ -44,7 +48,6 @@ const reducer = (state, { type, payload }) => {
                  */
                 openedSelect: payload.period.id ? '' : state.openedSelect,
                 period: payload.period,
-                orgUnit: {},
             }
         case ACTIONS.SELECT_ORG_UNIT:
             return {

diff --git a/src/top-bar/workflow-select/workflow-select.js b/src/top-bar/workflow-select/workflow-select.js
@@ -18,11 +18,12 @@ const WorkflowSelect = () => {
         setOpenedSelect,
     } = useSelectionContext()
     const open = openedSelect === WORKFLOW
-    const value = selectedWorkflow.displayName || i18n.t('Choose a workflow')
+    const value = selectedWorkflow.displayName
 
     return (
         <ContextSelect
             prefix={i18n.t('Workflow')}
+            placeholder={i18n.t('Choose a workflow')}
             value={value}
             open={open}
             onOpen={() => setOpenedSelect(WORKFLOW)}

diff --git a/src/top-bar/workflow-select/workflow-select.test.js b/src/top-bar/workflow-select/workflow-select.test.js
@@ -64,10 +64,15 @@ describe('<WorkflowSelect>', () => {
             setOpenedSelect: () => {},
         }))
         const wrapper = shallow(<WorkflowSelect />)
+        const placeholder = 'Choose a workflow'
 
-        expect(wrapper.find(ContextSelect).prop('value')).toBe(
-            'Choose a workflow'
+        expect(wrapper.find(ContextSelect).prop('value')).toBe(undefined)
+        expect(wrapper.find(ContextSelect).prop('placeholder')).toBe(
+            placeholder
         )
+        expect(
+            wrapper.find(ContextSelect).shallow().text().includes(placeholder)
+        ).toBe(true)
     })
 
     it('renders a the value when a workflow is selected', () => {