From 0a92315d977fa4f30a20a4025b2907f0250e9c8d Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Thu, 30 Sep 2021 10:15:28 +0200 Subject: [PATCH] fix(bottom-bar): disable approve button when it is allowed but pointless (#100) * refactor: store approval statuses in constant * fix(bottom-bar): disable button when approving is allowed but pointless * chore: fix typo in property name * test(bottom-bar): adjust test so it can assert disabled buttons too --- cypress/integration/actions.feature | 40 +++++++-------- cypress/integration/actions/index.js | 6 +-- src/bottom-bar/bottom-bar.js | 14 +++++- .../approval-status/approval-status-icon.js | 15 ++---- .../approval-status/approval-status-tag.js | 11 +---- .../approval-status/get-approval-status.js | 49 ++++++++++++------- .../get-approval-status.test.js | 32 +++++++++--- src/shared/approval-status/index.js | 5 +- .../approval-status-icons-legend.js | 22 +++++++-- .../org-unit-select/approval-status-label.js | 6 +-- .../org-unit-select/approval-statuses.js | 7 +-- 11 files changed, 125 insertions(+), 82 deletions(-) diff --git a/cypress/integration/actions.feature b/cypress/integration/actions.feature index 67756650..ad65cc73 100644 --- a/cypress/integration/actions.feature +++ b/cypress/integration/actions.feature @@ -17,11 +17,11 @@ Feature: Users can approve, accept, unapprove, and unaccept data When the confirmation button is clicked Then a circular loader is rendered And the following buttons are available - | label | available | - | Approve | | - | Accept | yes | - | Unapprove | yes | - | Unaccept | | + | label | visible | disabled | + | Approve | | | + | Accept | yes | | + | Unapprove | yes | | + | Unaccept | | | # In "Approved" state the "Accept" action becomes available Scenario: User accepts data @@ -30,11 +30,11 @@ Feature: Users can approve, accept, unapprove, and unaccept data Then a circular loader is rendered And the status tag shows the approval status "Ready for approval — Accepted" And the following buttons are available - | label | available | - | Approve | yes | - | Accept | | - | Unapprove | yes | - | Unaccept | yes | + | label | visible | disabled | + | Approve | yes | yes | + | Accept | | | + | Unapprove | yes | | + | Unaccept | yes | | # In "Ready for approval — Accepted" state the "Unaccept" action becomes available Scenario: User unaccepts data @@ -43,11 +43,11 @@ Feature: Users can approve, accept, unapprove, and unaccept data Then a circular loader is rendered And the status tag shows the approval status "Approved" And the following buttons are available - | label | available | - | Approve | | - | Accept | yes | - | Unapprove | yes | - | Unaccept | | + | label | visible | disabled | + | Approve | | | + | Accept | yes | | + | Unapprove | yes | | + | Unaccept | | | # After unaccepting the state jumps back to "Approved" and the "Unapprove" action becomes available Scenario: User unapproves data @@ -56,8 +56,8 @@ Feature: Users can approve, accept, unapprove, and unaccept data Then a circular loader is rendered And the status tag shows the approval status "Ready for approval" And the following buttons are available - | label | available | - | Approve | yes | - | Accept | | - | Unapprove | | - | Unaccept | | \ No newline at end of file + | label | visible | disabled | + | Approve | yes | | + | Accept | | | + | Unapprove | | | + | Unaccept | | | \ No newline at end of file diff --git a/cypress/integration/actions/index.js b/cypress/integration/actions/index.js index c60832fb..e32b8b96 100644 --- a/cypress/integration/actions/index.js +++ b/cypress/integration/actions/index.js @@ -19,13 +19,13 @@ When('the user clicks the {buttonLabel} button', buttonLabel => { }) Then('the following buttons are available', dataTable => { - dataTable.hashes().forEach(({ label, available }) => { - if (available) { + dataTable.hashes().forEach(({ label, visible, disabled }) => { + if (visible) { cy.get('[data-test="bottom-bar"]') .find('button') .contains(label) .should('be.visible') - .and('not.be.disabled') + .and(disabled ? 'be.disabled' : 'not.be.disabled') } else { cy.get('[data-test="bottom-bar"]') .find('button') diff --git a/src/bottom-bar/bottom-bar.js b/src/bottom-bar/bottom-bar.js index f8ea125f..f0716f5c 100644 --- a/src/bottom-bar/bottom-bar.js +++ b/src/bottom-bar/bottom-bar.js @@ -1,5 +1,5 @@ import React from 'react' -import { ApprovalStatusTag } from '../shared/index.js' +import { ApprovalStatusTag, APPROVAL_STATUSES } from '../shared/index.js' import { useWorkflowContext } from '../workflow-context/index.js' import { AcceptButton } from './accept-button/index.js' import { ApproveButton } from './approve-button/index.js' @@ -8,11 +8,21 @@ import styles from './bottom-bar.module.css' import { UnacceptButton } from './unaccept-button/index.js' import { UnapproveButton } from './unapprove-button/index.js' +const approvedStatuses = new Set([ + APPROVAL_STATUSES.APPROVED_HERE, + APPROVAL_STATUSES.APPROVED_ABOVE, + APPROVAL_STATUSES.ACCEPTED_HERE, +]) + const BottomBar = () => { const { allowedActions, approvalStatus, approvedBy, approvedAt } = useWorkflowContext() const { mayAccept, mayApprove, mayUnaccept, mayUnapprove } = allowedActions - const disableApproveBtn = !mayApprove && !mayUnapprove + const disableApproveBtn = + /* We want to signal that the user can't approve or unapprove anything + by showing a disabled button rather than an empty space */ + (!mayApprove && !mayUnapprove) || + (mayApprove && approvedStatuses.has(approvalStatus)) return ( <> diff --git a/src/shared/approval-status/approval-status-icon.js b/src/shared/approval-status/approval-status-icon.js index 9cfa2d4d..0bb154cd 100644 --- a/src/shared/approval-status/approval-status-icon.js +++ b/src/shared/approval-status/approval-status-icon.js @@ -2,7 +2,10 @@ import { colors } from '@dhis2/ui' import PropTypes from 'prop-types' import React from 'react' import classes from './approval-status-icon.module.css' -import { getApprovalStatusDisplayData } from './get-approval-status.js' +import { + getApprovalStatusDisplayData, + APPROVAL_STATUSES, +} from './get-approval-status.js' const getIconColorForType = type => { switch (type) { @@ -39,15 +42,7 @@ ApprovalStatusIcon.defaultProps = { } ApprovalStatusIcon.propTypes = { - approvalStatus: PropTypes.oneOf([ - 'APPROVED_HERE', - 'APPROVED_ABOVE', - 'ACCEPTED_HERE', - 'UNAPPROVED_READY', - 'UNAPPROVED_WAITING', - 'UNAPPROVED_ABOVE', - 'UNAPPROVABLE', - ]), + approvalStatus: PropTypes.oneOf(Object.values(APPROVAL_STATUSES)), showTitle: PropTypes.bool, } diff --git a/src/shared/approval-status/approval-status-tag.js b/src/shared/approval-status/approval-status-tag.js index 001865d7..e6e72b2e 100644 --- a/src/shared/approval-status/approval-status-tag.js +++ b/src/shared/approval-status/approval-status-tag.js @@ -6,6 +6,7 @@ import React from 'react' import { getApprovalStatusDisplayData, isApproved, + APPROVAL_STATUSES, } from './get-approval-status.js' import { useServerDateTimeAsLocal } from './use-server-date-time-as-local.js' @@ -42,15 +43,7 @@ const ApprovalStatusTag = ({ approvalStatus, approvedAt, approvedBy }) => { } ApprovalStatusTag.propTypes = { - approvalStatus: PropTypes.oneOf([ - 'APPROVED_HERE', - 'APPROVED_ABOVE', - 'ACCEPTED_HERE', - 'UNAPPROVED_READY', - 'UNAPPROVED_WAITING', - 'UNAPPROVED_ABOVE', - 'UNAPPROVABLE', - ]), + approvalStatus: PropTypes.oneOf(Object.values(APPROVAL_STATUSES)), approvedAt: PropTypes.string, approvedBy: PropTypes.string, } diff --git a/src/shared/approval-status/get-approval-status.js b/src/shared/approval-status/get-approval-status.js index 5fb2bad6..849c51ec 100644 --- a/src/shared/approval-status/get-approval-status.js +++ b/src/shared/approval-status/get-approval-status.js @@ -3,32 +3,44 @@ import { IconBlock16, IconError16 } from '@dhis2/ui' import moment from 'moment' import { Approved, Ready, Waiting } from './icons.js' +const APPROVAL_STATUSES = { + UNAPPROVED_READY: 'UNAPPROVED_READY', + ACCEPTED_HERE: 'ACCEPTED_HERE', + UNAPPROVED_WAITING: 'UNAPPROVED_WAITING', + UNAPPROVED_ABOVE: 'UNAPPROVED_ABOVE', + APPROVED_HERE: 'APPROVED_HERE', + APPROVED_ABOVE: 'APPROVED_ABOVE', + UNAPPROVABLE: 'UNAPPROVABLE', + LOADING: 'LOADING', + ERROR: 'ERROR', +} + const getApprovalStatusIcon = approvalStatus => { switch (approvalStatus) { - case 'UNAPPROVED_READY': - case 'ACCEPTED_HERE': + case APPROVAL_STATUSES.UNAPPROVED_READY: + case APPROVAL_STATUSES.ACCEPTED_HERE: return { icon: Ready, type: 'neutral', } - case 'UNAPPROVED_WAITING': - case 'UNAPPROVED_ABOVE': + case APPROVAL_STATUSES.UNAPPROVED_WAITING: + case APPROVAL_STATUSES.UNAPPROVED_ABOVE: return { icon: Waiting, type: 'default', } - case 'APPROVED_HERE': - case 'APPROVED_ABOVE': + case APPROVAL_STATUSES.APPROVED_HERE: + case APPROVAL_STATUSES.APPROVED_ABOVE: return { icon: Approved, type: 'positive', } - case 'UNAPPROVABLE': + case APPROVAL_STATUSES.UNAPPROVABLE: return { icon: IconBlock16, type: 'negative', } - case 'ERROR': + case APPROVAL_STATUSES.ERROR: return { icon: IconError16, type: 'negative', @@ -58,20 +70,20 @@ const getApprovalStatusText = ({ approvedBy, }) => { switch (approvalStatus) { - case 'UNAPPROVED_READY': + case APPROVAL_STATUSES.UNAPPROVED_READY: return i18n.t('Ready for approval') - case 'ACCEPTED_HERE': + case APPROVAL_STATUSES.ACCEPTED_HERE: return i18n.t('Ready for approval — Accepted') - case 'UNAPPROVED_WAITING': + case APPROVAL_STATUSES.UNAPPROVED_WAITING: return i18n.t('Waiting for lower level approval') - case 'UNAPPROVED_ABOVE': + case APPROVAL_STATUSES.UNAPPROVED_ABOVE: return i18n.t('Waiting for higher level approval') - case 'APPROVED_HERE': - case 'APPROVED_ABOVE': + case APPROVAL_STATUSES.APPROVED_HERE: + case APPROVAL_STATUSES.APPROVED_ABOVE: return getApprovedStatusText({ approvalDateTime, approvedBy }) - case 'UNAPPROVABLE': + case APPROVAL_STATUSES.UNAPPROVABLE: return i18n.t('Cannot be approved') - case 'ERROR': + case APPROVAL_STATUSES.ERROR: return i18n.t('Could not retrieve approval status') default: throw new Error(`Unknown approval status: '${approvalStatus}'`) @@ -79,7 +91,8 @@ const getApprovalStatusText = ({ } const isApproved = approvalStatus => - approvalStatus === 'APPROVED_HERE' || approvalStatus === 'APPROVED_ABOVE' + approvalStatus === APPROVAL_STATUSES.APPROVED_HERE || + approvalStatus === APPROVAL_STATUSES.APPROVED_ABOVE const getApprovalStatusDisplayData = ({ approvalStatus, @@ -96,4 +109,4 @@ const getApprovalStatusDisplayData = ({ return { displayName, icon, type } } -export { getApprovalStatusDisplayData, isApproved } +export { APPROVAL_STATUSES, getApprovalStatusDisplayData, isApproved } diff --git a/src/shared/approval-status/get-approval-status.test.js b/src/shared/approval-status/get-approval-status.test.js index 53903558..5612244a 100644 --- a/src/shared/approval-status/get-approval-status.test.js +++ b/src/shared/approval-status/get-approval-status.test.js @@ -1,6 +1,9 @@ import { IconBlock16, IconError16 } from '@dhis2/ui' import moment from 'moment' -import { getApprovalStatusDisplayData } from './get-approval-status.js' +import { + getApprovalStatusDisplayData, + APPROVAL_STATUSES, +} from './get-approval-status.js' import { Approved, Ready, Waiting } from './icons.js' jest.mock('moment', () => { @@ -10,7 +13,9 @@ jest.mock('moment', () => { describe('getApprovalStatusDisplayData', () => { it('returns the correct display data for approval status "UNAPPROVED_READY"', () => { expect( - getApprovalStatusDisplayData({ approvalStatus: 'UNAPPROVED_READY' }) + getApprovalStatusDisplayData({ + approvalStatus: APPROVAL_STATUSES.UNAPPROVED_READY, + }) ).toEqual({ displayName: 'Ready for approval', icon: Ready, @@ -19,7 +24,9 @@ describe('getApprovalStatusDisplayData', () => { }) it('returns the correct display data for approval status "ACCEPTED_HERE"', () => { expect( - getApprovalStatusDisplayData({ approvalStatus: 'ACCEPTED_HERE' }) + getApprovalStatusDisplayData({ + approvalStatus: APPROVAL_STATUSES.ACCEPTED_HERE, + }) ).toEqual({ displayName: 'Ready for approval — Accepted', icon: Ready, @@ -29,7 +36,7 @@ describe('getApprovalStatusDisplayData', () => { it('returns the correct display data for approval status "UNAPPROVED_WAITING"', () => { expect( getApprovalStatusDisplayData({ - approvalStatus: 'UNAPPROVED_WAITING', + approvalStatus: APPROVAL_STATUSES.UNAPPROVED_WAITING, }) ).toEqual({ displayName: 'Waiting for lower level approval', @@ -39,7 +46,9 @@ describe('getApprovalStatusDisplayData', () => { }) it('returns the correct display data for approval status "UNAPPROVED_ABOVE"', () => { expect( - getApprovalStatusDisplayData({ approvalStatus: 'UNAPPROVED_ABOVE' }) + getApprovalStatusDisplayData({ + approvalStatus: APPROVAL_STATUSES.UNAPPROVED_ABOVE, + }) ).toEqual({ displayName: 'Waiting for higher level approval', icon: Waiting, @@ -47,7 +56,10 @@ describe('getApprovalStatusDisplayData', () => { }) }) describe('approved approval statuses "APPROVED_HERE" and "APPROVED_ABOVE"', () => { - for (const approvalStatus of ['APPROVED_HERE', 'APPROVED_ABOVE']) { + for (const approvalStatus of [ + APPROVAL_STATUSES.APPROVED_HERE, + APPROVAL_STATUSES.APPROVED_ABOVE, + ]) { it(`returns the correct diplay data for ${approvalStatus} when only approvalStatus is supplied`, () => { expect( getApprovalStatusDisplayData({ approvalStatus }) @@ -98,7 +110,9 @@ describe('getApprovalStatusDisplayData', () => { }) it('returns the correct display data for approval status "UNAPPROVABLE"', () => { expect( - getApprovalStatusDisplayData({ approvalStatus: 'UNAPPROVABLE' }) + getApprovalStatusDisplayData({ + approvalStatus: APPROVAL_STATUSES.UNAPPROVABLE, + }) ).toEqual({ displayName: 'Cannot be approved', icon: IconBlock16, @@ -107,7 +121,9 @@ describe('getApprovalStatusDisplayData', () => { }) it('returns the correct display data for approval status "ERROR"', () => { expect( - getApprovalStatusDisplayData({ approvalStatus: 'ERROR' }) + getApprovalStatusDisplayData({ + approvalStatus: APPROVAL_STATUSES.ERROR, + }) ).toEqual({ displayName: 'Could not retrieve approval status', icon: IconError16, diff --git a/src/shared/approval-status/index.js b/src/shared/approval-status/index.js index 3b6eaebc..e64837d7 100644 --- a/src/shared/approval-status/index.js +++ b/src/shared/approval-status/index.js @@ -1,3 +1,6 @@ export { ApprovalStatusTag } from './approval-status-tag.js' export { ApprovalStatusIcon } from './approval-status-icon.js' -export { getApprovalStatusDisplayData } from './get-approval-status.js' +export { + getApprovalStatusDisplayData, + APPROVAL_STATUSES, +} from './get-approval-status.js' diff --git a/src/top-bar/org-unit-select/approval-status-icons-legend.js b/src/top-bar/org-unit-select/approval-status-icons-legend.js index 7b30a80a..5275ef79 100644 --- a/src/top-bar/org-unit-select/approval-status-icons-legend.js +++ b/src/top-bar/org-unit-select/approval-status-icons-legend.js @@ -1,17 +1,29 @@ import i18n from '@dhis2/d2-i18n' import React from 'react' -import { ApprovalStatusIcon } from '../../shared/approval-status/index.js' +import { + ApprovalStatusIcon, + APPROVAL_STATUSES, +} from '../../shared/approval-status/index.js' import classes from './approval-status-icons-legend.module.css' // Not all approval statuses are defined here as some share the same icons const approvalStatuses = [ { - status: 'UNAPPROVED_WAITING', + status: APPROVAL_STATUSES.UNAPPROVED_WAITING, displayName: i18n.t('Waiting for approval'), }, - { status: 'UNAPPROVED_READY', displayName: i18n.t('Ready for approval') }, - { status: 'APPROVED_HERE', displayName: i18n.t('Approved') }, - { status: 'UNAPPROVABLE', displayName: i18n.t('Cannot be approved') }, + { + status: APPROVAL_STATUSES.UNAPPROVED_READY, + displayName: i18n.t('Ready for approval'), + }, + { + status: APPROVAL_STATUSES.APPROVED_HERE, + displayName: i18n.t('Approved'), + }, + { + status: APPROVAL_STATUSES.UNAPPROVABLE, + displayName: i18n.t('Cannot be approved'), + }, ] const ApprovalStatusIconsLegend = () => ( diff --git a/src/top-bar/org-unit-select/approval-status-label.js b/src/top-bar/org-unit-select/approval-status-label.js index 4cdc601b..80f6c19c 100644 --- a/src/top-bar/org-unit-select/approval-status-label.js +++ b/src/top-bar/org-unit-select/approval-status-label.js @@ -3,14 +3,14 @@ import { IconWarning16, colors, Tooltip } from '@dhis2/ui' import PropTypes from 'prop-types' import React, { useEffect } from 'react' import { useSelectionContext } from '../../selection-context/index.js' -import { ApprovalStatusIcon } from '../../shared/approval-status/index.js' +import { ApprovalStatusIcon, APPROVAL_STATUSES } from '../../shared/index.js' import classes from './approval-status-label.module.css' import { useApprovalStatus } from './approval-statuses.js' const renderIcon = approvalStatus => { - if (approvalStatus === 'LOADING') { + if (approvalStatus === APPROVAL_STATUSES.LOADING) { return - } else if (approvalStatus === 'FETCH_ERROR') { + } else if (approvalStatus === APPROVAL_STATUSES.ERROR) { return ( {({ onMouseOver, onMouseOut, ref }) => ( diff --git a/src/top-bar/org-unit-select/approval-statuses.js b/src/top-bar/org-unit-select/approval-statuses.js index c6b647d7..0b1f1dda 100644 --- a/src/top-bar/org-unit-select/approval-statuses.js +++ b/src/top-bar/org-unit-select/approval-statuses.js @@ -2,6 +2,7 @@ import { useDataEngine } from '@dhis2/app-runtime' import PropTypes from 'prop-types' import React, { createContext, useContext, useRef, useState } from 'react' import { useDebouncedCallback } from 'use-debounce' +import { APPROVAL_STATUSES } from '../../shared/index.js' const ApprovalStatusesContext = createContext() @@ -61,7 +62,7 @@ const useFetchApprovalStatus = ({ updateApprovalStatuses }) => { workflowId, approvalStatusUpdates: orgUnitIds.reduce( (statuses, orgUnitId) => { - statuses[orgUnitId] = 'LOADING' + statuses[orgUnitId] = APPROVAL_STATUSES.LOADING return statuses }, {} @@ -81,11 +82,11 @@ const useFetchApprovalStatus = ({ updateApprovalStatuses }) => { }, }) approvalStatuses.forEach(({ ou, state }) => { - updateObject[ou] = state || 'UNAPPROVABLE' + updateObject[ou] = state || APPROVAL_STATUSES.UNAPPROVABLE }) } catch (error) { orgUnitIds.forEach(orgUnitId => { - updateObject[orgUnitId] = 'FETCH_ERROR' + updateObject[orgUnitId] = APPROVAL_STATUSES.ERROR }) } updateApprovalStatuses({