Skip to content

Process ses results wrapper #2519

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.

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

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

Conversation

whabanks
Copy link
Contributor

@whabanks whabanks commented May 8, 2025

Summary | Résumé

This PR turns process_ses_results into a wrapper so it can handle both batched and unbatched messages from the delivery receipts queue. This will allow us to hot swap the ses_to_sqs_email_callbacks lambda and TF infrastructure between batch saving and non-batch saving versions without dropping any notifications. It is essentially a round about way of feature flagging the batch saving work.

Test instructions | Instructions pour tester la modification

TODO: Fill in test instructions for the reviewer.

Release Instructions | Instructions pour le déploiement

None.

Reviewer checklist | Liste de vérification du réviseur

  • This PR does not break existing functionality.
  • This PR does not violate GCNotify's privacy policies.
  • This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
  • This PR does not significantly alter performance.
  • Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected.

@@ -29,8 +103,160 @@
max_retries=5,
default_retry_delay=300,
)
def process_ses_results(self, response):

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns Note

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.

Copilot Autofix

AI 3 days ago

To fix the issue, we will add an explicit return statement at the end of the process_ses_results function. This ensures that the function always returns a value explicitly, even when neither messages nor message is present in the response. The explicit return value will be None, as this aligns with the current behavior of the function when no conditions are met.


Suggested changeset 1
app/celery/process_ses_receipts_tasks.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py
--- a/app/celery/process_ses_receipts_tasks.py
+++ b/app/celery/process_ses_receipts_tasks.py
@@ -119,3 +119,3 @@
         return unbatched_process_ses_results(self, response)
-
+    return None
 
EOF
@@ -119,3 +119,3 @@
return unbatched_process_ses_results(self, response)

return None

Copilot is powered by AI and may make mistakes. Always verify output.


@statsd(namespace="tasks")
def batched_process_ses_results(self, response):

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns Note

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.

Copilot Autofix

AI 3 days ago

To fix the issue, we will add an explicit return statement at the end of the batched_process_ses_results function. This ensures that the function always returns a consistent value, even if the execution reaches the end without encountering any other return statements. Since the function already uses return True as an explicit return, we will add a return None at the end to make the implicit behavior explicit.


Suggested changeset 1
app/celery/process_ses_receipts_tasks.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py
--- a/app/celery/process_ses_receipts_tasks.py
+++ b/app/celery/process_ses_receipts_tasks.py
@@ -255,3 +255,3 @@
         self.retry(queue=QueueNames.RETRY, args=[{"Messages": updates}])
-
+        return None
 
EOF
@@ -255,3 +255,3 @@
self.retry(queue=QueueNames.RETRY, args=[{"Messages": updates}])

return None

Copilot is powered by AI and may make mistakes. Always verify output.
@statsd(namespace="tasks")
def process_ses_results(self, response): # noqa: C901
def unbatched_process_ses_results(self, response): # noqa: C901

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns Note

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.

Copilot Autofix

AI 3 days ago

To fix the issue, we will ensure that all code paths in the unbatched_process_ses_results function have an explicit return statement. Specifically:

  1. Identify all paths where the function currently falls through without a return statement.
  2. Add return None explicitly at the end of the function or in any other paths where no return value is currently specified.
  3. Ensure that the function's behavior remains unchanged while improving its readability and consistency.

Suggested changeset 1
app/celery/process_ses_receipts_tasks.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py
--- a/app/celery/process_ses_receipts_tasks.py
+++ b/app/celery/process_ses_receipts_tasks.py
@@ -375 +375,3 @@
         self.retry(queue=QueueNames.RETRY)
+
+    return None
EOF
@@ -375 +375,3 @@
self.retry(queue=QueueNames.RETRY)

return None
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants