feat(dingtalk): implement one-click QR registration and polling mechanism#8198
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The Lark and Dingtalk creation mode UI logic in AddNewPlatform.vue is largely duplicated (state, validation, template structure); consider extracting a shared creation-mode component or generic reactive state to reduce repetition and keep future platform additions simpler.
- In PlatformRegistrationAction.vue,
handleSuccesswritesclient_idandclient_secretintoplatformConfigwhenever they exist in the response; consider restricting this to the Dingtalk platform (or checkingplatformConfig.type) to avoid accidentally mutating other platform configs if backend responses change. - In
_handle_dingtalk_registrationthestartresponse exposes bothdevice_codeandregistration_codewith identical values; if only one is needed, simplifying this payload or clearly standardizing on a single field name would reduce potential confusion for frontend consumers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Lark and Dingtalk creation mode UI logic in AddNewPlatform.vue is largely duplicated (state, validation, template structure); consider extracting a shared creation-mode component or generic reactive state to reduce repetition and keep future platform additions simpler.
- In PlatformRegistrationAction.vue, `handleSuccess` writes `client_id` and `client_secret` into `platformConfig` whenever they exist in the response; consider restricting this to the Dingtalk platform (or checking `platformConfig.type`) to avoid accidentally mutating other platform configs if backend responses change.
- In `_handle_dingtalk_registration` the `start` response exposes both `device_code` and `registration_code` with identical values; if only one is needed, simplifying this payload or clearly standardizing on a single field name would reduce potential confusion for frontend consumers.
## Individual Comments
### Comment 1
<location path="dashboard/src/components/platform/PlatformRegistrationAction.vue" line_range="69-74" />
<code_context>
successKey: 'registrationAction.weixinOc.created',
statusKeyPrefix: 'registrationAction.weixinOc.status',
},
+ dingtalk: {
+ endpoint: '/api/platform/registration/dingtalk',
+ icon: 'mdi-qrcode',
+ titleKey: 'registrationAction.dingtalk.title',
+ scanTitleKey: 'registrationAction.dingtalk.scanTitle',
+ successKey: 'registrationAction.dingtalk.created',
+ },
};
</code_context>
<issue_to_address>
**issue (bug_risk):** DingTalk registration config lacks a statusKeyPrefix, which may break or limit status messaging.
Other providers define a `statusKeyPrefix`, and the UI logic (e.g. `getStatusText`) likely depends on it to resolve localized status messages. Without it, DingTalk statuses may be missing or fall back to defaults. Please add a `statusKeyPrefix` for DingTalk consistent with the existing entries if it uses status messages in the same way.
</issue_to_address>
### Comment 2
<location path="astrbot/core/platform/sources/dingtalk/app_registration.py" line_range="42-48" />
<code_context>
+ return ""
+
+
+def _int_field(data: dict[str, Any], key: str, default: int) -> int:
+ value = data.get(key)
+ if isinstance(value, int):
+ return value
+ if isinstance(value, float):
+ return int(value)
+ return default
+
+
</code_context>
<issue_to_address>
**suggestion:** Integer parsing is strict and ignores numeric strings, which could cause subtle integration issues.
`_int_field` only accepts `int` and `float`, so numeric strings (e.g. "7200") will silently fall back to `default` if the API ever returns them as strings. To make this more robust to API changes or inconsistent environments, consider handling `str` values by attempting `int(value)` before using `default`.
```suggestion
def _int_field(data: dict[str, Any], key: str, default: int) -> int:
value = data.get(key)
if isinstance(value, int):
return value
if isinstance(value, float):
return int(value)
if isinstance(value, str):
try:
# Allow numeric strings like "7200" to be parsed as integers
return int(value)
except ValueError:
pass
return default
```
</issue_to_address>
### Comment 3
<location path="astrbot/core/platform/sources/dingtalk/app_registration.py" line_range="129-138" />
<code_context>
+ if status_raw == "SUCCESS":
+ client_id = _string_field(raw, "client_id")
+ client_secret = _string_field(raw, "client_secret")
+ if not client_id or not client_secret:
+ return {"status": "error", "message": "扫码成功但未获取到钉钉应用凭证"}
+ return {
+ "status": "created",
+ "client_id": client_id,
+ "client_secret": client_secret,
+ }
+ if status_raw == "FAIL":
+ return {
+ "status": "error",
+ "message": _string_field(raw, "fail_reason") or "钉钉扫码创建失败",
+ }
+ if status_raw == "EXPIRED":
+ return {"status": "expired", "message": "钉钉扫码已过期，请重新创建"}
+ return {
+ "status": "error",
+ "message": f"钉钉扫码创建返回未知状态: {status_raw or 'UNKNOWN'}",
+ }
</code_context>
<issue_to_address>
**suggestion:** The poll result messages are hard-coded in Chinese, which may not align with the app’s localization strategy.
These error/status messages are Chinese-only. If they can reach a multi-locale UI, consider returning structured error codes and letting a higher layer map them to localized strings, or otherwise making the messages language-agnostic so the frontend can handle localization.
Suggested implementation:
```python
def dingtalk_registration_poll_result(raw: dict[str, Any]) -> dict[str, Any]:
status_raw = _string_field(raw, "status").upper()
if status_raw == "WAITING":
return {"status": "pending"}
if status_raw == "SUCCESS":
client_id = _string_field(raw, "client_id")
client_secret = _string_field(raw, "client_secret")
if not client_id or not client_secret:
# Missing app credentials from DingTalk after a reported success.
# Return a structured error code so the caller can localize the message.
return {
"status": "error",
"error_code": "DINGTALK_APP_CREDENTIAL_MISSING",
}
return {
"status": "created",
"client_id": client_id,
"client_secret": client_secret,
}
if status_raw == "FAIL":
fail_reason = _string_field(raw, "fail_reason")
# Expose the raw reason as non-localized detail, and a stable error_code
# for the UI / higher layers to map to localized strings.
result: dict[str, Any] = {
"status": "error",
"error_code": "DINGTALK_QR_REGISTRATION_FAILED",
}
if fail_reason:
result["error_detail"] = fail_reason
return result
if status_raw == "EXPIRED":
return {
"status": "expired",
"error_code": "DINGTALK_QR_EXPIRED",
}
return {
"status": "error",
"error_code": "DINGTALK_QR_UNKNOWN_STATUS",
"raw_status": status_raw or "UNKNOWN",
```
1. Callers that currently rely on the Chinese `message` field will need to be updated to:
- Prefer `error_code` for handling / localization.
- Optionally surface `error_detail` and/or `raw_status` as debug info or fallbacks.
2. If your API contract requires a `message` field, you can:
- Either keep `message` as a short, language-agnostic English string alongside `error_code`, or
- Let the web/API layer wrap these results and inject localized `message` strings based on `error_code`.
</issue_to_address>
### Comment 4
<location path="tests/test_dingtalk_app_registration.py" line_range="10-15" />
<code_context>
+)
+
+
+def test_dingtalk_registration_defaults(monkeypatch):
+ monkeypatch.delenv("DINGTALK_REGISTRATION_BASE_URL", raising=False)
+ monkeypatch.delenv("DINGTALK_REGISTRATION_SOURCE", raising=False)
+
+ assert dingtalk_registration_base_url() == DEFAULT_DINGTALK_REGISTRATION_BASE_URL
+ assert dingtalk_registration_source() == DEFAULT_DINGTALK_REGISTRATION_SOURCE
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for environment-based overrides and URL normalization of DingTalk registration settings
This only covers the defaults when env vars are unset. Please also add tests where `DINGTALK_REGISTRATION_BASE_URL` and `DINGTALK_REGISTRATION_SOURCE` are set, including cases with surrounding whitespace and trailing slashes in the base URL, to verify the `strip()` and `rstrip('/')` behavior and avoid configuration surprises.
</issue_to_address>
### Comment 5
<location path="tests/test_dingtalk_app_registration.py" line_range="1-6" />
<code_context>
from astrbot.core import logger
from astrbot.core.core_lifecycle import AstrBotCoreLifecycle
from astrbot.core.platform import Platform
+from astrbot.core.platform.sources.dingtalk.app_registration import (
+ poll_dingtalk_app_registration_once,
+ request_dingtalk_app_registration,
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the async registration workflow and error handling around DingTalk API calls
These new async functions (`request_dingtalk_app_registration`, `poll_dingtalk_app_registration_once`, `_raise_dingtalk_registration_error`) are not currently covered by tests. Please add async tests that:
- Mock `_post_registration` (or `aiohttp.ClientSession.post`) to cover: successful init+begin, and cases with missing `nonce`, `device_code`, and `verification_uri_complete`, asserting the expected `RuntimeError` messages.
- Check that `expires_in` and `interval` are clamped to the configured minimum values.
- For `poll_dingtalk_app_registration_once`, simulate non-zero `errcode` and HTTP status ≥ 400 to verify `_raise_dingtalk_registration_error` raises the appropriate exception.
This will validate the new network logic and error handling without calling real DingTalk endpoints.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+69
to
+74
| dingtalk: { | ||
| endpoint: '/api/platform/registration/dingtalk', | ||
| icon: 'mdi-qrcode', | ||
| titleKey: 'registrationAction.dingtalk.title', | ||
| scanTitleKey: 'registrationAction.dingtalk.scanTitle', | ||
| successKey: 'registrationAction.dingtalk.created', |
Contributor
There was a problem hiding this comment.
issue (bug_risk): DingTalk registration config lacks a statusKeyPrefix, which may break or limit status messaging.
Other providers define a statusKeyPrefix, and the UI logic (e.g. getStatusText) likely depends on it to resolve localized status messages. Without it, DingTalk statuses may be missing or fall back to defaults. Please add a statusKeyPrefix for DingTalk consistent with the existing entries if it uses status messages in the same way.
Comment on lines
+42
to
+48
| def _int_field(data: dict[str, Any], key: str, default: int) -> int: | ||
| value = data.get(key) | ||
| if isinstance(value, int): | ||
| return value | ||
| if isinstance(value, float): | ||
| return int(value) | ||
| return default |
Contributor
There was a problem hiding this comment.
suggestion: Integer parsing is strict and ignores numeric strings, which could cause subtle integration issues.
_int_field only accepts int and float, so numeric strings (e.g. "7200") will silently fall back to default if the API ever returns them as strings. To make this more robust to API changes or inconsistent environments, consider handling str values by attempting int(value) before using default.
Suggested change
| def _int_field(data: dict[str, Any], key: str, default: int) -> int: | |
| value = data.get(key) | |
| if isinstance(value, int): | |
| return value | |
| if isinstance(value, float): | |
| return int(value) | |
| return default | |
| def _int_field(data: dict[str, Any], key: str, default: int) -> int: | |
| value = data.get(key) | |
| if isinstance(value, int): | |
| return value | |
| if isinstance(value, float): | |
| return int(value) | |
| if isinstance(value, str): | |
| try: | |
| # Allow numeric strings like "7200" to be parsed as integers | |
| return int(value) | |
| except ValueError: | |
| pass | |
| return default |
Comment on lines
+129
to
+138
| if not client_id or not client_secret: | ||
| return {"status": "error", "message": "扫码成功但未获取到钉钉应用凭证"} | ||
| return { | ||
| "status": "created", | ||
| "client_id": client_id, | ||
| "client_secret": client_secret, | ||
| } | ||
| if status_raw == "FAIL": | ||
| return { | ||
| "status": "error", |
Contributor
There was a problem hiding this comment.
suggestion: The poll result messages are hard-coded in Chinese, which may not align with the app’s localization strategy.
These error/status messages are Chinese-only. If they can reach a multi-locale UI, consider returning structured error codes and letting a higher layer map them to localized strings, or otherwise making the messages language-agnostic so the frontend can handle localization.
Suggested implementation:
def dingtalk_registration_poll_result(raw: dict[str, Any]) -> dict[str, Any]:
status_raw = _string_field(raw, "status").upper()
if status_raw == "WAITING":
return {"status": "pending"}
if status_raw == "SUCCESS":
client_id = _string_field(raw, "client_id")
client_secret = _string_field(raw, "client_secret")
if not client_id or not client_secret:
# Missing app credentials from DingTalk after a reported success.
# Return a structured error code so the caller can localize the message.
return {
"status": "error",
"error_code": "DINGTALK_APP_CREDENTIAL_MISSING",
}
return {
"status": "created",
"client_id": client_id,
"client_secret": client_secret,
}
if status_raw == "FAIL":
fail_reason = _string_field(raw, "fail_reason")
# Expose the raw reason as non-localized detail, and a stable error_code
# for the UI / higher layers to map to localized strings.
result: dict[str, Any] = {
"status": "error",
"error_code": "DINGTALK_QR_REGISTRATION_FAILED",
}
if fail_reason:
result["error_detail"] = fail_reason
return result
if status_raw == "EXPIRED":
return {
"status": "expired",
"error_code": "DINGTALK_QR_EXPIRED",
}
return {
"status": "error",
"error_code": "DINGTALK_QR_UNKNOWN_STATUS",
"raw_status": status_raw or "UNKNOWN",- Callers that currently rely on the Chinese
messagefield will need to be updated to:- Prefer
error_codefor handling / localization. - Optionally surface
error_detailand/orraw_statusas debug info or fallbacks.
- Prefer
- If your API contract requires a
messagefield, you can:- Either keep
messageas a short, language-agnostic English string alongsideerror_code, or - Let the web/API layer wrap these results and inject localized
messagestrings based onerror_code.
- Either keep
Comment on lines
+10
to
+15
| def test_dingtalk_registration_defaults(monkeypatch): | ||
| monkeypatch.delenv("DINGTALK_REGISTRATION_BASE_URL", raising=False) | ||
| monkeypatch.delenv("DINGTALK_REGISTRATION_SOURCE", raising=False) | ||
|
|
||
| assert dingtalk_registration_base_url() == DEFAULT_DINGTALK_REGISTRATION_BASE_URL | ||
| assert dingtalk_registration_source() == DEFAULT_DINGTALK_REGISTRATION_SOURCE |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for environment-based overrides and URL normalization of DingTalk registration settings
This only covers the defaults when env vars are unset. Please also add tests where DINGTALK_REGISTRATION_BASE_URL and DINGTALK_REGISTRATION_SOURCE are set, including cases with surrounding whitespace and trailing slashes in the base URL, to verify the strip() and rstrip('/') behavior and avoid configuration surprises.
Comment on lines
+1
to
+6
| from astrbot.core.platform.sources.dingtalk.app_registration import ( | ||
| DEFAULT_DINGTALK_REGISTRATION_BASE_URL, | ||
| DEFAULT_DINGTALK_REGISTRATION_SOURCE, | ||
| dingtalk_registration_base_url, | ||
| dingtalk_registration_poll_result, | ||
| dingtalk_registration_source, |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for the async registration workflow and error handling around DingTalk API calls
These new async functions (request_dingtalk_app_registration, poll_dingtalk_app_registration_once, _raise_dingtalk_registration_error) are not currently covered by tests. Please add async tests that:
- Mock
_post_registration(oraiohttp.ClientSession.post) to cover: successful init+begin, and cases with missingnonce,device_code, andverification_uri_complete, asserting the expectedRuntimeErrormessages. - Check that
expires_inandintervalare clamped to the configured minimum values. - For
poll_dingtalk_app_registration_once, simulate non-zeroerrcodeand HTTP status ≥ 400 to verify_raise_dingtalk_registration_errorraises the appropriate exception.
This will validate the new network logic and error handling without calling real DingTalk endpoints.
dosubot
Bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a one-click QR code registration flow for DingTalk, encompassing backend API clients, dashboard route handlers, frontend UI components, and updated documentation. The reviewer identified several improvement opportunities: optimizing network performance by reusing aiohttp sessions instead of creating them per request, moving hardcoded backend strings to the frontend to adhere to internationalization best practices, and refactoring duplicate response construction logic into a shared helper function to improve maintainability.
| payload: dict[str, str], | ||
| ) -> tuple[int, dict[str, Any]]: | ||
| timeout = aiohttp.ClientTimeout(total=15) | ||
| async with aiohttp.ClientSession(timeout=timeout, trust_env=True) as session: |
Contributor
There was a problem hiding this comment.
Creating a new aiohttp.ClientSession for every request is inefficient as it prevents connection pooling and increases overhead. It is recommended to use a single persistent session for the application or pass an existing session to this function.
Comment on lines
+130
to
+145
| return {"status": "error", "message": "扫码成功但未获取到钉钉应用凭证"} | ||
| return { | ||
| "status": "created", | ||
| "client_id": client_id, | ||
| "client_secret": client_secret, | ||
| } | ||
| if status_raw == "FAIL": | ||
| return { | ||
| "status": "error", | ||
| "message": _string_field(raw, "fail_reason") or "钉钉扫码创建失败", | ||
| } | ||
| if status_raw == "EXPIRED": | ||
| return {"status": "expired", "message": "钉钉扫码已过期，请重新创建"} | ||
| return { | ||
| "status": "error", | ||
| "message": f"钉钉扫码创建返回未知状态: {status_raw or 'UNKNOWN'}", |
Contributor
There was a problem hiding this comment.
Hardcoded user-facing strings in the backend (e.g., "扫码成功但未获取到钉钉应用凭证") violate internationalization (i18n) best practices. These messages should be moved to the frontend or replaced with translation keys that can be resolved using the project's locale files.
Comment on lines
+212
to
+227
| return ( | ||
| Response() | ||
| .ok( | ||
| { | ||
| "status": "pending", | ||
| "device_code": registration.device_code, | ||
| "registration_code": registration.device_code, | ||
| "user_code": registration.user_code, | ||
| "verification_uri": registration.verification_uri, | ||
| "verification_uri_complete": registration.verification_uri_complete, | ||
| "expires_in": registration.expires_in, | ||
| "interval": registration.interval, | ||
| } | ||
| ) | ||
| .__dict__ | ||
| ) |
Contributor
There was a problem hiding this comment.
The logic for constructing the registration response is identical to the one used in _handle_lark_registration. Per the general rules of this repository, this should be refactored into a shared helper function to avoid code duplication and improve maintainability.
References
- When implementing similar functionality for different cases, refactor the logic into a shared helper function to avoid code duplication.
dosubot
Bot
added
the
area:platform
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
1 participant
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.
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add one-click QR-based DingTalk bot registration with backend polling support and integrate it into the platform creation UI alongside existing methods.
New Features:
Enhancements:
Documentation:
Tests: