Skip to content

Refactor PIINVOICE so env vars are fetched at start of billing pipeline. #179

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

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

Refactor PIINVOICE so env vars are fetched at start of billing pipeline. #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions process_report/invoices/pi_specific_invoice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import os
import sys
from dataclasses import dataclass
import subprocess
import tempfile
Expand All @@ -13,7 +12,6 @@


TEMPLATE_DIR_PATH = "process_report/templates"
CHROME_BIN_PATH = os.environ.get("CHROME_BIN_PATH", "/usr/bin/chromium")


logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -121,17 +119,12 @@ def _create_html_invoice(temp_fd):
temp_fd.flush()

def _create_pdf_invoice(temp_fd_name):
if not os.path.exists(CHROME_BIN_PATH):
sys.exit(
f"Chrome binary does not exist at {CHROME_BIN_PATH}. Make sure the env var CHROME_BIN_PATH is set correctly and that Google Chrome is installed"
)

invoice_pdf_path = (
f"{self.name}/{pi_instituition}_{pi}_{self.invoice_month}.pdf"
)
subprocess.run(
[
CHROME_BIN_PATH,
os.environ.get("CHROME_BIN_PATH", "/usr/bin/chromium"),
Copy link
Member

@larsks larsks Aug 25, 2025

Choose a reason for hiding this comment

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

What was the motivation for this change? I think it's generally better practice to read your environment variables early, rather than doing it inline like this.

"--headless",
"--no-sandbox",
f"--print-to-pdf={invoice_pdf_path}",
Expand Down
2 changes: 1 addition & 1 deletion process_report/process_report.py
View file
Open in desktop
Copy link
Contributor

@QuanMPhm QuanMPhm Jun 3, 2025

Choose a reason for hiding this comment

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

You should add CHROME_BIN_PATH in REQUIRED_ENV_VARS

Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ def main():
)
args = parser.parse_args()

required_env_vars = []
required_env_vars = ["CHROME_BIN_PATH"]
if not args.coldfront_data_file:
required_env_vars.extend(["KEYCLOAK_CLIENT_ID", "KEYCLOAK_CLIENT_SECRET"])
validate_required_env_vars(required_env_vars)
Expand Down
4 changes: 2 additions & 2 deletions process_report/tests/unit/invoices/test_pi_specific_invoice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import os
import tempfile
from unittest import TestCase, mock
import pandas

from process_report.tests import util as test_utils
from process_report.invoices.pi_specific_invoice import CHROME_BIN_PATH


class TestPISpecificInvoice(TestCase):
Expand Down Expand Up @@ -138,7 +138,7 @@ def test_export_pi(self, mock_subprocess_run, mock_path_exists, mock_filter_cols
for i, pi_pdf_path in enumerate([pi_pdf_1, pi_pdf_2]):
chrome_arglist, _ = mock_subprocess_run.call_args_list[i]
answer_arglist = [
CHROME_BIN_PATH,
os.environ.get("CHROME_BIN_PATH", "/usr/bin/chromium"),
"--headless",
"--no-sandbox",
f"--print-to-pdf={pi_pdf_path}",
Expand Down
Loading