Add AllocationDailyBillableUsage model and improve tests by jimmysway · Pull Request #317 · nerc-project/coldfront-plugin-cloud

jimmysway · 2026-05-26T21:18:05Z

Closes #292

…mmand Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

…mpty usage, remove duplicate print Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

…odel Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

…Model from the start Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Added tests to validate ORM layer and fetching layer that interacts with ORM

…ueries and standardizing string formatting in models and tests.

…nfo-database-table

reconfigured imports

…nfo-database-table

knikolla · 2026-06-01T15:12:08Z


    def ready(self):
        import coldfront_plugin_cloud.signals  # noqa: F401
+        import coldfront_plugin_cloud.models.daily_billable_usage  # noqa: F401


why is this being imported here?

I don't really understand everything here but I stumbled across this article when trying to figure out why .signals was being imported which led me to this article in the django documentation

https://docs.djangoproject.com/en/6.0/ref/applications/#how-applications-are-loaded

Where it says

Then Django attempts to import the models submodule of each application, if there is one.

You must define or import all models in your application’s models.py or models/init.py. Otherwise, the application registry may not be fully populated at this point, which could cause the ORM to malfunction.

Once this stage completes, APIs that operate on models such as get_model() become usable.

We have historically left the __init__.py blank and I don't know if this daily_billable_usage model is fundamentally different than the other ones because it is ORM related? I might need some help understanding this

Would adding the import in models/__init__.py instead of here work?

knikolla

Did a first pass at reviewing timeboxed to 25mins. Overall looks in the right direction. Please instead of creating a new command enhance the testing code to create the testing setup you need.

knikolla · 2026-06-04T20:21:46Z

+    return f"{today.year}-{today.month:02d}"
+
+
+class Command(BaseCommand):


Please don't create a new command for this. Instead look into either mocking the tests so that the values that you need are returned from the fictional CSV or sample values inserted into the DB.

Just to clarify, this seeding logic is primarily for testing of the downstream graphical UI elements in coldfront-nerc. I will find another way to insert sample values directly into the DB

knikolla · 2026-06-04T20:26:41Z


    def ready(self):
        import coldfront_plugin_cloud.signals  # noqa: F401
+        import coldfront_plugin_cloud.models.daily_billable_usage  # noqa: F401


Would adding the import in models/__init__.py instead of here work?

Copilot

Pull request overview

Adds persistent storage and retrieval for per-allocation daily billable usage by introducing an AllocationDailyBillableUsage model/table, wiring the existing fetch_daily_billable_usage command to upsert rows (and support deleting a day’s rows via --remove), and expanding test coverage plus a local-dev seeding command.

Changes:

Introduce AllocationDailyBillableUsage model + initial migration to store daily usage per (allocation, date, SU type).
Extend fetch_daily_billable_usage to write daily usage rows to DB and add --remove to delete rows for a given date.
Add DB-backed read helpers (billable_usage.py), a seed_daily_billable_usage command, and new/expanded unit tests.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/coldfront_plugin_cloud/tests/unit/test_fetch_daily_billable_usage.py	Adds tests covering DB insert/update behavior and the new `--remove` path.
src/coldfront_plugin_cloud/tests/unit/test_daily_billable_usage.py	New unit tests for DB read helpers and the new model’s constraints/behavior.
src/coldfront_plugin_cloud/models/daily_billable_usage.py	Adds the `AllocationDailyBillableUsage` Django model (FK to Allocation, date, su_type, value).
src/coldfront_plugin_cloud/migrations/0001_initial.py	Creates the new table, indexes, and unique constraint.
src/coldfront_plugin_cloud/migrations/init.py	Migration package marker.
src/coldfront_plugin_cloud/management/commands/seed_daily_billable_usage.py	New command to seed daily usage rows for local development/testing.
src/coldfront_plugin_cloud/management/commands/fetch_daily_billable_usage.py	Persists fetched usage to DB and adds `--remove` option to delete a date’s rows.
src/coldfront_plugin_cloud/billable_usage.py	New helper APIs to read daily usage (single day and date range) from the DB.
src/coldfront_plugin_cloud/apps.py	Imports the new model module during app initialization.
setup.cfg	Adds `django-model-utils` dependency for `TimeStampedModel`.
.gitignore	Ignores `*.db` files (likely local sqlite DB artifacts).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        time.sleep(0.02)
+        row.value = Decimal("150.00")
+        row.save()
+        row.refresh_from_db()


+    for su_type, amount in base.items():
+        step = _RAMP_STEP.get(su_type, 0)
+        ramped[su_type] = str(float(amount) + day_index * step)
+    return usage_models.UsageInfo(ramped)


+    Raises:
+        TypeError: If rows is None.
+        ValueError: If a row has an empty su_type.


jimmysway · 2026-06-05T14:01:21Z

