fix(bottom-bar): disable approve button when it is allowed but pointl…

…ess (#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

@@ -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  |           |
+            | label     | visible | disabled |
+            | Approve   | yes     |          |
+            | Accept    |         |          |
+            | Unapprove |         |          |
+            | Unaccept  |         |          |

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')

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 (
         <>

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,
 }
 

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,
 }

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,28 +70,29 @@ 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}'`)
     }
 }
 
 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 }

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,15 +46,20 @@ 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,
             type: 'default',
         })
     })
     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,

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'

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 = () => (