-
Notifications
You must be signed in to change notification settings - Fork 52
(fix) O3-5200 Add validation so that once a bill has been "posted" its line items cannot be modified #516
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
@ELVIS-KATO The point of this ticket was that this needs to be fixed on both the backend and frontend. We should fix the backend first because the frontend is also going to need to respond to the errors produced by the backend. Also, you asked a good clarifying question on the ticket, but it's unaddressed here? If the ticket needs clarifications, we need to sort that out before committing code because it probably points to things that aren't well-specified. |
|
Ok @ibacher, thanks for that. So, it means this will be valid after the backend has been refactored. |
|
I have made a second commit that is working on what @NethmiRodrigo had talked about |
NethmiRodrigo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ELVIS-KATO
| {canEditBill ? ( | ||
| <Button | ||
| data-testid={`edit-button-${item.uuid}`} | ||
| renderIcon={Edit} | ||
| hasIconOnly | ||
| kind="ghost" | ||
| iconDescription={t('editThisBillItem', 'Edit this bill item')} | ||
| tooltipPosition="left" | ||
| onClick={() => handleSelectBillItem(item)} | ||
| /> | ||
| ) : ( | ||
| <Button | ||
| data-tested={`edit-button-${item.uuid}`} | ||
| renderIcon={Edit} | ||
| hasIconOnly | ||
| kind="ghost" | ||
| disabled | ||
| iconDescription={t('cannotEditThisBill', 'You can not edit this bill')} | ||
| tooltipPosition="left" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don’t need to conditionally render a ghost button. Just disabling the button is sufficient, like so:
| {canEditBill ? ( | |
| <Button | |
| data-testid={`edit-button-${item.uuid}`} | |
| renderIcon={Edit} | |
| hasIconOnly | |
| kind="ghost" | |
| iconDescription={t('editThisBillItem', 'Edit this bill item')} | |
| tooltipPosition="left" | |
| onClick={() => handleSelectBillItem(item)} | |
| /> | |
| ) : ( | |
| <Button | |
| data-tested={`edit-button-${item.uuid}`} | |
| renderIcon={Edit} | |
| hasIconOnly | |
| kind="ghost" | |
| disabled | |
| iconDescription={t('cannotEditThisBill', 'You can not edit this bill')} | |
| tooltipPosition="left" | |
| /> | |
| )} | |
| <Button | |
| data-testid={`edit-button-${item.uuid}`} | |
| renderIcon={Edit} | |
| hasIconOnly | |
| kind=“ghost” | |
| iconDescription={t('editThisBillItem', 'Edit this bill item’)} | |
| tooltipPosition=“left” | |
| onClick={() => handleSelectBillItem(item)} | |
| disabled={bill?.status !== ‘PENDING’} | |
| /> |
src/billing.resource.ts
Outdated
| const isSpecialStatus = bill.status === 'POSTED' || bill.status === 'PAID' || bill.status === 'ADJUSTED'; | ||
|
|
||
| const status = | ||
| activeLineItems.length > 0 | ||
| const status = isSpecialStatus | ||
| ? bill.status | ||
| : activeLineItems.length > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t understand why we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we need it coz it ensures that if the backend has already marked a bill as a final state (POSTED, PAID, or ADJUSTED), the UI keeps that status instead of recalculating one from the line items. Coz to my observation previously, the UI always derived the status from line-item states, which overwrote important server-driven statuses and incorrectly made finalized bills appear editable. The isSpecialStatus check prevents that by preserving the backend’s status so the UI can correctly block edits unless the bill is truly PENDING. But maybe I miss understood the code functionality or it could be catered for in the backend.
| if (bill?.status === 'PENDING') { | ||
| showSnackbar({ | ||
| title: t('cannotEditThisBill', 'You can not edit this bill'), | ||
| subtitle: t('onlyPendingBillsCanBeEdited', 'Only pending bills can be edited'), | ||
| kind: 'error', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think this is the proper place to trigger a snackbar. I don’t think we need to add a safeguard in this modal at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let me have it removed
Requirements
Summary
Modified the code so that bills can only be modified only if the status is pending. The code checks for the status before allowing someone to edit a bill.
The Edit button has been made active only when the status is pending.
I have tried to Preserve POSTED/ADJUSTED from backend so UI can block it.
Screenshots
the NEW VIDEO
Pending2.mp4
Related Issue
https://openmrs.atlassian.net/issues?filter=-4&selectedIssue=O3-5200
Other
@ibacher @denniskigen