fix: handle empty EMAIL_PORT in test-email endpoint (HTTP 500 → 400) by eltypical · Pull Request #9491 · makeplane/plane

eltypical · 2026-07-29T03:11:52Z

Summary

Bug

On a fresh Plane installation with SKIP_ENV_VAR=True, the EMAIL_PORT row exists in InstanceConfiguration with value "" (empty string) because no SMTP settings have been saved yet.

Clicking Send Test Email before saving the form calls EmailCredentialCheckEndpoint.post(), which reaches:

connection = get_connection(
    port=int(EMAIL_PORT),   # int("") → ValueError → HTTP 500
    ...
)

The unhandled ValueError propagates as an opaque HTTP 500, giving the user no indication of what went wrong.

Root Cause

get_email_configuration() reads EMAIL_PORT from the DB via get_configuration_value(). When SKIP_ENV_VAR=True and the row exists with "", the function returns "" instead of falling back to the env-var default (587). The call site did not guard against this before attempting int().

Fix

Wrap the int(EMAIL_PORT) conversion in a try/except (ValueError, TypeError) block that returns HTTP 400 with a descriptive message, consistent with every other error branch in this endpoint:

try:
    smtp_port = int(EMAIL_PORT)
except (ValueError, TypeError):
    return Response(
        {
            "error": "Email port is not configured. "
                     "Please save your email settings before sending a test email."
        },
        status=status.HTTP_400_BAD_REQUEST,
    )

Tests

Four pytest unit tests added in apps/api/tests/test_email_credential_check.py:

Test	Scenario
`test_returns_400_for_empty_email_port`	`EMAIL_PORT = ""` — the exact regression case
`test_returns_400_for_none_email_port`	`EMAIL_PORT = None` — missing DB row
`test_returns_400_for_non_numeric_email_port`	`EMAIL_PORT = "not-a-port"` — corrupt value
`test_returns_400_when_receiver_email_is_missing`	Pre-existing guard still works

All tests mock get_email_configuration at the call site and assert HTTP 400 + "error" key in the response — no network or DB required.

Change surface

apps/api/plane/license/api/views/configuration.py — 8-line guard added before get_connection(); port=smtp_port replaces inline int() call
apps/api/tests/test_email_credential_check.py — new regression test file

Summary by CodeRabbit

Bug Fixes
- Email credential checks now handle missing or invalid email ports gracefully.
- Invalid port values return a clear HTTP 400 error instead of causing a server error.
Tests
- Added coverage for empty, missing, and non-numeric port values.
- Verified that missing recipient email addresses continue to return a validation error.

When SMTP settings have never been saved (fresh Plane install with SKIP_ENV_VAR=True), the EMAIL_PORT row exists in InstanceConfiguration with value "" (empty string). The call to int(EMAIL_PORT) raised an unhandled ValueError, surfacing as an opaque HTTP 500 to the user. Fix: wrap int(EMAIL_PORT) in a try/except (ValueError, TypeError) that returns HTTP 400 with a descriptive error message, consistent with all other error branches in this endpoint. Regression tests added in tests/test_email_credential_check.py covering empty string, None, and non-numeric port values. Fixes makeplane#9472 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

CLAassistant · 2026-07-29T03:12:01Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

coderabbitai · 2026-07-29T03:12:15Z

📝 Walkthrough

Walkthrough

The email credential check endpoint now validates EMAIL_PORT before SMTP connection setup and returns HTTP 400 for invalid values. Regression tests cover empty, null, and non-numeric ports, plus missing recipient email.

Changes

SMTP port validation

Layer / File(s)	Summary
Validate SMTP port before connection `apps/api/plane/license/api/views/configuration.py`, `apps/api/tests/test_email_credential_check.py`	`EMAIL_PORT` parsing errors now return HTTP 400 with an error payload, and regression tests cover invalid ports and missing `receiver_email`.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

makeplane/plane#9487: Updates SMTP EMAIL_PORT validation in the test_email management command.

