Skip to content

fix: handle UNAPPROVED_ABOVE status #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mediremi merged 2 commits into from
Jul 7, 2021
Merged

fix: handle UNAPPROVED_ABOVE status #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
114e1f5
fix: handle UNAPPROVED_ABOVE status
mediremi Jul 6, 2021
ac2f2c7
fix: throw on unknown approval state
mediremi Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/shared/status-tag/status-tag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ StatusTag.propTypes = {
'UNAPPROVED_READY',
'UNAPPROVED_WAITING',
'UNAPPROVED_ELSEWHERE',
'UNAPPROVED_ABOVE',
'UNAPPROVABLE',
]),
}
Expand Down
4 changes: 4 additions & 0 deletions src/shared/status-tag/use-approval-state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const useApprovalState = approvalState => {

case 'UNAPPROVED_WAITING':
case 'UNAPPROVED_ELSEWHERE':
case 'UNAPPROVED_ABOVE':

@Mohammer5 Mohammer5 Jul 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I get these docs (search for "UNAPPROVED_ABOVE") right, the this means that it's already been approved and the next level has to approve it, but I'm really not sure if I'm interpreting the docs correctly

@Mohammer5 Mohammer5 Jul 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is: Is "waiting" the right label?

@HendrikThePendric HendrikThePendric Jul 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I’m not sure about the correct interpretation either, but we need to get these confirmed anyway, so I guess we can add this state to this case (waiting) for now.

Now that we already encountered this undocumented state on day 1 already, I think we should:

  • Definitely talk to Lars/Jim about the correct interpretation of these states
  • Probably scan the dhis-core source code for a complete overview of the statuses

Probably it makes sense to ensure we have a complete list before we talk to one of them…

@HendrikThePendric HendrikThePendric Jul 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: I am referring to this state as undocumented because I don’t see it in the list of approval states. Could be I’m not looking properly or in the wrong place…

@HendrikThePendric HendrikThePendric Jul 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look in the dhis2-core code-base and was expecting the relevant section of code to be in the DataApprovalState.java file. But that only contained the 7 states and we already had comprised a list of 8 from the docs:

UNAPPROVABLE
APPROVED_ELSEWHERE // in docs, but not in DataApprovalState.java
UNAPPROVED_ABOVE
UNAPPROVED_WAITING
UNAPPROVED_READY
APPROVED_ABOVE
APPROVED_HERE
ACCEPTED_HERE

Then looked in dataApproval.js (I think the is the Javascript part of the old struts app) and there I found 14 states:

ACCEPTED_ELSEWHERE
ACCEPTED_HERE
APPROVED_ABOVE
APPROVED_ELSEWHERE
APPROVED_HERE
PARTIALLY_ACCEPTED_ELSEWHERE
PARTIALLY_ACCEPTED_HERE
PARTIALLY_APPROVED_ELSEWHERE
PARTIALLY_APPROVED_HERE
UNAPPROVABLE
UNAPPROVED_ABOVE
UNAPPROVED_ELSEWHERE
UNAPPROVED_READY
UNAPPROVED_WAITING

What was puzzling to is how the JavaScript file could have to deal with more approval states than defined in the Java code. I think we need to get more info on this.

For now, I guess we could just add the case UNAPPROVED_ABOVE as you suggested, but perhaps it would be good to also add a default case which throws a descriptive error message?

return {
icon: Waiting,
displayName: i18n.t('Waiting'),
Expand All @@ -49,6 +50,9 @@ const useApprovalState = approvalState => {
displayName: i18n.t('Cannot approve'),
type: 'negative',
}

default:
throw new Error(`Unknown approval state: '${approvalState}'`)
}
}

Expand Down
Loading