Implemented processor for New-PI credit #112
Conversation
QuanMPhm
force-pushed
the
101/proc_new_pi_credit
branch
2 times, most recently
from
0299851 to
59c0012
Compare
|
I fixed a bug with @knikolla @naved001 I could write the test cases that would catch this mistake in this PR, but since the PR for #102 would catch this, I believe I will just write these tests in that PR? |
Could you say more about the impact of this bug? Would we need to rerun the invoice with the bug fix?
Which commit has the bug fix? the commit message in the 3rd commit covers a bunch of things and I didn't find a note about the bug fix. Also, if the fix for a bug is in this PR I think the test should also be a part of this. |
| """ | ||
| Dependancies: | ||
| - ValidateBillablePIsProcessor | ||
| - NewPICreditProcessor | ||
| """ |
There was a problem hiding this comment.
What are these dependencies? Are we documenting them here (and elsewhere in this file) because they are otherwise non-obvious?
There was a problem hiding this comment.
Many of the invoice subclasses expect certain columns to exist in the dataframe they are given, and some of those columns are initialized and populated by certain processors. ValidateBillablePIsProcessor populates the IS_BILLABLE_FIELD and MISSING_PI_FIELD, while NewPICreditProcessor populates many columns, such as BALANCE_FIELD. These columns are used by many invoice classes, including NERCTotalInvoice, during their filtering or exporting steps. As such, I believed that there are dependencies between invoices and processors, since many invoices need certain processors to run before them to work properly.
I thought that I should highlight these dependencies with these docstrings. I will follow your advice if you believe something better should be done.
There was a problem hiding this comment.
I think it's fine to document this in the class docstring, but I think you should be a little bit more verbose. E.g., This class requires the following columns to be defined in the input dataframe:. This makes it clear that you're not referring to Python modules or classes or something else that could be considered a "dependency". If the required fields are only provided by a specific processor, it's probably better to say something like This class operates on data that has been processed by NewPICreditProcess.
Also note that the spelling is "dependencies".
| from process_report.processors import discount_processor | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
Why is this a dataclass? It doesn't appear to have any class attributes. I guess the same question applies to discount_process.DiscountProcessor as well.
There was a problem hiding this comment.
NewPICreditProcessor does have old_pi_filepath and limit_new_pi_credit_to_partners as its class attributes, but as for DiscountProcessor, you are right, it has none. I guess I made them dataclasses for uniformity with all the other processors and invoices and didn't put too much thought into it.
I will remove @dataclass from DiscountProcessor
QuanMPhm
force-pushed
the
101/proc_new_pi_credit
branch
from
59c0012 to
2073310
Compare
Code from `BillableInvoice` and `DiscountInvoice` has been copied over without change
The `DiscountProcessor` class has been created, to be subclassed by processors which applies discounts, such as the New-PI Credit. This processor class introduces an important class constant, `IS_DISCOUNT_BY_NERC`, which detemines whether the final balance, as opposed to the PI balance (more info below), reflects the discount being applied. The New-PI credit is now implemented by `NewPICreditProcessor` During discussions about billing, it was made noted that some discounts are not provided by the MGHPCC, but instead from other sources, such as the BU subsidy, which is provided to BU PIs by BU. This provided motivation for a `PI Balance` field, which would reflect how much money the PI should be billed, as opposed to the `Balance` field, which currently reflects how much money the MGHPCC should receive. These two fields would not equal each other if the PI received discounts not provided by the MGHPCC. Implementation of `NewPICreditProcessor` and the new billing feature required a range of changes: - `apply_discount_on_project()` in `DiscountProcessor` has been slightly modified, where the PI balance and MGHPCC balance is now calculated seperately. - As `BillableInvoice` no longer performs any processing itself, the dataframe from `NewPICreditProcessor` is now passed to all invoice objects. - The test cases for the New-PI credit have been refactored. Test cases for the new billing feature is not written yet. I plan to write them when the processor for the BU Subsidy is implemented - With the new processor, certain Processor and Invoice classes depend on fields created by other Processors, such as the case with `NewPICreditProcessor` and `ValidateBillablePIsProcessor`. As such, docstrings have been added to indicate dependancies
QuanMPhm
force-pushed
the
101/proc_new_pi_credit
branch
from
2073310 to
1686888
Compare
|
@knikolla do you think you'd have time to review this? |
Closes #101. Depends on #108. I've implemented processor for applying the New-PI Credit, and included a new field to distinguish between the PI balance and the NERC balance. More details are in my commit message.
There are two commits to this PR, starting with
Initialized processor for applying the New-PI Credit and discounts. The first one only copies over code, while the second commit actually makes functional changes.