-
Notifications
You must be signed in to change notification settings - Fork 98
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: add aggregate cost center table #5885
Feat: add aggregate cost center table #5885
Conversation
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 will add other comments during the Pull Request test
@@ -2561,4 +2573,20 @@ CREATE TABLE `configuration_analysis_tools` ( | |||
CONSTRAINT `config_analysis_tools__analysis_tool_type` FOREIGN KEY (`analysis_tool_type_id`) REFERENCES `analysis_tool_type` (`id`) | |||
) ENGINE=InnoDB DEFAULT CHARACTER SET = utf8mb4 DEFAULT COLLATE = utf8mb4_unicode_ci; | |||
|
|||
|
|||
DROP TABLE IF EXISTS `cost_center_aggregate`; |
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.
Will it be possible to rename this table fee_center_aggregate or cost_profit_center_aggregate, Just because this way it will make it look like this table is not about profit centers
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 was implementing the proposal from #5861. I think in English, all off the fee centers are called cost centers. But I could be wrong.
@jmcameron has read more of the documentation than i have. @jmcameron do you have a thought on what the best name is?
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.
Let's wait for his feedback
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.
@jmcameron has read more of the documentation than i have. @jmcameron do you have a thought on what the best name is?
I suggest "Cost Centers" for all services and cost/profit centers. In a sense this is appropriate: All services or centers have costs. Some centers can generate revenue. These are usually also called revenue centers. But they are still cost centers. Regarding the "Adminstration" center at IMCK: This should probably be split up into a Registration Service which could be treated as revenue center. But Administration is normally a cost center NOT a revenue center.
The term "Cost Center" is not explicitly used in the "Essentials of Healthcare Finance" book (we have its chapter 13). But "Cost Center" is used in the "Annex B" handbook. AND "Cost Center" is used in the list of things Larry wants us to do. So he uses this terminology. So I encourage us to use "Cost Center" as the general term for all services/cost/profit/revenue centers.
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.
@@ -1315,6 +1323,8 @@ CREATE TABLE `posting_journal` ( | |||
KEY `period_id` (`period_id`), | |||
KEY `currency_id` (`currency_id`), | |||
KEY `user_id` (`user_id`), | |||
KEY `cost_center_id` (`cost_center_id`), |
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.
Will it be possible to rename this property fee_center_id or cost_profit_center_id
@jniles You forgot to put the changes in the migrate.sql file |
This commit adds the cost_center_id and principal_center_id to the financial tables in the database. Closes IMA-WorldHealth#5880.
57067fa
to
b96c33d
Compare
@lomamech @jmcameron changes landed. |
@jmcameron what do you think of this PR i think we can merge it up |
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 wonder whether this is a good time to rename the 'fee_center" table to 'cost_center'? It will only get more complicated as we add functionality?
) ENGINE=InnoDB DEFAULT CHARACTER SET = utf8mb4 DEFAULT COLLATE = utf8mb4_unicode_ci; | ||
|
||
|
||
ALTER TABLE `posting_journal` ADD COLUMN `cost_center_id` MEDIUMINT(8) UNSIGNED NULL; |
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 suggest you use the add_column_if_missing() function to add these new columns
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.
Whoops, good point.
Adds the admin SQL procedures to create the cost_center_aggregates table. Closes IMA-WorldHealth#5881.
b96c33d
to
af4987c
Compare
@jmcameron changes landed. I tested these on the vanga database. For the renaming of the fee_center, let's get this one in first and then I'll work on renaming the fee center table. I think that may take a lot longer than this. |
Sounds good. |
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.
Looks good.
One question (not a blocker): Can 'IGNORE' be used with the ADD CONSTRAINT lines in the migrate.sql file (so it can be loaded more than once). I see that we have an SQL function Constraint_exists() so it would not be hard to create a function like add_column_if_missing() function for constraints.
There is no This would therefore need to be a custom stored procedure. I'll make an issue. |
This PR adds the back-end tables for aggregating costs by cost center and the stored procedure for generating it. It should serve as a reference for how to create the table for step-down analysis.
Closes #5880.
Closes #5881.