Added more processing logic to find institution name, applying credits, and to export HU and BU invoices #12
Conversation
QuanMPhm
force-pushed
the
6/more_processing
branch
2 times, most recently
from
9bc423c
to
f23dd4e
Compare
|
I don't know Python well enough to check your code but the logic is sound. |
QuanMPhm
force-pushed
the
6/more_processing
branch
2 times, most recently
from
22170f8
to
3f135bd
Compare
There was a problem hiding this comment.
Thanks for proposing this PR and the good work.
Some suggestions, besides the comments inline. This PR implements at least 3 new features to this codebase.
- Deriving institution name from a PIs username.
- Exporting two additional versions of the output for only BU, and HU/BU.
- The credits system.
There's not a lot of overlap between them and in situations like this in the future I would encourage you to break them up into several PRs. In this case I would have gone with 3.
This makes the code easier to review. You don't need to break this one up, just a suggestion for the future.
QuanMPhm
force-pushed
the
6/more_processing
branch
3 times, most recently
from
2f53b15
to
916549d
Compare
|
The ordering of |
QuanMPhm
force-pushed
the
6/more_processing
branch
2 times, most recently
from
af098d6
to
e233a3e
Compare
process_report/process_report.py
Outdated
|
|
||
| def get_institution_from_pi(pi_uname): | ||
|
|
||
| dir_path = os.path.dirname(__file__) |
There was a problem hiding this comment.
When loading a file from a program, it is good practice to have the path be relative to the current working directory, not the location of the script.
process_report/process_report.py
Outdated
| ### | ||
|
|
||
|
|
||
| def get_institution_from_pi(pi_uname): |
There was a problem hiding this comment.
This function will read and load this file at every function call. Split the function into two, load_institution_map() -> dict() and get_institution_for_pi(pi_name) -> str.
process_report/process_report.py
Outdated
| filtered_dataframe.to_csv(output_file, index=False) | ||
|
|
||
|
|
||
| def validate_billables(dataframe): | ||
| # Validate PI name |
There was a problem hiding this comment.
This comment would be unnecessary if that was the function name instead. I know that you are writing it in preparation of validating more things, but for now it's the only thing you are validating.
process_report/process_report.py
Outdated
| for i, row in dataframe.iterrows(): | ||
| pi_name = row[PI_FIELD] | ||
| if pandas.isna(pi_name): | ||
| print(f"Project {row[PROJECT_FIELD]} has no PI") # Nan check |
There was a problem hiding this comment.
This comment is unnecessary because the function name describes the same thing due to its name isna.
process_report/process_report.py
Outdated
| with open(f'{dir_path}/institute_map.json', 'r') as f: | ||
| institute_map = json.load(f) | ||
|
|
||
| if '@' in pi_uname: |
There was a problem hiding this comment.
This can be rewritten as the following, due to how split will still work regardless of whether there is @ in the string, and how we always want the last entry in the list, accessible by -1 index.
institution_key = pi_uname.split('@')[-1]
institution = institute_map.get(institution_key, '')
QuanMPhm
force-pushed
the
6/more_processing
branch
2 times, most recently
from
37e6a45
to
49400f9
Compare
|
|
||
|
|
||
| def is_old_pi(old_pi_dict, pi, invoice_month): | ||
| if pi in old_pi_dict and old_pi_dict[pi] != invoice_month: |
There was a problem hiding this comment.
This will not work if you are processing old invoices. Say you want to process invoices for 2024-02 that is 2 months ago. A PI with first invoice of 2024-03 is going to appear as an old PI.
I'm okay with fixing this in a follow-up PR while introducing a mechanism to update the PI file.
process_report/tests/unit_tests.py
Outdated
| self.dataframe = pandas.DataFrame(data) | ||
| old_pi = ['PI2,2023-09', 'PI3,2024-02', 'PI4,2024-03'] # Case with old and new pi in pi file | ||
| old_pi_file = tempfile.NamedTemporaryFile(delete=False, mode='w', suffix='.csv') | ||
| for pi in old_pi: old_pi_file.write(pi + "\n") |
There was a problem hiding this comment.
please write this in 2 lines
There was a problem hiding this comment.
@QuanMPhm after this is addressed you can merge this PR!
… for each PI, and exporting HU and BU invoices
QuanMPhm
force-pushed
the
6/more_processing
branch
from
49400f9
to
c5ff060
Compare
Closes #6 This is a draft PR that implemented most of the functionalities @joachimweyl asked for. These include:
add_institution()to get the institute name for all PIs in the invoiceapply_credits_new_pi()to apply the New PI credit to all projects in the invoiceexport_HU_only()andexport_HU_BU()to export HU and BU specific invoicesA feature worth considering is to export the CSVs to a storage service like Google Drive, will per @knikolla's suggestion, should be put in a separate issue.
Aside from the implementation of the processing functions to apply credits and institution, as well as the export functions, some structural changes are added:
institute_map.json