Refactored BU Internal Invoice

[QuanMPhm]

Closes #67. A new bu_internal_invoice.py file has been added. Note that the BU Internal invoice depends on the billable invoice. The test case for the BU Internal invoice has been modified appropriately.

[QuanMPhm]

@knikolla This may be too much but...since I'll be doing more nuanced refactoring with the BU Internal invoice, should that be a separate commit as well? Some of the "nuanced refactoring" has already been added to the first commit, so I wonder if its still worth the effort to split to commit and reorganize things.

[knikolla]

@knikolla This may be too much but...since I'll be doing more nuanced refactoring with the BU Internal invoice, should that be a separate commit as well? Some of the "nuanced refactoring" has already been added to the first commit, so I wonder if its still worth the effort to split to commit and reorganize things.

Think of it this way. Your goal is to make reviewing a PR as easy as possible. Splitting things into different commits often enables that by grouping a set of changes together with a clear commit message and description. This allows one commit to do one thing and do it well and clearly.

To understand what making a PR easier to review means, you should go through the process of reviewing your own PRs (and obviously the code of others). Some questions to think about.

• Is it clear what the PR is trying to accomplish (and each commit)? (clear scope?)
• Is it hard to know where to even start reviewing this PR/commit? How could you make it easier for a reviewer to know where to start? (too big?)
• Once you start reviewing, how do you progress? Do you go top to bottom? Do you find yourself scrolling up and down? Which parts do you find yourself coming back to the most?
• Are there particular parts that are really hard for you to understand? How could you make those parts easier to review and understand?

Sometimes just splitting things into several commits significantly helps. Sometimes it doesn't. I think in this case it will.

But ultimately, there is no single best answer on how to organize changes and you shouldn't seek to find an answer that fits every solution. Perfect is the enemy of done, and engineering software is a game of trade offs. You're trying to optimize both your time, the time of reviewers, your cognitive workload, and the cognitive workload of reviewers.

[QuanMPhm]

@knikolla I have re-organized the commits. Now, the first commit will only show code copying and the initial implementation of the BU-Internal invoice. The second one contains more detailed refactoring decisions.

[naved001]

will continue to review this. Thanks for all the docstrings!

[naved001]

will this modify the billable_inv dataframe? The reason I ask this is because we could consolidate all the s3 exports later. something like:

if args.upload_to_s3:
    bucket = get_invoice_bucket()
    billable_inv.export_s3(bucket)
    bu_internal_inv.export_s3(bucket)

[naved001]

also, it's just good to understand if this does modify it because then it would mean that after this runs "billable_inv" is no longer what it says (it may have turned into bu_internal_inv)

[QuanMPhm]

If I don't explicitly call copy() on a dataframe, then you can assume that the same dataframe is being assigned by reference:

a = pd.DataFrame({'a': [1,2,3,4],
                  'b': [1,2,3,4]})
b = a
b.loc[0, 'a'] = 100
print(a)
print(b)>>> 
>>> a = pd.DataFrame({'a': [1,2,3,4],
...                   'b': [1,2,3,4]})
>>> b = a
>>> b.loc[0, 'a'] = 100
>>> print(a)
     a  b
0  100  1
1    2  2
2    3  3
3    4  4
>>> print(b)
     a  b
0  100  1
1    2  2
2    3  3
3    4  4 

I will make sure each invoice's dataframes won't reference each other when exported together like in your suggestion.

[QuanMPhm]

@naved001 I'll consolidate all the export() and export_s3() calls together after your detailed review.

[naved001]

@QuanMPhm thanks, this largely looks fine to me.

[QuanMPhm]

While consolidating the export statements, I thought that it would make the code more organized if I process and export the invoices in the same code block. So instead of consolidating all the export() calls together, I defined process_and_export_invoices to process and export a list of invoices.

For now, process_and_export_invoices is called twice. First to export the nonbillable, billable, and lenovo invoice. Second to export the HU-BU and BU-Internal, since these two invoices depend on processing that's done in the billable invoice.

@naved001 What do you think about this?

[knikolla]

Looks almost there!

[knikolla]

@QuanMPhm can you fix the merge conflicts?

@naved001 would appreciate if you can you try to give this a review this week.

[QuanMPhm]

@naved001 I have rebased the PR.