Refactored billable invoice #61
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I would like to see the second commit updated so that the commit message has some description of the changes involved in "Refactored billable invoice". |
QuanMPhm
force-pushed
the
52.2/refactor_billable
branch
2 times, most recently
from
ef15ce6 to
342fb30
Compare
naved001
reviewed
Jun 18, 2024
Closed
22 tasks
QuanMPhm
force-pushed
the
52.2/refactor_billable
branch
from
342fb30 to
845697b
Compare
QuanMPhm
force-pushed
the
52.2/refactor_billable
branch
from
845697b to
e4e11a1
Compare
knikolla
requested changes
Jul 16, 2024
The billable invoice now subclasses the Invoice base class. The computations involved in preparation, namely filtering out nonbillable projects and PIs, and validating PI names, have been moved to `util.py`. The function for applying the New-PI credit (`apply_credits_new_pi`) is also moved there, and the I/O needed to read and write out the PI file has been moved out of this functions for ease of testing.
QuanMPhm
force-pushed
the
52.2/refactor_billable
branch
2 times, most recently
from
71f8443 to
dce97da
Compare
naved001
reviewed
Jul 24, 2024
QuanMPhm
force-pushed
the
52.2/refactor_billable
branch
from
dce97da to
a2f6fef
Compare
naved001
reviewed
Jul 30, 2024
knikolla
requested changes
Jul 31, 2024
QuanMPhm
force-pushed
the
52.2/refactor_billable
branch
from
a2f6fef to
0a08fea
Compare
naved001
approved these changes
Aug 12, 2024
knikolla
approved these changes
Aug 15, 2024
larsks
approved these changes
Aug 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #64, I have refactored the billable invoice. I'll explain in more detail below, and have also have a question that I would like the professionals' opinions on.
For some context, the billable invoice performs 2 preparation steps (removing nonbillables and validating PI names) and 3 distinct steps during processing (loading the old PI file, applying the new PI credit, and then writing back the old PI file). I decided to split these steps into their own functions, and to store those functions in
util.py, instead of making them private functions in theBillableInvoiceclass.As for the test cases, I created a new test class for the billable invoice. Note that this new test case uses randomly generated values, instead of a hardcoded dataframe. I figured this would be more robust?
While writing the test case, l also had a few design questions I wanted to ask...
Considering how unsightly some of the test cases are, especially for the New PI credit, I was trying to think of ways to make the test case more organized and easier to follow. One of them was to create small test functions that would only test for one specific scenario (i.e a case for new PI, 1-month PI, overwritten entry, etc...), and to make a generator function that would that randomly generate the invoice and old-pi file (along with the expected output after they are processed) for a PI. I'm follow your opinions on whether randomized test data, instead of hardcoded data, would be better.