Suggested reviewers: dheeru0198

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: validating EMAIL_PORT and returning 400 instead of 500.
Description check	✅ Passed	The description is mostly complete and includes summary, bug, fix, tests, and issue reference.
Linked Issues check	✅ Passed	The code and tests address `#9472` by converting invalid EMAIL_PORT values into a clear HTTP 400 response.
Out of Scope Changes check	✅ Passed	The changes stay focused on the email credential check bug fix and its regression tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/api/tests/test_email_credential_check.py`:
- Around line 49-71: Update all three invalid EMAIL_PORT tests in
test_returns_400_for_empty_email_port, test_returns_400_for_none_email_port, and
test_returns_400_for_non_numeric_email_port to assert that
response.data["error"] contains the configuration-specific message instructing
users to save their email settings, rather than only checking that the error key
exists.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ec8efff6-e17a-421e-a904-1829add638e3

📥 Commits

Reviewing files that changed from the base of the PR and between 49c4da6 and 7e97e04.

📒 Files selected for processing (2)

apps/api/plane/license/api/views/configuration.py
apps/api/tests/test_email_credential_check.py

coderabbitai · 2026-07-29T03:14:09Z

+    @patch("plane.license.api.views.configuration.get_email_configuration")
+    def test_returns_400_for_empty_email_port(self, mock_config):
+        """Regression for #9472: int('') must not propagate as HTTP 500."""
+        mock_config.return_value = self._make_config("")
+        response = self._post({"receiver_email": "test@example.com"})
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert "error" in response.data
+
+    @patch("plane.license.api.views.configuration.get_email_configuration")
+    def test_returns_400_for_none_email_port(self, mock_config):
+        """EMAIL_PORT of None (missing DB row) must not raise TypeError → HTTP 500."""
+        mock_config.return_value = self._make_config(None)
+        response = self._post({"receiver_email": "test@example.com"})
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert "error" in response.data
+
+    @patch("plane.license.api.views.configuration.get_email_configuration")
+    def test_returns_400_for_non_numeric_email_port(self, mock_config):
+        """Non-numeric EMAIL_PORT (corrupt config row) must not cause HTTP 500."""
+        mock_config.return_value = self._make_config("not-a-port")
+        response = self._post({"receiver_email": "test@example.com"})
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert "error" in response.data


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the configuration-specific error message.

These tests only verify that an "error" key exists, so they would pass if the required instruction to save email settings were replaced with a generic message.

Suggested assertion

assert response.status_code == status.HTTP_400_BAD_REQUEST assert "error" in response.data + assert "save your email settings before sending a test email" in response.data["error"]

Apply this assertion to all three invalid-port cases.

Based on the PR objective, the endpoint must return a descriptive message instructing users to save their email settings.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/tests/test_email_credential_check.py` around lines 49 - 71, Update all three invalid EMAIL_PORT tests in test_returns_400_for_empty_email_port, test_returns_400_for_none_email_port, and test_returns_400_for_non_numeric_email_port to assert that response.data["error"] contains the configuration-specific message instructing users to save their email settings, rather than only checking that the error key exists.

eltypical requested a review from dheeru0198 as a code owner July 29, 2026 03:11

Copilot AI review requested due to automatic review settings July 29, 2026 03:11

eltypical requested a review from pablohashescobar as a code owner July 29, 2026 03:11

Copilot AI reviewed Jul 29, 2026

coderabbitai Bot reviewed Jul 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: handle empty EMAIL_PORT in test-email endpoint (HTTP 500 → 400) - #9491

fix: handle empty EMAIL_PORT in test-email endpoint (HTTP 500 → 400)#9491
eltypical wants to merge 1 commit into
makeplane:previewfrom
eltypical:fix/smtp-test-email-empty-port-500

eltypical commented Jul 29, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

CLAassistant commented Jul 29, 2026

Uh oh!

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

eltypical commented Jul 29, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Bug

Root Cause

Fix

Tests

Change surface

Summary by CodeRabbit

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jul 29, 2026

Uh oh!

coderabbitai Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eltypical commented Jul 29, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading