Repository navigation
fix(connect): use the real server_settings/mail/test endpoint - #760
Draft
ian-flores wants to merge 1 commit into
Draft
ian-flores wants to merge 1 commit into
ian-flores wants to merge 1 commit into
Conversation
send_test_email called POST /__api__/v1/tasks/send-test-email, which does not exist in Connect (it 404s on 2026.09.0, and the path is absent from the Connect source at v2022.02.0, v2024.12.0 and v2026.09.0). The Connect admin UI's "Send Test Email" button uses GET /__api__/server_settings/mail/test, so call that instead. The endpoint takes no parameters, sends to the API key owner's address, requires an administrator key, and returns an empty 200 on success. send_test_email therefore no longer takes a recipient or returns a body. The scenario now skips when the API user is not an administrator or has no email address, and asserts the request is accepted. Selftests cover the request path and an error status.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new prerequisite branches incorrectly report an unperformed configured check as not applicable.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Corrects Connect test-email requests to use the endpoint exercised by the admin UI.
Changes:
- Switches to
GET /server_settings/mail/test. - Updates the BDD scenario for synchronous acceptance.
- Adds request-path and HTTP-error selftests.
| File | Description |
|---|---|
src/vip/clients/connect.py |
Uses the correct test-email endpoint. |
src/vip_tests/connect/test_email.py |
Updates prerequisites and acceptance handling. |
src/vip_tests/connect/test_email.feature |
Revises the expected outcome. |
selftests/test_clients_connect.py |
Tests request routing and error propagation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+25
to
28
| if user.get("user_role") != "administrator": | ||
| pytest.skip("Sending a test email requires an administrator API key") | ||
| if not user.get("email"): | ||
| pytest.skip("Current API user has no email address configured") |
Contributor
|
Preview Links
|
This branch has not been deployed
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
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.
Fixes #751.
test_send_email 404ed because ConnectClient.send_test_email called POST /api/v1/tasks/send-test-email, which Connect has never had. It has been in VIP since the initial scaffold, so the scenario has likely never passed. The Connect admin UI's "Send Test Email" button uses GET /api/server_settings/mail/test, which this PR switches to.
toargument and JSON return value are gone.The second half of the traceback in #751 (list_vip_content failing on a bare array) was already fixed by #748.
Notes: the route is marked internal in Connect's OpenAPI, so it is not part of the documented public API, though the admin UI depends on it. This was verified against the Connect source, not against a live server, so I would appreciate a run against Connect 2026.09.0 from the reporter.