-
Notifications
You must be signed in to change notification settings - Fork 47
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
Feat: Added bill manager #277
Feat: Added bill manager #277
Conversation
Thanks, @amosmachora. This is a real change. Tagging @makombe and @donaldkibet for their reviews. |
I am not the best person to be able to answer the specifics of what may or may not be edited but whatever you guys decide just inform me I'll be on it as soon as. What I created was just of the top of my head. |
package.json
Outdated
@@ -31,7 +31,7 @@ | |||
"zod": "^3.21.4" | |||
}, | |||
"devDependencies": { | |||
"@openmrs/esm-framework": "^5.6.1-pre.1895", | |||
"@openmrs/esm-framework": "^5.6.1-pre.2065", |
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.
"@openmrs/esm-framework": "^5.6.1-pre.2065", | |
"@openmrs/esm-framework": "next", |
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.
Hi @donaldkibet this change seems to have broken the E2E tests. Please advice.
packages/esm-billing-app/src/billable-services/bill-manager/bill-line-items.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-billing-app/src/billable-services/bill-manager/modals/cancel-bill.modal.tsx
Outdated
Show resolved
Hide resolved
packages/esm-billing-app/src/billable-services/bill-manager/modals/cancel-bill.scss
Outdated
Show resolved
Hide resolved
packages/esm-billing-app/src/billable-services/bill-manager/patient-bills.component.tsx
Outdated
Show resolved
Hide resolved
<SideNavLink onClick={() => handleNavigation('')} renderIcon={Wallet} isActive={pathname === '/'}> | ||
{t('billableServices', 'Billable Services')} | ||
</SideNavLink> | ||
<UserHasAccess privilege="coreapps.systemAdministration"> |
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.
This is hard cording this such that only system admin will see this. We need to define roles and privileges and assign them accordingly cc @ojwanganto @makombe thoughts?
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.
@donaldkibet @makombe - you know you are more experienced here. Please guide him so that this is closed soonest.
Thanks :)
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 should use the privileges on extension to configure privileges for this component. We can merge the PR and add a ticket to address access for it
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, @donaldkibet. Could you kindly provide a github link of where a similar piece has been implemented?
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.
In that code I just assumed the person who wrote it knew what he was doing so I just left it there : )
Seems the build is also failing @amosmachora |
Thanks for your feedback @donaldkibet will work on this. Im understanding this project a bit more each day : ) . Well received though. I had a question though about translations. From the community I have come to gather (I might be very wrong about this) translations are a feature to allow for multi lingual support, but in our code we tend to translate from English to English which I don`t understand. Mind helping me undertand. |
@amosmachora, we're not translating from English to English. The keys should be in English by default, for example: const { t } = useTranslation()
<h2>{t('billManager', 'Bill manager')}</h2> Transifex will handle translation into different languages. English is sufficient for our needs, but we're translating hard-coded values to comply with community standards and conventions in OpenMRS 3.x. |
Co-authored-by: Donald Kibet <[email protected]>
@donaldkibet okay. I see it now. Thanks for clarifying. |
Requirements
Summary
In this PR I have replaced the bill waiver page with a more generic bill manager which allows for waive, edit, cancel, and delete operations on individual line items and also on the bill. I have also added a lot of UI improvements to correctly show loading screeens and data to provide more informative feedback to the user.
I have unfortunately not added tests yet (Majorly because i don`t know how to write tests in jest lol) but i will learn and provide more comprehensive PRs as we continue.
Also currently the only functionality that is calling the backend APIS is ** waive**. I will update the remaining three as soon as the backend apis are ready.
I have also fixed a slight UI issue that the sidenav did not show the active status correctly.
All feedback will be highly appreciated.
Screenshots
Bill.Manager.PR.Video.-.Made.with.Clipchamp.mp4
Related Issue
None.
Other
None.