fix more sqlalchemy #1417

terrazoon · 2024-11-14T19:43:50Z

Description

Fix remaining queries affected by the upgrade to sqlalchemy 2.0

NOTE: To find things that need changing the search is grep -ri "query\." ./**/**.py. In order to make search either, occurrences where 'query' was just being used as a variable name were changed to 'querie'.

NOTE: There is one test in notification_dao where I changed the expected result. I think sqlalchemy broke whatever strange thing that test was trying to do. As a matter of good maintenance, I don't think the test is doing the right thing trying to override the behavior of the Pagination object based on a flag.

Security Considerations

N/A

ccostino

Thank you, @terrazoon, this is quite a bit of work and I know the last big push on the query updates! 🎉

I am curious about the change of query to querie and if that was intentional or not, as it is confusing. After catching up in Slack about this, perhaps a quick note explaining the naming would be helpful, and/or a future cleanup task to update the names again/correct the spelling.

I'd also call out the use of scalars() again with some of these, just to make sure that's the intended change we need to have on all of these queries and that it doesn't disrupt anything with the results we're getting back. It only bit us once so far, but noting it again to be absolutely sure!

With the adjustments to some of the tests to also account for the use of scalars(), we're going to have to make sure that the results that the app itself is expecting are still accurate and correct, so there will be a lot of QA involved.

ccostino · 2024-11-20T15:34:03Z

app/dao/fact_billing_dao.py

@@ -65,7 +65,7 @@ def fetch_sms_free_allowance_remainder_until_date(end_date):

 def fetch_sms_billing_for_all_services(start_date, end_date):
    # ASSUMPTION: AnnualBilling has been populated for year.
