Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implemented processors for removing nonbillables and validating billable PIs #108

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Implemented processors for removing nonbillables and validating billable PIs #108

merged 1 commit into from
Nov 19, 2024

Conversation

QuanMPhm
Copy link
Contributor

Closes #104. Dependant on #100 and #99. The processing steps of removing nonbillables and validating billables in the Billable Invoice has been moved into Processor subclasses.

@knikolla @naved001 Because future processing steps (applying discounts, etc) assume that the invoice has gone through validation processing, this PR will be blocking other refactoring issues, namely #101 and #102.

@QuanMPhm QuanMPhm force-pushed the 104/proc_remove_nonbill branch from bac60f7 to 0a88aba Compare October 22, 2024 13:26
@QuanMPhm QuanMPhm force-pushed the 104/proc_remove_nonbill branch from 0a88aba to fd68927 Compare October 23, 2024 17:55
@QuanMPhm
Copy link
Contributor Author

@knikolla I've implemented your feedback, and now all Processor steps are before the Invoices. @knikolla @naved001 Let me know what you guys think.

As a note to myself: I've stashed the earlier version of the PR in branch 104_draft

@QuanMPhm QuanMPhm requested a review from knikolla October 24, 2024 14:44
knikolla
knikolla requested changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@knikolla knikolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you're not removing those rows from the dataframe where credits are applying, we do need to ignore non billable projects when applying credits, otherwise a PIs credits may be used from a project that is not billable.

process_report/processors/remove_nonbillables_processor.py Outdated


@dataclass
class RemoveNonbillablesProcessor(processor.Processor):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is now incorrect. Since there's not a lot happening here, I would suggest moving the logic of this processor inside of the validate billable pi processor.

@knikolla
Copy link
Contributor

Now that you're not removing those rows from the dataframe where credits are applying, we do need to ignore non billable projects when applying credits, otherwise a PIs credits may be used from a project that is not billable.

Apologies. I see that you're doing this in the Billable invoice, since that hasn't be refactored yet. Disregard this comment.

@QuanMPhm QuanMPhm force-pushed the 104/proc_remove_nonbill branch from fd68927 to 85ad7f7 Compare November 6, 2024 18:39
@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Nov 7, 2024

@knikolla @naved001 I have rebased the PR and addressed all current feedback

@QuanMPhm QuanMPhm mentioned this pull request Sep 25, 2024
Closed
@QuanMPhm QuanMPhm requested a review from knikolla November 7, 2024 19:49
@QuanMPhm QuanMPhm force-pushed the 104/proc_remove_nonbill branch from 85ad7f7 to c9db8d0 Compare November 7, 2024 20:38
@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Nov 7, 2024

Added IS_BILLABLE_FIELD and MISSING_PI_FIELD as constants in invoice.py

@QuanMPhm QuanMPhm mentioned this pull request Nov 7, 2024
Merged
@QuanMPhm QuanMPhm force-pushed the 104/proc_remove_nonbill branch 2 times, most recently from e3455e9 to 4621b09 Compare November 8, 2024 19:21
@QuanMPhm
Implemented processor for removing nonbillables and validating billab…
32ec283 
…le PIs

Two new fields, which are used internally by the scripts and never exported,
have been added:
- IS_BILLABLE_FIELD: Boolean field for whether the project is billable
- MISSING_PI_FIELD: Boolean field that is True if PI name field is empty
@QuanMPhm QuanMPhm force-pushed the 104/proc_remove_nonbill branch from 4621b09 to 32ec283 Compare November 12, 2024 19:37
@QuanMPhm QuanMPhm mentioned this pull request Nov 13, 2024
Merged
knikolla
knikolla reviewed Nov 15, 2024
View reviewed changes
Copy link
Contributor

@knikolla knikolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since nonbillables are not being removed from the dataframe now, are the processing steps where we are applying credits ignoring those nonbillable rows?

@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Nov 15, 2024 •
edited
Loading

@knikolla For now, the invoices that only care about billable projects (Billable, BU-Internal, etc) filter out billable projects like this. This means nonbillable rows will still be ignored correctly.

In #111, I'll have the invoices assign the filtered dataframe to a different variable

knikolla
knikolla approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@knikolla knikolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. I see it now. Looks good to me!

Yes, at the end of this set of refactorings we want to

  1. Processors to operate on the same DataFrame (without doing .copy)
  2. Invoices to filter out and shape the output how they want, without reassigning self.data.

@QuanMPhm QuanMPhm merged commit 2bfabc5 into CCI-MOC:main Nov 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@naved001 naved001 naved001 approved these changes

@knikolla knikolla knikolla approved these changes

@hakasapl hakasapl Awaiting requested review from hakasapl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor processing for removing nonbillables and PI name validation
3 participants
@QuanMPhm @knikolla @naved001