I will go over all the comments but I also have one question right now there is a management/commands/fetch_daily_billable_usage.py which is the fetching code for S3 and also my newly written models/daily_billable_usage.py which is fetching from the database.

I find it confusing sometimes for which is which do you think I should rename these to distinguish between them better or is it fine.

loaded ORM with __init__.py Rewrote testing logic

jimmysway · 2026-06-12T17:00:29Z

I will also update the table implementation to use this injection pattern instead

…bed in nerc-project/coldfront-plugin-cloud#317

QuanMPhm

The structure looks good to me. I have comments on some lower-level details

QuanMPhm · 2026-06-18T17:38:05Z

If this file is intended to inject test data that will be used to test the UI graphs in coldfront-nerc, shouldn't this file be added in that PR instead? I'm not familiar with the UI code or how testing will be done with it yet, but that's my reaction. It's odd seeing a file added in a PR that doesn't it.

@jimmysway Is this only way to test the UI graphs?

Well this file tests the injection pattern that the UI charts will get their data from, injecting sample data into a database tests the entire flow rather than just mocking it at a higher level.

I can for sure add this to another PR but it is very dependent on the contents of this PR so it was the easiest way for me to write a script to write some sample data into the db

QuanMPhm · 2026-06-18T17:42:01Z

+    allocation = models.ForeignKey(
+        Allocation,
+        on_delete=models.CASCADE,
+        related_name="daily_usage_records",


Why do we want related_name to be daily_usage_records?

It lets you write allocation.daily_usage_records to get all the usage rows for a given allocation, instead of Django's ugly default name (allocation.allocationdailybillableusage_set). It's just for readability/convenience when querying.

QuanMPhm · 2026-06-18T17:48:38Z

+        db_table = "coldfront_plugin_cloud_allocationdailybillableusage"
+        unique_together = [["allocation", "date", "su_type"]]
+        indexes = [
+            models.Index(fields=["allocation", "date"]),
+            models.Index(fields=["date"]),
+        ]
+        ordering = ["-date", "allocation", "su_type"]


aside from unique_together, why are any of the other Meta options nessecary? I suppose indexes gives a performance boost.

unique_together can also be simplified to ["allocation", "date", "su_type"], no need for nested list

db_table = "coldfront_plugin_cloud_allocationdailybillableusage"

Will be auto generated by django yes, but the preference for being explicit to navigate the db without a lot of prior understanding behind the magic autogeneration pattern that django would do on its own

QuanMPhm · 2026-06-18T18:02:22Z

+    if rows is None:
+        raise TypeError("rows must not be None")


I impression with the majority of the raised errors in this module is that they are unnecessary. This one, for example, wouldn't be anymore helpful than the TypeError we'll get below from for row in rows:

I would only raise an error if the code encounters a case where we have deliberately decided that the expected behavior should be a raised error. I think raising errors like in this example only adds a slightly more helpful error message, and more mental burden when maintaining and reviewing code. @knikolla Do you have an opinion?

QuanMPhm · 2026-06-18T18:03:16Z

+        if not row.su_type:
+            raise ValueError(f"usage row id={row.pk} has empty su_type")


When would su_type be empty, and do we want to have empty su_types ("")

QuanMPhm · 2026-06-18T18:06:12Z

+    if not isinstance(date, str) or not date.strip():
+        raise ValueError("date must be a non-empty YYYY-MM-DD string")


I think this is unnecessary since validate_date_str would validate the string anyways

QuanMPhm · 2026-06-18T18:16:01Z

+    return usage_info
+
+
+def get_daily_billable_usage(allocation: Allocation, date: str) -> UsageInfo:


Can't we remove this function, since get_daily_billable_usage_by_date can accomplish the same thing by passing the same date to both start_date and end_date?

Alternatively, why not use this in get_daily_billable_usage_by_date? You make a few more queries, but it usually won't be more than a month of queries, yes?

QuanMPhm · 2026-06-18T18:25:00Z

 [._]s[a-rt-v][a-z]
 [._]ss[a-gi-z]
 [._]sw[a-p]
+*.db


I don't think this should be here. I don't think we'll ever need to commit a .db file, but this doesn't seem nessecary

QuanMPhm · 2026-06-18T18:29:10Z

@knikolla It seems if we were to run fetch_daily_billable_usage twice on the same day, and the S3 invoicing data has changed such that a project is gone from the invoices, then the project's usage entry would still persist in the Coldfront database, unless we run --remove and rerun fetch_daily_billable_usage normally again. This is fine?

QuanMPhm · 2026-06-18T18:31:42Z

+        self.assertEqual(usage.root["OpenStack CPU"], Decimal("100"))
+        self.assertEqual(usage.root["Storage"], Decimal("30.12"))
+        self.assertEqual(usage.total_charges, Decimal("130.12"))


For all of these test cases, I think its more clear to just create a UsageInfo or dict "answer", and compare them directly

Copilot AI and others added 21 commits February 10, 2026 20:05

Initial plan

186c879

Add UsageInfo database model and update fetch_daily_billable_usage co…

96285b2

…mmand Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Add unit tests for database insertion and removal functionality

1815ed8

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Fix test assertions to use Decimal for database value comparisons

5e2c5d0

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Remove committed database file and update gitignore

960f7b6

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Fix trailing whitespace in code files

7b070eb

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Address code review feedback: improve related_name, add logging for e…

f1ac8f7

…mpty usage, remove duplicate print Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Rename UsageInfo to AllocationDailyBillableUsage and use TimeStampedM…

6762430

…odel Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Update migration to use AllocationDailyBillableUsage with TimeStamped…

92b4898

…Model from the start Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Remove confusing UsageInfoModel alias in tests

b162bb1

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Clean up redundant import alias in tests

c282a7b

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Address PR feedback: simplify code and use UsageInfo alias

befbe1c

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Keep both UsageInfo classes explicit to avoid confusion

75a1203

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Add explicit UsageInfo import to avoid unnecessary changes

6e68b5d

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Refactor tests: combine insertion/removal and add update test

b88e458

Co-authored-by: knikolla <4123322+knikolla@users.noreply.github.com>

Added methods to retrieve daily billable usage information.

06449fb

Added tests to validate ORM layer and fetching layer that interacts with ORM

Set PYTHONPATH to 'src' in CI scripts for functional and unit tests

01edadc

Refactor code for consistency and readability by simplifying filter q…

3a6eee1

…ueries and standardizing string formatting in models and tests.

Merge remote-tracking branch 'upstream/main' into copilot/add-usage-i…

f4e950e

…nfo-database-table

merged into upstream/main

6bb794e

reconfigured imports

fixed imports

0553fa8

QuanMPhm mentioned this pull request May 28, 2026

Resolve breaking changes by kubernetes package #318

Merged

fixed ruff

8781083

jimmysway force-pushed the copilot/add-usage-info-database-table branch from 01b3089 to 8781083 Compare May 28, 2026 20:07

jimmysway added 2 commits May 28, 2026 16:22

Merge remote-tracking branch 'upstream/main' into copilot/add-usage-i…

7aaa12e

…nfo-database-table

fix linter errors

392a176

jimmysway mentioned this pull request May 29, 2026

feat(#178): Responsiveness & interactivity for PI Dashboard nerc-project/coldfront-nerc#180

Open

knikolla reviewed Jun 1, 2026

View reviewed changes

knikolla requested changes Jun 4, 2026

View reviewed changes

knikolla requested a review from Copilot June 4, 2026 20:29

Copilot started reviewing on behalf of knikolla June 4, 2026 20:29 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

jimmysway force-pushed the copilot/add-usage-info-database-table branch from 805e059 to 39cdb54 Compare June 9, 2026 18:49

Rewrote seeding to avoid Command

7f67ab7

loaded ORM with __init__.py Rewrote testing logic

jimmysway force-pushed the copilot/add-usage-info-database-table branch from 39cdb54 to 7f67ab7 Compare June 9, 2026 20:38

jimmysway requested a review from knikolla June 12, 2026 16:59

Add seeding script for daily billable usage and remove legacy command

5809ec7

jimmysway force-pushed the copilot/add-usage-info-database-table branch from 0b8cb59 to 5809ec7 Compare June 15, 2026 17:54

jimmysway added a commit to jimmysway/coldfront-nerc that referenced this pull request Jun 15, 2026

Updated table only UI to use the new injection pattern that is descri…

609a886

…bed in nerc-project/coldfront-plugin-cloud#317

jimmysway added a commit to jimmysway/coldfront-nerc that referenced this pull request Jun 15, 2026

Updated table only UI to use the new injection pattern that is descri…

0d5d2fc

…bed in nerc-project/coldfront-plugin-cloud#317

QuanMPhm requested changes Jun 18, 2026

View reviewed changes

		return f"{today.year}-{today.month:02d}"


		class Command(BaseCommand):

		if not row.su_type:
		raise ValueError(f"usage row id={row.pk} has empty su_type")

		if not isinstance(date, str) or not date.strip():
		raise ValueError("date must be a non-empty YYYY-MM-DD string")

		return usage_info


		def get_daily_billable_usage(allocation: Allocation, date: str) -> UsageInfo:

Conversation

jimmysway commented May 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmysway Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knikolla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmysway Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

jimmysway commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmysway commented Jun 12, 2026

Uh oh!

QuanMPhm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jimmysway Jun 1, 2026 •

edited

Loading

jimmysway Jun 5, 2026 •

edited

Loading

jimmysway commented Jun 5, 2026 •

edited

Loading