feat: notify workspace owners when bot integrations run out of credits#906
Open
milovate wants to merge 3 commits into
Open
feat: notify workspace owners when bot integrations run out of credits#906milovate wants to merge 3 commits into
milovate wants to merge 3 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an out-of-credits notification flow: new settings flags ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_out_of_credits_email.py (1)
34-47: Settings modifications may cause test pollution.The tests directly mutate
settings.OUT_OF_CREDITS_EMAIL_ENABLEDand other settings without cleanup. This can leak into subsequent tests if test order changes. Consider using@pytest.fixturewith cleanup or Django's@override_settingsdecorator.♻️ Suggested approach
import pytest from unittest.mock import patch # Option 1: Use pytest fixture with cleanup `@pytest.fixture` def disable_out_of_credits_email(): original = settings.OUT_OF_CREDITS_EMAIL_ENABLED settings.OUT_OF_CREDITS_EMAIL_ENABLED = False yield settings.OUT_OF_CREDITS_EMAIL_ENABLED = original # Option 2: Use mock.patch as context manager def test_dont_send_out_of_credits_email_if_feature_is_disabled(transactional_db): # ... setup ... with patch.object(settings, 'OUT_OF_CREDITS_EMAIL_ENABLED', False): out_of_credits_email_check(sr) assert not pytest_outboxNote: The static analysis warnings about unused
transactional_dbare false positives - this is a pytest-django fixture that enables database transaction support for the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_out_of_credits_email.py` around lines 34 - 47, The test test_dont_send_out_of_credits_email_if_feature_is_disabled mutates global settings.OUT_OF_CREDITS_EMAIL_ENABLED directly which can leak into other tests; change it to temporarily override that setting (e.g. use pytest fixture that saves/restores settings.OUT_OF_CREDITS_EMAIL_ENABLED or use unittest.mock.patch.object as a context manager) around the call to out_of_credits_email_check(sr) so the flag is restored after the test and avoids test pollution.daras_ai_v2/send_email.py (1)
74-100: Consider logging a warning if no workspace owners exist.If
workspace.get_owners()returns an empty QuerySet, the loop silently completes without sending any emails. While this follows the same pattern assend_low_balance_email, consider adding a warning log for observability.♻️ Suggested improvement
def send_out_of_credits_email( *, workspace: "Workspace", integration_name: str, integration_link: str, ): from routers.account import account_route logger.info( f"Sending out-of-credits email for integration {integration_name} {integration_link}..." ) recipients = "support@gooey.ai, devs@gooey.ai" + owners = list(workspace.get_owners()) + if not owners: + logger.warning(f"No owners found for workspace {workspace.id}, skipping out-of-credits email") + return - for user in workspace.get_owners(): + for user in owners: html_body = templates.get_template("out_of_credits_email.html").render(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daras_ai_v2/send_email.py` around lines 74 - 100, In send_out_of_credits_email, check the result of workspace.get_owners() and if it is empty, emit a logger.warning indicating no workspace owners were found (include workspace identifier and integration_name/integration_link for context) before returning; otherwise proceed with the existing loop sending emails to each owner (use the existing recipients fallback/BCC values) so you get observability when no owners receive the notification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@celeryapp/tasks.py`:
- Around line 167-169: The comparison uses InsufficientCredits.__name__ but
elsewhere error types are stored using type(e).__qualname__; update the check in
the sr handling to compare against InsufficientCredits.__qualname__ (i.e.,
change the condition that references InsufficientCredits.__name__ to
InsufficientCredits.__qualname__) so sr.error_type comparisons are consistent
with where sr.error_type is set (referencing sr, InsufficientCredits, and
out_of_credits_email_check to locate the code).
- Around line 300-304: The call to sr.parent_published_run() can return None, so
before calling .title you must guard that value; fetch parent =
sr.parent_published_run() and set integration_name = parent.title if parent is
not None, otherwise use a safe fallback (e.g., sr.parent_version or an
empty/"Unknown" string), then pass integration_name into
send_out_of_credits_email along with the existing workspace and
sr.get_app_url(); update the call site where send_out_of_credits_email(...) is
invoked to use this guarded integration_name.
In `@daras_ai_v2/bots.py`:
- Around line 634-648: Move the format_bot_error(bot: BotInterface) function so
it is defined before any calls (it's currently defined at format_bot_error but
used earlier at the call sites around lines 543 and 589); also change the
WhatsApp-specific link: instead of using bot.saved_run.get_app_url() (which
points to the run details page), return a link that directs the owner to the
integration/billing or integration configuration page where they can manage
credits (e.g., use a billing/integration URL from the BotInterface/bi like
bi.get_config_url() or a global billing page constant), and keep the existing
checks on bot.saved_run.error_type == InsufficientCredits.__name__ and
bot.platform == Platform.WHATSAPP and the human-friendly message and fallback to
ERROR_MSG.format(bot.saved_run.error_msg or "").
---
Nitpick comments:
In `@daras_ai_v2/send_email.py`:
- Around line 74-100: In send_out_of_credits_email, check the result of
workspace.get_owners() and if it is empty, emit a logger.warning indicating no
workspace owners were found (include workspace identifier and
integration_name/integration_link for context) before returning; otherwise
proceed with the existing loop sending emails to each owner (use the existing
recipients fallback/BCC values) so you get observability when no owners receive
the notification.
In `@tests/test_out_of_credits_email.py`:
- Around line 34-47: The test
test_dont_send_out_of_credits_email_if_feature_is_disabled mutates global
settings.OUT_OF_CREDITS_EMAIL_ENABLED directly which can leak into other tests;
change it to temporarily override that setting (e.g. use pytest fixture that
saves/restores settings.OUT_OF_CREDITS_EMAIL_ENABLED or use
unittest.mock.patch.object as a context manager) around the call to
out_of_credits_email_check(sr) so the flag is restored after the test and avoids
test pollution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 493ea261-2d46-433b-a4e5-d66d086e6031
📒 Files selected for processing (9)
celeryapp/tasks.pydaras_ai_v2/bots.pydaras_ai_v2/exceptions.pydaras_ai_v2/send_email.pydaras_ai_v2/settings.pytemplates/out_of_credits_email.htmltests/test_out_of_credits_email.pyworkspaces/migrations/0014_workspace_out_of_credits_email_sent_at.pyworkspaces/models.py
Comment on lines
+167
to
+169
| if sr.is_api_call and sr.error_type == InsufficientCredits.__name__: | ||
| out_of_credits_email_check(sr=sr) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and check context around lines 140 and 167-169
cat -n celeryapp/tasks.py | sed -n '135,145p;165,172p'Repository: GooeyAI/gooey-server
Length of output: 800
🏁 Script executed:
# Find where InsufficientCredits is defined
rg "class InsufficientCredits" -B 2 -A 5Repository: GooeyAI/gooey-server
Length of output: 531
🏁 Script executed:
# Check if InsufficientCredits is imported or defined in celeryapp/tasks.py
rg "InsufficientCredits" celeryapp/tasks.pyRepository: GooeyAI/gooey-server
Length of output: 202
Use __qualname__ for consistency with line 140.
Line 140 stores sr.error_type = type(e).__qualname__, but line 167 compares against InsufficientCredits.__name__. While both currently return the same value since InsufficientCredits is a top-level class, using __qualname__ here ensures consistency and prevents issues if the exception class is ever nested:
Proposed fix
- if sr.is_api_call and sr.error_type == InsufficientCredits.__name__:
+ if sr.is_api_call and sr.error_type == InsufficientCredits.__qualname__:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if sr.is_api_call and sr.error_type == InsufficientCredits.__name__: | |
| out_of_credits_email_check(sr=sr) | |
| if sr.is_api_call and sr.error_type == InsufficientCredits.__qualname__: | |
| out_of_credits_email_check(sr=sr) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@celeryapp/tasks.py` around lines 167 - 169, The comparison uses
InsufficientCredits.__name__ but elsewhere error types are stored using
type(e).__qualname__; update the check in the sr handling to compare against
InsufficientCredits.__qualname__ (i.e., change the condition that references
InsufficientCredits.__name__ to InsufficientCredits.__qualname__) so
sr.error_type comparisons are consistent with where sr.error_type is set
(referencing sr, InsufficientCredits, and out_of_credits_email_check to locate
the code).
milovate
force-pushed
the
wa-credits-exhaust
branch
from
b4461f1 to
11ce363
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
celeryapp/tasks.py (1)
300-304:⚠️ Potential issue | 🟠 MajorGuard
parent_published_run()before accessing.title.Line 302 can raise
AttributeErrorwhen no parent published run is attached.💡 Proposed fix
send_out_of_credits_email( workspace=workspace, - integration_name=sr.parent_published_run().title, + integration_name=( + sr.parent_published_run().title + if sr.parent_published_run() + else "Your integration" + ), integration_link=sr.get_app_url(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@celeryapp/tasks.py` around lines 300 - 304, The call to sr.parent_published_run().title can raise AttributeError when parent_published_run() returns None; update the send_out_of_credits_email invocation to guard the parent run (e.g., assign parent = sr.parent_published_run() or call sr.parent_published_run() once) and pass a safe integration_name such as parent.title if parent is not None else a fallback string (or None), while still using sr.get_app_url() for integration_link; ensure you reference sr, parent_published_run, title, and send_out_of_credits_email when making the change so the code avoids dereferencing None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@celeryapp/tasks.py`:
- Around line 294-307: The current cutoff check then send sequence is race-prone
because two workers can both pass the if-check and both call
send_out_of_credits_email before either writes out_of_credits_email_sent_at;
change this to an atomic conditional update against the Workspace row: compute
now = timezone.now() and run a single query like
Workspace.objects.filter(pk=workspace.pk).filter(Q(out_of_credits_email_sent_at__lt=email_date_cutoff)
|
Q(out_of_credits_email_sent_at__isnull=True)).update(out_of_credits_email_sent_at=now)
and only call send_out_of_credits_email (using sr.parent_published_run().title
and sr.get_app_url()) if that update returns 1 (i.e. row was claimed); otherwise
return without sending. This ensures a DB-level check-and-set instead of
separate read and write.
In `@daras_ai_v2/bots.py`:
- Line 543: The error formatting currently reads bot.saved_run instead of the
freshly created submitted run (sr), causing incorrect messages; update
format_bot_error to accept an explicit run argument (e.g., format_bot_error(bot,
run)) and prefer that run over bot.saved_run inside the function, then change
the callers that handle failures (the places that call format_bot_error(bot)
where sr is in scope—the failing-send sites including the current line and the
other occurrences noted) to pass sr (format_bot_error(bot, sr)); alternatively,
if you prefer minimal changes, set bot.saved_run = sr immediately before the
failing-send call so format_bot_error(bot) sees the correct run—but the
preferred fix is adding the run parameter and updating all call sites.
In `@tests/test_out_of_credits_email.py`:
- Around line 34-136: The tests are leaking global state by mutating settings
and sharing pytest_outbox; update each test (e.g.,
test_dont_send_out_of_credits_email_if_feature_is_disabled,
test_dont_send_out_of_credits_email_if_workspace_not_paying,
test_dont_send_out_of_credits_email_if_recently_emailed,
test_dont_send_out_of_credits_email_if_not_api_call,
test_send_out_of_credits_email) to restore or isolate globals: use pytest's
monkeypatch to set settings.OUT_OF_CREDITS_EMAIL_ENABLED,
settings.OUT_OF_CREDITS_EMAIL_DAYS, settings.LOW_BALANCE_EMAIL_ENABLED only
within the test and ensure cleanup, and clear pytest_outbox at the start (and/or
end) of each test so out_of_credits_email_check and post_runner_tasks see a
clean outbox; alternatively add a fixture that resets relevant settings and does
pytest_outbox.clear() before each test.
---
Duplicate comments:
In `@celeryapp/tasks.py`:
- Around line 300-304: The call to sr.parent_published_run().title can raise
AttributeError when parent_published_run() returns None; update the
send_out_of_credits_email invocation to guard the parent run (e.g., assign
parent = sr.parent_published_run() or call sr.parent_published_run() once) and
pass a safe integration_name such as parent.title if parent is not None else a
fallback string (or None), while still using sr.get_app_url() for
integration_link; ensure you reference sr, parent_published_run, title, and
send_out_of_credits_email when making the change so the code avoids
dereferencing None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9b66bcb-bdda-44de-91ae-436ac56ff937
📒 Files selected for processing (9)
celeryapp/tasks.pydaras_ai_v2/bots.pydaras_ai_v2/exceptions.pydaras_ai_v2/send_email.pydaras_ai_v2/settings.pytemplates/out_of_credits_email.htmltests/test_out_of_credits_email.pyworkspaces/migrations/0014_workspace_out_of_credits_email_sent_at.pyworkspaces/models.py
🚧 Files skipped from review as they are similar to previous changes (3)
- daras_ai_v2/settings.py
- daras_ai_v2/exceptions.py
- templates/out_of_credits_email.html
Comment on lines
+294
to
+307
| if ( | ||
| workspace.out_of_credits_email_sent_at | ||
| and workspace.out_of_credits_email_sent_at >= email_date_cutoff | ||
| ): | ||
| return | ||
|
|
||
| send_out_of_credits_email( | ||
| workspace=workspace, | ||
| integration_name=sr.parent_published_run().title, | ||
| integration_link=sr.get_app_url(), | ||
| ) | ||
| workspace.out_of_credits_email_sent_at = timezone.now() | ||
| workspace.save(update_fields=["out_of_credits_email_sent_at"]) | ||
|
|
Contributor
There was a problem hiding this comment.
Cooldown check and send are race-prone under concurrent task execution.
Two workers can both pass the cutoff check and both send before either writes out_of_credits_email_sent_at.
💡 Proposed fix
+from django.db import transaction
@@
def out_of_credits_email_check(sr: SavedRun):
- workspace = sr.workspace
+ workspace = sr.workspace
@@
- if (
- workspace.out_of_credits_email_sent_at
- and workspace.out_of_credits_email_sent_at >= email_date_cutoff
- ):
- return
-
- send_out_of_credits_email(
- workspace=workspace,
- integration_name=sr.parent_published_run().title,
- integration_link=sr.get_app_url(),
- )
- workspace.out_of_credits_email_sent_at = timezone.now()
- workspace.save(update_fields=["out_of_credits_email_sent_at"])
+ with transaction.atomic():
+ workspace = type(workspace).objects.select_for_update().get(pk=workspace.pk)
+ if (
+ workspace.out_of_credits_email_sent_at
+ and workspace.out_of_credits_email_sent_at >= email_date_cutoff
+ ):
+ return
+ send_out_of_credits_email(
+ workspace=workspace,
+ integration_name=(
+ sr.parent_published_run().title
+ if sr.parent_published_run()
+ else "Your integration"
+ ),
+ integration_link=sr.get_app_url(),
+ )
+ workspace.out_of_credits_email_sent_at = timezone.now()
+ workspace.save(update_fields=["out_of_credits_email_sent_at"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@celeryapp/tasks.py` around lines 294 - 307, The current cutoff check then
send sequence is race-prone because two workers can both pass the if-check and
both call send_out_of_credits_email before either writes
out_of_credits_email_sent_at; change this to an atomic conditional update
against the Workspace row: compute now = timezone.now() and run a single query
like
Workspace.objects.filter(pk=workspace.pk).filter(Q(out_of_credits_email_sent_at__lt=email_date_cutoff)
|
Q(out_of_credits_email_sent_at__isnull=True)).update(out_of_credits_email_sent_at=now)
and only call send_out_of_credits_email (using sr.parent_published_run().title
and sr.get_app_url()) if that update returns 1 (i.e. row was claimed); otherwise
return without sending. This ensures a DB-level check-and-set instead of
separate read and write.
11ce363 to
63b21db
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
daras_ai_v2/bots.py (1)
543-543:⚠️ Potential issue | 🟠 MajorUse the submitted run (
sr) when formatting errors.At Line 543 and Line 589,
format_bot_error(bot)is called while the failing run issr(created at Line 511). Then Line 636/646 readsbot.saved_run, which can show staleerror_type/error_msgand miss the intended out-of-credits branch.Proposed fix
- bot.send_msg(text=format_bot_error(bot)) + bot.send_msg(text=format_bot_error(bot, sr)) @@ - bot.send_msg(text=format_bot_error(bot)) + bot.send_msg(text=format_bot_error(bot, sr)) @@ -def format_bot_error(bot: BotInterface) -> str: +def format_bot_error(bot: BotInterface, sr: SavedRun) -> str: if ( - bot.saved_run.error_type == InsufficientCredits.__name__ + sr.error_type == InsufficientCredits.__name__ and bot.platform == Platform.WHATSAPP ): integration_name = bot.bi.name - integration_link = bot.saved_run.get_app_url() + integration_link = sr.get_app_url() return ( f"💰 The owner of **{integration_name}** is out of Gooey.AI credits." "Please try your message again later.\n\n" f"{integration_link}" ) - return ERROR_MSG.format(bot.saved_run.error_msg or "") + return ERROR_MSG.format(sr.error_msg or "")Also applies to: 589-589, 634-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daras_ai_v2/bots.py` at line 543, The error formatting is using the bot's saved_run instead of the failing run object `sr`; update calls to format_bot_error(bot) used after the failing run is created (the `sr` variable in the surrounding code) to instead pass the submitted run so the formatter can read the correct fields (e.g., call format_bot_error(sr, bot) or otherwise make format_bot_error accept/inspect `sr`). Specifically, replace usages around where `sr` is created and later where `bot.send_msg` is invoked (currently calling format_bot_error(bot)) so the formatter reads `sr.error_type`/`sr.error_msg` and correctly detects out-of-credits, and remove reliance on `bot.saved_run` for those cases.celeryapp/tasks.py (2)
300-303:⚠️ Potential issue | 🟠 MajorGuard
parent_published_run()before reading.title.Line 302 can raise
AttributeErrorwhensr.parent_published_run()isNone.Proposed fix
+ parent_run = sr.parent_published_run() + integration_name = ( + parent_run.title if parent_run and parent_run.title else "Your integration" + ) send_out_of_credits_email( workspace=workspace, - integration_name=sr.parent_published_run().title or "", + integration_name=integration_name, integration_link=sr.get_app_url(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@celeryapp/tasks.py` around lines 300 - 303, The call to sr.parent_published_run() is not guarded before accessing .title and can raise AttributeError; update the send_out_of_credits_email call to first retrieve parent = sr.parent_published_run() (or use getattr) and then pass integration_name = parent.title if parent and parent.title else "" (or use getattr(parent, "title", "") ) so the integration_name argument never dereferences None.
294-307:⚠️ Potential issue | 🟠 MajorCooldown check/send/update sequence is race-prone.
Lines 294-307 perform read-check-send-write in separate steps, so concurrent workers can both send before either writes the timestamp.
Proposed fix
-from django.db.models import Sum, F +from django.db.models import F, Q, Sum @@ - if ( - workspace.out_of_credits_email_sent_at - and workspace.out_of_credits_email_sent_at >= email_date_cutoff - ): - return + now = timezone.now() + claimed = type(workspace).objects.filter(pk=workspace.pk).filter( + Q(out_of_credits_email_sent_at__isnull=True) + | Q(out_of_credits_email_sent_at__lt=email_date_cutoff) + ).update(out_of_credits_email_sent_at=now) + if not claimed: + return send_out_of_credits_email( workspace=workspace, integration_name=sr.parent_published_run().title or "", integration_link=sr.get_app_url(), ) - workspace.out_of_credits_email_sent_at = timezone.now() - workspace.save(update_fields=["out_of_credits_email_sent_at"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@celeryapp/tasks.py` around lines 294 - 307, The current read-check-send-write around workspace.out_of_credits_email_sent_at is race-prone; replace it with an atomic check-and-set so only one worker proceeds to send. Use an atomic DB update (e.g. Workspace.objects.filter(pk=workspace.pk).filter(Q(out_of_credits_email_sent_at__lt=email_date_cutoff) | Q(out_of_credits_email_sent_at__isnull=True)).update(out_of_credits_email_sent_at=timezone.now())) and only call send_out_of_credits_email if the update returned 1 (rows affected); alternatively wrap a select_for_update(lock=True) on the workspace inside transaction.atomic before re-checking and sending, then save—apply this change around the code that currently calls send_out_of_credits_email and workspace.save.
🧹 Nitpick comments (1)
tests/test_out_of_credits_email.py (1)
125-154: Add a regression test for missingparent_version.Current tests always create a
parent_version, so they won’t catch theparent_published_run() is Nonecrash path inceleryapp/tasks.py(Line 302).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_out_of_credits_email.py` around lines 125 - 154, The test currently always creates a parent_version so it doesn't hit the crash path where parent_published_run() returns None; update test_send_out_of_credits_email to cover that case by constructing the support run (sr) without creating a parent_version (or patching sr.parent_published_run to return None) before calling out_of_credits_email_check, then assert the function does not raise and behaves correctly (e.g., sends the expected email or no email as appropriate); reference the test function name test_send_out_of_credits_email and the runtime call parent_published_run()/out_of_credits_email_check to locate where to change the setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daras_ai_v2/bots.py`:
- Around line 642-644: The out-of-credits message built in bots.py concatenates
two string literals for the WhatsApp message, causing "credits.Please" to run
together; update the message assembly that uses integration_name and
integration_link so there is a separator (add a space or newline) between the
first sentence and the "Please try..." sentence—e.g., ensure the string after
f"💰 The owner of **{integration_name}** is out of Gooey.AI credits." ends with
a space or the next literal begins with a leading space/newline so the final
message reads correctly.
In `@daras_ai_v2/send_email.py`:
- Around line 82-84: The info log in send_email.py currently includes the
sensitive integration_link; update the logger.info call (the one that references
integration_name and integration_link) to stop writing the full URL to
persistent logs—log only non-sensitive identifiers such as integration_name and
either a redacted/sanitized version of the link (e.g., domain-only or a
hashed/short token) or omit the link entirely; ensure any helper used for
redaction is applied before logging so no run/user identifiers are emitted.
---
Duplicate comments:
In `@celeryapp/tasks.py`:
- Around line 300-303: The call to sr.parent_published_run() is not guarded
before accessing .title and can raise AttributeError; update the
send_out_of_credits_email call to first retrieve parent =
sr.parent_published_run() (or use getattr) and then pass integration_name =
parent.title if parent and parent.title else "" (or use getattr(parent, "title",
"") ) so the integration_name argument never dereferences None.
- Around line 294-307: The current read-check-send-write around
workspace.out_of_credits_email_sent_at is race-prone; replace it with an atomic
check-and-set so only one worker proceeds to send. Use an atomic DB update (e.g.
Workspace.objects.filter(pk=workspace.pk).filter(Q(out_of_credits_email_sent_at__lt=email_date_cutoff)
|
Q(out_of_credits_email_sent_at__isnull=True)).update(out_of_credits_email_sent_at=timezone.now()))
and only call send_out_of_credits_email if the update returned 1 (rows
affected); alternatively wrap a select_for_update(lock=True) on the workspace
inside transaction.atomic before re-checking and sending, then save—apply this
change around the code that currently calls send_out_of_credits_email and
workspace.save.
In `@daras_ai_v2/bots.py`:
- Line 543: The error formatting is using the bot's saved_run instead of the
failing run object `sr`; update calls to format_bot_error(bot) used after the
failing run is created (the `sr` variable in the surrounding code) to instead
pass the submitted run so the formatter can read the correct fields (e.g., call
format_bot_error(sr, bot) or otherwise make format_bot_error accept/inspect
`sr`). Specifically, replace usages around where `sr` is created and later where
`bot.send_msg` is invoked (currently calling format_bot_error(bot)) so the
formatter reads `sr.error_type`/`sr.error_msg` and correctly detects
out-of-credits, and remove reliance on `bot.saved_run` for those cases.
---
Nitpick comments:
In `@tests/test_out_of_credits_email.py`:
- Around line 125-154: The test currently always creates a parent_version so it
doesn't hit the crash path where parent_published_run() returns None; update
test_send_out_of_credits_email to cover that case by constructing the support
run (sr) without creating a parent_version (or patching sr.parent_published_run
to return None) before calling out_of_credits_email_check, then assert the
function does not raise and behaves correctly (e.g., sends the expected email or
no email as appropriate); reference the test function name
test_send_out_of_credits_email and the runtime call
parent_published_run()/out_of_credits_email_check to locate where to change the
setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5b1bb88-52f6-4326-af89-cb05d0d9a8df
📒 Files selected for processing (9)
celeryapp/tasks.pydaras_ai_v2/bots.pydaras_ai_v2/exceptions.pydaras_ai_v2/send_email.pydaras_ai_v2/settings.pytemplates/out_of_credits_email.htmltests/test_out_of_credits_email.pyworkspaces/migrations/0014_workspace_out_of_credits_email_sent_at.pyworkspaces/models.py
🚧 Files skipped from review as they are similar to previous changes (3)
- daras_ai_v2/exceptions.py
- workspaces/models.py
- daras_ai_v2/settings.py
milovate
force-pushed
the
wa-credits-exhaust
branch
3 times, most recently
from
2473fdf to
fd42039
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
celeryapp/tasks.py (1)
294-306:⚠️ Potential issue | 🟠 MajorMake cooldown/send atomic and handle missing parent published run.
The current read-check-send-write flow can double-send under concurrent workers, and
sr.parent_published_run()may beNonebefore.titleaccess.💡 Proposed fix
+from django.db import transaction @@ def out_of_credits_email_check(sr: SavedRun): @@ - if ( - workspace.out_of_credits_email_sent_at - and workspace.out_of_credits_email_sent_at >= email_date_cutoff - ): - return - - send_out_of_credits_email( - workspace=workspace, - integration_name=sr.parent_published_run().title or "", - integration_link=sr.get_app_url(), - ) - workspace.out_of_credits_email_sent_at = timezone.now() - workspace.save(update_fields=["out_of_credits_email_sent_at"]) + with transaction.atomic(): + workspace = type(workspace).objects.select_for_update().get(pk=workspace.pk) + if ( + workspace.out_of_credits_email_sent_at + and workspace.out_of_credits_email_sent_at >= email_date_cutoff + ): + return + parent_run = sr.parent_published_run() + send_out_of_credits_email( + workspace=workspace, + integration_name=parent_run.title if parent_run else "Your integration", + integration_link=sr.get_app_url(), + ) + workspace.out_of_credits_email_sent_at = timezone.now() + workspace.save(update_fields=["out_of_credits_email_sent_at"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@celeryapp/tasks.py` around lines 294 - 306, The read-check-send-write must be made atomic and guard against sr.parent_published_run() being None: wrap the check-and-mark in a DB transaction using select_for_update on the Workspace (or perform a conditional UPDATE like Workspace.objects.filter(pk=..., out_of_credits_email_sent_at__lt=email_date_cutoff).update(out_of_credits_email_sent_at=now)) so only one worker proceeds to send; after successfully reserving (or seeing the update affected 1 row) call send_out_of_credits_email; and when building integration_name, handle a missing parent run by checking sr.parent_published_run() for None before accessing .title (fall back to empty string) while still passing sr.get_app_url() safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daras_ai_v2/send_email.py`:
- Around line 82-84: The logger.info call in send_email.py currently uses an
unnecessary f-string (logger.info(f"Sending out-of-credits email...")) which
triggers an F541 lint error; remove the f prefix and collapse the call to a
single line (logger.info("Sending out-of-credits email...")) so the string is a
plain literal while keeping the same logger.info invocation.
---
Duplicate comments:
In `@celeryapp/tasks.py`:
- Around line 294-306: The read-check-send-write must be made atomic and guard
against sr.parent_published_run() being None: wrap the check-and-mark in a DB
transaction using select_for_update on the Workspace (or perform a conditional
UPDATE like Workspace.objects.filter(pk=...,
out_of_credits_email_sent_at__lt=email_date_cutoff).update(out_of_credits_email_sent_at=now))
so only one worker proceeds to send; after successfully reserving (or seeing the
update affected 1 row) call send_out_of_credits_email; and when building
integration_name, handle a missing parent run by checking
sr.parent_published_run() for None before accessing .title (fall back to empty
string) while still passing sr.get_app_url() safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bc8824b-e72a-42e4-b09f-9632a3076fc6
📒 Files selected for processing (9)
celeryapp/tasks.pydaras_ai_v2/bots.pydaras_ai_v2/exceptions.pydaras_ai_v2/send_email.pydaras_ai_v2/settings.pytemplates/out_of_credits_email.htmltests/test_out_of_credits_email.pyworkspaces/migrations/0014_workspace_out_of_credits_email_sent_at.pyworkspaces/models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/out_of_credits_email.html
milovate
force-pushed
the
wa-credits-exhaust
branch
from
fd42039 to
041527c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
daras_ai_v2/bots.py (1)
635-646:⚠️ Potential issue | 🟠 MajorDon’t send the run URL back to the WhatsApp user.
sr.get_app_url()is a workspace-side run link, but this message is delivered to the end user who cannot resolve the billing problem. That turns the friendly error into an unhelpful CTA and leaks an internal URL.Safer user-facing message
if ( sr.error_type == InsufficientCredits.__name__ and bot.platform == Platform.WHATSAPP ): integration_name = bot.bi.name - integration_link = sr.get_app_url() return ( f"💰 The owner of **{integration_name}** is out of Gooey.AI credits. " - "Please try your message again later.\n\n" - f"{integration_link}" + "Please try your message again later." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daras_ai_v2/bots.py` around lines 635 - 646, The message returned by format_bot_error leaks an internal workspace run URL via sr.get_app_url() when sr.error_type == InsufficientCredits.__name__ and bot.platform == Platform.WHATSAPP; remove the integration_link and do not include sr.get_app_url() in WhatsApp-facing responses. Update format_bot_error to only use bot.bi.name (and a generic user-facing instruction such as asking the user to retry later or contact the integration owner/support) so no internal URLs or workspace links are sent to end users.
🧹 Nitpick comments (2)
tests/test_out_of_credits_email.py (2)
125-154: Exercise thepost_runner_tasks()success path too.The production wiring lives in
post_runner_tasks(). Callingout_of_credits_email_check()directly here won’t catch a regression in thesr.is_api_call and sr.error_type == InsufficientCredits.__name__branch that actually dispatches this email.Proposed test adjustment
def test_send_out_of_credits_email(transactional_db): @@ workspace.create_with_owner() sr, _ = _make_sr_and_bi(workspace=workspace, bot_name="Support Bot") + sr.error_type = InsufficientCredits.__name__ + sr.save(update_fields=["error_type"]) - out_of_credits_email_check(sr) + post_runner_tasks(sr.id) assert len(pytest_outbox) == 1 @@ assert "Buy Credits" in body assert "run_id=test_run" in body pytest_outbox.clear() - out_of_credits_email_check(sr) + post_runner_tasks(sr.id) assert not pytest_outbox🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_out_of_credits_email.py` around lines 125 - 154, The test calls out_of_credits_email_check() directly, so it doesn't cover the production path in post_runner_tasks() that checks sr.is_api_call and sr.error_type == InsufficientCredits.__name__; update the test to invoke post_runner_tasks() (or the function that triggers that flow) with a SearchResult sr configured with is_api_call=True and error_type set to InsufficientCredits.__name__ so the email dispatch branch is exercised, then assert the pytest_outbox contents as before and that repeat calls do not resend the email.
20-62: Move the fixture/helper block below the tests.
_isolate_email_settings()and_make_sr_and_bi()are dependencies of the test cases, so this module is currently ordered opposite to the repo’s reverse-topological rule.As per coding guidelines, "Format code in reverse topological order: place the main() function at the top and dependencies below it".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_out_of_credits_email.py` around lines 20 - 62, The fixture and helper (_isolate_email_settings and _make_sr_and_bi) are placed above the tests, violating the repo’s reverse-topological ordering; move the entire block defining _isolate_email_settings (the pytest fixture) and _make_sr_and_bi (the helper that creates SavedRun/PublishedRun/BotIntegration objects) so they appear after the test functions in this module, leaving tests at the top and dependencies below; ensure the fixtures' names remain unchanged (so pytest still finds the autouse fixture) and run the test file to confirm no import or name-resolution issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daras_ai_v2/send_email.py`:
- Around line 84-97: The email subject can end up empty when integration_name is
empty; update the subject construction in the send_email_via_postmark call to
use a fallback name (e.g., default_name = integration_name or "Your
integration") so the subject never starts with a leading space. Locate the call
to send_email_via_postmark (and the variable integration_name used when building
the subject) and replace the direct f-string with one that substitutes a
non-empty fallback before formatting the subject string.
---
Duplicate comments:
In `@daras_ai_v2/bots.py`:
- Around line 635-646: The message returned by format_bot_error leaks an
internal workspace run URL via sr.get_app_url() when sr.error_type ==
InsufficientCredits.__name__ and bot.platform == Platform.WHATSAPP; remove the
integration_link and do not include sr.get_app_url() in WhatsApp-facing
responses. Update format_bot_error to only use bot.bi.name (and a generic
user-facing instruction such as asking the user to retry later or contact the
integration owner/support) so no internal URLs or workspace links are sent to
end users.
---
Nitpick comments:
In `@tests/test_out_of_credits_email.py`:
- Around line 125-154: The test calls out_of_credits_email_check() directly, so
it doesn't cover the production path in post_runner_tasks() that checks
sr.is_api_call and sr.error_type == InsufficientCredits.__name__; update the
test to invoke post_runner_tasks() (or the function that triggers that flow)
with a SearchResult sr configured with is_api_call=True and error_type set to
InsufficientCredits.__name__ so the email dispatch branch is exercised, then
assert the pytest_outbox contents as before and that repeat calls do not resend
the email.
- Around line 20-62: The fixture and helper (_isolate_email_settings and
_make_sr_and_bi) are placed above the tests, violating the repo’s
reverse-topological ordering; move the entire block defining
_isolate_email_settings (the pytest fixture) and _make_sr_and_bi (the helper
that creates SavedRun/PublishedRun/BotIntegration objects) so they appear after
the test functions in this module, leaving tests at the top and dependencies
below; ensure the fixtures' names remain unchanged (so pytest still finds the
autouse fixture) and run the test file to confirm no import or name-resolution
issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f18cde0-3dfb-4504-b546-d63adb0ddeca
📒 Files selected for processing (9)
celeryapp/tasks.pydaras_ai_v2/bots.pydaras_ai_v2/exceptions.pydaras_ai_v2/send_email.pydaras_ai_v2/settings.pytemplates/out_of_credits_email.htmltests/test_out_of_credits_email.pyworkspaces/migrations/0014_workspace_out_of_credits_email_sent_at.pyworkspaces/models.py
🚧 Files skipped from review as they are similar to previous changes (2)
- daras_ai_v2/settings.py
- celeryapp/tasks.py
Comment on lines
+84
to
+97
| recipients = "support@gooey.ai, devs@gooey.ai" | ||
| for user in workspace.get_owners(): | ||
| html_body = templates.get_template("out_of_credits_email.html").render( | ||
| user=user, | ||
| integration_name=integration_name, | ||
| integration_link=integration_link, | ||
| buy_credits_url=get_app_route_url(account_route), | ||
| ) | ||
| send_email_via_postmark( | ||
| from_address=settings.SUPPORT_EMAIL, | ||
| to_address=user.email or recipients, | ||
| bcc=recipients, | ||
| subject=f"{integration_name} can't respond due to lack of Gooey.AI credits", | ||
| html_body=html_body, |
Contributor
There was a problem hiding this comment.
Give the subject a fallback title.
out_of_credits_email_check() can pass integration_name = "", so this subject can render as " can't respond due to lack of Gooey.AI credits" even though the body already has an unnamed-integration fallback.
Small fallback for the subject line
send_email_via_postmark(
from_address=settings.SUPPORT_EMAIL,
to_address=user.email or recipients,
bcc=recipients,
- subject=f"{integration_name} can't respond due to lack of Gooey.AI credits",
+ subject=f"{integration_name or 'Your bot'} can't respond due to lack of Gooey.AI credits",
html_body=html_body,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| recipients = "support@gooey.ai, devs@gooey.ai" | |
| for user in workspace.get_owners(): | |
| html_body = templates.get_template("out_of_credits_email.html").render( | |
| user=user, | |
| integration_name=integration_name, | |
| integration_link=integration_link, | |
| buy_credits_url=get_app_route_url(account_route), | |
| ) | |
| send_email_via_postmark( | |
| from_address=settings.SUPPORT_EMAIL, | |
| to_address=user.email or recipients, | |
| bcc=recipients, | |
| subject=f"{integration_name} can't respond due to lack of Gooey.AI credits", | |
| html_body=html_body, | |
| recipients = "support@gooey.ai, devs@gooey.ai" | |
| for user in workspace.get_owners(): | |
| html_body = templates.get_template("out_of_credits_email.html").render( | |
| user=user, | |
| integration_name=integration_name, | |
| integration_link=integration_link, | |
| buy_credits_url=get_app_route_url(account_route), | |
| ) | |
| send_email_via_postmark( | |
| from_address=settings.SUPPORT_EMAIL, | |
| to_address=user.email or recipients, | |
| bcc=recipients, | |
| subject=f"{integration_name or 'Your bot'} can't respond due to lack of Gooey.AI credits", | |
| html_body=html_body, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daras_ai_v2/send_email.py` around lines 84 - 97, The email subject can end up
empty when integration_name is empty; update the subject construction in the
send_email_via_postmark call to use a fallback name (e.g., default_name =
integration_name or "Your integration") so the subject never starts with a
leading space. Locate the call to send_email_via_postmark (and the variable
integration_name used when building the subject) and replace the direct f-string
with one that substitutes a non-empty fallback before formatting the subject
string.
- show a user-friendly message to WhatsApp end-users instead of the generic error.
milovate
force-pushed
the
wa-credits-exhaust
branch
2 times, most recently
from
8f9931e to
5158f18
Compare
milovate
force-pushed
the
wa-credits-exhaust
branch
from
5158f18 to
ecab155
Compare
Member
|
I am worried we are duplicating functionality here between the low balance email and out of credits email. My take is that the code path is really similar between the two and we should just update the low balance email template with the better messaging |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Q/A checklist
How to check import time?
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.