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

Allow exporting a snapshot of NERC Prepay Credits file #128

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow exporting a snapshot of NERC Prepay Credits file #128

wants to merge 1 commit into from
+112 −0

Conversation

QuanMPhm
Copy link
Contributor

Closes #86. This PR consists of only the last commit, "NERC Prepay Credits...".

Two functions have been added to the Prepayment Processor. One is to obtain the credits "snapshot". A test case has been written for this function.

@QuanMPhm QuanMPhm changed the title 86/prepay credits Allow exporting a snapshot of NERC Prepay Credits file Jan 2, 2025
knikolla
knikolla reviewed Jan 10, 2025
View reviewed changes
process_report/processors/prepayment_processor.py Outdated
return self.prepay_credits[credits_mask]

def _export_prepay_credits_snapshot(self, credits_snapshot):
credits_snapshot.to_csv(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also upload to s3.

Copy link
Contributor Author

@QuanMPhm QuanMPhm Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The Credits snapshot will now be uploaded to f"Invoices/{self.invoice_month}/NERC_Prepaid_Group-Credits-{self.invoice_month}.csv" and the archive location f"Invoices/{self.invoice_month}/Archive/NERC_Prepaid_Group-Credits-{self.invoice_month} {util.get_iso8601_time()}.csv"

@QuanMPhm
Copy link
Contributor Author

I have rebased the PR

@QuanMPhm QuanMPhm requested a review from knikolla January 14, 2025 21:12
@QuanMPhm QuanMPhm force-pushed the 86/prepay_credits branch 3 times, most recently from dace52c to dc66206 Compare January 15, 2025 14:47
@QuanMPhm QuanMPhm mentioned this pull request Feb 7, 2025
Open
@joachimweyl
Copy link
Contributor

@knikolla did Quan manage to resolve the issue you had with this PR? are we ready to approve?

@QuanMPhm QuanMPhm requested a review from naved001 February 25, 2025 16:41
@QuanMPhm
Copy link
Contributor Author

The PR has been rebased

@joachimweyl
Copy link
Contributor

@QuanMPhm please re-request review after you rebase.

@QuanMPhm
Copy link
Contributor Author

@joachimweyl I don't see a button to allow a re-review yet :P I guess, @knikolla @naved001 @hakasapl Any opinions?

knikolla
knikolla reviewed Feb 26, 2025
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.

Stylistic and consistency comments. Otherwise looks good.

process_report/processors/prepayment_processor.py Outdated
prepay_credits: pandas.DataFrame
prepay_projects: pandas.DataFrame
prepay_contacts: pandas.DataFrame
prepay_debits_filepath: str
upload_to_s3: bool
export_NERC_credits: bool = True # For testing purposes
Copy link
Contributor

Choose a reason for hiding this comment

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

export_NERC_credits is inconsistently capitalized, should be export_nerc_credits.

process_report/processors/prepayment_processor.py Outdated
@@ -22,11 +22,24 @@ class PrepaymentProcessor(discount_processor.DiscountProcessor):
def PREPAY_DEBITS_S3_BACKUP_FILEPATH(self):
return f"Prepay/Archive/prepay_debits {util.get_iso8601_time()}.csv"

@property
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually try to have the following order for things defined in a class.

  1. attributes
  2. properties
  3. methods

This makes it easier to find out where things are defined, so please move the properties after the attributes.

process_report/processors/prepayment_processor.py Outdated
return f"NERC_Prepaid_Group-Credits-{self.invoice_month}.csv"

@property
def CREDITS_SNAPSHOT_S3_FILEPATH(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

While you could argue that these are all constants and therefore should be all caps, please don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have repositioned and removed the capitalization on the properties.

@QuanMPhm QuanMPhm force-pushed the 86/prepay_credits branch 2 times, most recently from 317850c to e9382b5 Compare February 26, 2025 19:13
knikolla
knikolla requested changes Feb 26, 2025
View reviewed changes
process_report/processors/prepayment_processor.py Outdated
def _export_s3_prepay_credits_snapshot(self, credits_snapshot):
invoice_bucket = util.get_invoice_bucket()
invoice_bucket.upload_file(
self.CREDITS_SNAPSHOT_FILEPATH, self.CREDITS_SNAPSHOT_S3_FILEPATH
Copy link
Contributor

Choose a reason for hiding this comment

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

you changed the capitalization, but didn't change the capitalization of where they are used.

knikolla
knikolla reviewed Feb 27, 2025
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.

I don't like the idea of Processors exporting files into the Invoices\<invoice_month> folder, as that should be limited to only Invoices.

Is this something that can be done in an Invoice?

@QuanMPhm
Copy link
Contributor Author

@knikolla Initially, I didn't want to create an invoice class for the NERC credits snapshot, since it wasn't technically an invoice. But now that you point it out, I'll make an Invoice class for it

@knikolla
Copy link
Contributor

@knikolla Initially, I didn't want to create an invoice class for the NERC credits snapshot, since it wasn't technically an invoice. But now that you point it out, I'll make an Invoice class for it

@QuanMPhm Or alternatively, if it's not an Invoice, you could put it in a different folder. What is our purpose for exporting this file? What happens with it after export?

@QuanMPhm
Copy link
Contributor Author

From the prepay scaffolding document:

NERC_Prepaid_Group-Credits-YYYY-MM.csv:
Duplicate of Prepaid Group Credit
This will provide the Prepaid credit information so that MGHPCC can add the cost to the bill
MGHPCC does not differentiate on the PDF what is for what so it will show up under the NERC header, not important for codding just good for us to know
Will be used to generate PDFs for Prepaid Group

@joachimweyl Let me know if this is still correct.

@knikolla
Copy link
Contributor

This will provide the Prepaid credit information so that MGHPCC can add the cost to the bill

@QuanMPhm Sounds like it's an Invoice :)

  • It's not just a backup for archival purposes (similar to old PI files, etc)
  • We give it out to the MGHPCC for their use

@QuanMPhm
Copy link
Contributor Author

@knikolla Could I have the prepay processor create the snapshot, and have the invoice class export it? That would make the code a bit less repetitive, but I understand if you think it might make the code look messier.

@QuanMPhm
Copy link
Contributor Author

I've decided to put everything into a new invoice class.

@QuanMPhm QuanMPhm requested a review from knikolla February 27, 2025 21:05
@QuanMPhm
NERC Prepay Credits file is now exported through `PrepayCreditsSnapsh…
a7a7f68 
…ot` invoice class

The snapshot has custom export paths, and a test case
has been written for it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knikolla knikolla knikolla approved these changes

@hakasapl hakasapl Awaiting requested review from hakasapl

@naved001 naved001 Awaiting requested review from naved001

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

Successfully merging this pull request may close these issues.

Generate NERC Prepaid Group Credits CSV
3 participants
@QuanMPhm @joachimweyl @knikolla