-    allowance_left_at_start_date_query = fetch_sms_free_allowance_remainder_until_date(
+    allowance_left_at_start_date_querie = fetch_sms_free_allowance_remainder_until_date(


Was this supposed to be changed to allowance_left_at_start_date_queries?

Suggested change

allowance_left_at_start_date_querie = fetch_sms_free_allowance_remainder_until_date(

allowance_left_at_start_date_queries = fetch_sms_free_allowance_remainder_until_date(

I've changed all the querie to stmt, with the side effect that you will now see sub stmt in places.

ccostino · 2024-11-20T15:34:20Z

app/dao/fact_billing_dao.py

@@ -76,14 +76,14 @@ def fetch_sms_billing_for_all_services(start_date, end_date):
    # subtract sms_billable_units units accrued since report's start date to get up-to-date
    # allowance remainder
    sms_allowance_left = func.greatest(
-        allowance_left_at_start_date_query.c.sms_remainder - sms_billable_units, 0
+        allowance_left_at_start_date_querie.c.sms_remainder - sms_billable_units, 0


Same question on the name change:

Suggested change

allowance_left_at_start_date_querie.c.sms_remainder - sms_billable_units, 0

allowance_left_at_start_date_queries.c.sms_remainder - sms_billable_units, 0

ccostino · 2024-11-20T15:34:36Z

app/dao/fact_billing_dao.py

    )

    # billable units here are for period between start date and end date only, so to see
    # how many are chargeable, we need to see how much free allowance was used up in the
    # period up until report's start date and then do a subtraction
    chargeable_sms = func.greatest(
-        sms_billable_units - allowance_left_at_start_date_query.c.sms_remainder, 0
+        sms_billable_units - allowance_left_at_start_date_querie.c.sms_remainder, 0


Same question on the name change:

Suggested change

sms_billable_units - allowance_left_at_start_date_querie.c.sms_remainder, 0

sms_billable_units - allowance_left_at_start_date_queries.c.sms_remainder, 0

ccostino · 2024-11-20T15:34:50Z

app/dao/fact_billing_dao.py

@@ -93,7 +93,7 @@ def fetch_sms_billing_for_all_services(start_date, end_date):
            Organization.id.label("organization_id"),
            Service.name.label("service_name"),
            Service.id.label("service_id"),
-            allowance_left_at_start_date_query.c.free_sms_fragment_limit,
+            allowance_left_at_start_date_querie.c.free_sms_fragment_limit,


Same question on the name change:

Suggested change

allowance_left_at_start_date_querie.c.free_sms_fragment_limit,

allowance_left_at_start_date_queries.c.free_sms_fragment_limit,

ccostino · 2024-11-20T15:35:10Z

app/dao/fact_billing_dao.py

+            allowance_left_at_start_date_querie,
+            Service.id == allowance_left_at_start_date_querie.c.service_id,


Same question on the name change:

Suggested change

allowance_left_at_start_date_querie,

Service.id == allowance_left_at_start_date_querie.c.service_id,

allowance_left_at_start_date_queries,

Service.id == allowance_left_at_start_date_queries.c.service_id,

ccostino · 2024-11-20T15:35:39Z

app/dao/fact_billing_dao.py

+            allowance_left_at_start_date_querie.c.free_sms_fragment_limit,
+            allowance_left_at_start_date_querie.c.sms_remainder,


Same question on the name change:

Suggested change

allowance_left_at_start_date_querie.c.free_sms_fragment_limit,

allowance_left_at_start_date_querie.c.sms_remainder,

allowance_left_at_start_date_queries.c.free_sms_fragment_limit,

allowance_left_at_start_date_queries.c.sms_remainder,

ccostino · 2024-11-20T15:53:36Z

app/performance_dashboard/rest.py

@@ -100,6 +100,7 @@ def transform_into_notification_by_type_json(total_notifications):
 def transform_processing_time_results_to_json(processing_time_results):
    j = []
    for x in processing_time_results:
+        print(f"HERE IS A PROCESSING TIME RESULT {x}")


Do we need to keep this print statement? If so, is there a more useful logging statement we could use instead?

If not, then let's remove!

Suggested change

print(f"HERE IS A PROCESSING TIME RESULT {x}")

ccostino · 2024-11-20T15:59:22Z

tests/app/dao/test_fact_processing_time_dao.py

@@ -77,7 +79,7 @@ def test_get_processing_time_percentage_for_date_range_handles_zero_cases(
    )

    results = get_processing_time_percentage_for_date_range("2021-02-21", "2021-02-22")
-
+    print(f"HERE ARE THE RESULTS {results}")


Same, do we need this print statement here in the tests?

Suggested change

print(f"HERE ARE THE RESULTS {results}")

Kenneth Kehl added 2 commits November 14, 2024 11:26

fix more sqlalchemy

6d8fdab

fix more

7b83ea3

terrazoon temporarily deployed to staging November 14, 2024 21:15 — with GitHub Actions Inactive

hmmm

310386a

terrazoon temporarily deployed to staging November 14, 2024 21:45 — with GitHub Actions Inactive

hmmm

5276ba4

terrazoon temporarily deployed to staging November 14, 2024 21:54 — with GitHub Actions Inactive

hmmm

441b75d

terrazoon temporarily deployed to staging November 14, 2024 22:05 — with GitHub Actions Inactive

hmmm

b708498

terrazoon temporarily deployed to staging November 14, 2024 22:30 — with GitHub Actions Inactive

revert one test

f16c814

terrazoon temporarily deployed to staging November 14, 2024 22:40 — with GitHub Actions Inactive

hmmm

0177fdc

terrazoon temporarily deployed to staging November 14, 2024 22:53 — with GitHub Actions Inactive

more tests

8c7cb44

terrazoon temporarily deployed to staging November 15, 2024 15:21 — with GitHub Actions Inactive

xlorepdarkhelm approved these changes Nov 15, 2024

View reviewed changes

terrazoon marked this pull request as draft November 15, 2024 15:26

more

b0bf042

terrazoon temporarily deployed to staging November 15, 2024 15:49 — with GitHub Actions Inactive

more

704fff7

terrazoon temporarily deployed to staging November 15, 2024 16:09 — with GitHub Actions Inactive

more

8f211cc

terrazoon temporarily deployed to staging November 15, 2024 16:17 — with GitHub Actions Inactive

more

d1ebcba

terrazoon temporarily deployed to staging November 15, 2024 16:26 — with GitHub Actions Inactive

more

1311598

terrazoon temporarily deployed to staging November 15, 2024 16:50 — with GitHub Actions Inactive

fix more

3673a92

fix notifications

76dc06c

terrazoon temporarily deployed to staging November 19, 2024 20:17 — with GitHub Actions Inactive

fix notifications

8b74448

terrazoon temporarily deployed to staging November 19, 2024 20:28 — with GitHub Actions Inactive

fix notifications

331f0eb

terrazoon temporarily deployed to staging November 19, 2024 20:43 — with GitHub Actions Inactive

fix notifications

fd7b3b9

terrazoon temporarily deployed to staging November 19, 2024 20:52 — with GitHub Actions Inactive

fix notifications

a3658ce

terrazoon temporarily deployed to staging November 19, 2024 21:03 — with GitHub Actions Inactive

remove print statements

4a03f5b

terrazoon temporarily deployed to staging November 19, 2024 21:10 — with GitHub Actions Inactive

terrazoon requested review from ccostino, A-Shumway42 and stvnrlly November 19, 2024 21:14

terrazoon self-assigned this Nov 19, 2024

xlorepdarkhelm self-requested a review November 19, 2024 21:34

xlorepdarkhelm approved these changes Nov 20, 2024

View reviewed changes

terrazoon marked this pull request as ready for review November 20, 2024 15:17

ccostino reviewed Nov 20, 2024

View reviewed changes

change querie to stmt

1a1de39

terrazoon temporarily deployed to staging November 21, 2024 17:39 — with GitHub Actions Inactive

code review feedback and merge from main

d283613

terrazoon temporarily deployed to staging November 21, 2024 17:59 — with GitHub Actions Inactive

terrazoon requested a review from ccostino November 21, 2024 18:06

merge from main

6e23d0b

terrazoon temporarily deployed to staging November 22, 2024 15:59 — with GitHub Actions Inactive

terrazoon temporarily deployed to staging November 25, 2024 15:24 — with GitHub Actions Inactive

merge from main

9e5b21e

terrazoon temporarily deployed to staging November 26, 2024 16:37 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix more sqlalchemy #1417

fix more sqlalchemy #1417

terrazoon commented Nov 14, 2024 •

edited

Loading

ccostino left a comment

ccostino Nov 20, 2024

terrazoon Nov 21, 2024

ccostino Nov 20, 2024

ccostino Nov 20, 2024

ccostino Nov 20, 2024

ccostino Nov 20, 2024

ccostino Nov 20, 2024

ccostino Nov 20, 2024

ccostino Nov 20, 2024

	allowance_left_at_start_date_querie = fetch_sms_free_allowance_remainder_until_date(
	allowance_left_at_start_date_queries = fetch_sms_free_allowance_remainder_until_date(

	allowance_left_at_start_date_querie.c.sms_remainder - sms_billable_units, 0
	allowance_left_at_start_date_queries.c.sms_remainder - sms_billable_units, 0

	sms_billable_units - allowance_left_at_start_date_querie.c.sms_remainder, 0
	sms_billable_units - allowance_left_at_start_date_queries.c.sms_remainder, 0

	allowance_left_at_start_date_querie.c.free_sms_fragment_limit,
	allowance_left_at_start_date_queries.c.free_sms_fragment_limit,

		allowance_left_at_start_date_querie,
		Service.id == allowance_left_at_start_date_querie.c.service_id,

fix more sqlalchemy #1417

Are you sure you want to change the base?

fix more sqlalchemy #1417

Conversation

terrazoon commented Nov 14, 2024 • edited Loading

Description

Security Considerations

ccostino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrazoon commented Nov 14, 2024 •

edited

Loading