fix(gmail): request settings scope and auto-fill send-as display name - #984
Open
syf2211 wants to merge 2 commits into
Open
fix(gmail): request settings scope and auto-fill send-as display name#984syf2211 wants to merge 2 commits into
syf2211 wants to merge 2 commits into
Conversation
Fixes taylorwilsdon#752 Service Account + DWD mode only grants scopes declared on send_gmail_message. Add gmail.settings.basic so signature fetch works, and reuse one send-as settings lookup to populate From displayName when from_name is omitted.
Contributor
📝 WalkthroughWalkthrough
ChangesGmail send-as settings
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant send_gmail_message
participant GmailSettings
participant _prepare_gmail_message
Caller->>send_gmail_message: send message
send_gmail_message->>GmailSettings: list sendAs settings when needed
GmailSettings-->>send_gmail_message: signature HTML and display name
send_gmail_message->>_prepare_gmail_message: body and resolved_from_name
_prepare_gmail_message-->>send_gmail_message: prepared message
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 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 `@gmail/gmail_tools.py`:
- Around line 2176-2178: Update the require_google_service usage around the
forwarding flow to avoid requiring gmail_settings_basic when forward_message_id
triggers the early return. Preserve the existing forwarding scope set while
retaining the settings scope for operations that need it, using
invocation-dependent scope selection or a separate forwarding path.
🪄 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: cae5008f-41b9-45bc-876e-6bc77a2674b2
📒 Files selected for processing (2)
gmail/gmail_tools.pytests/gmail/test_draft_gmail_message.py
Split send-as settings lookup into a separate authenticated helper that requests gmail_settings_basic only for compose sends. Forwarding no longer requires the settings scope, preserving DWD deployments without that grant. Fixes CodeRabbit review on PR taylorwilsdon#984.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gmail/gmail_tools.py (1)
2433-2442: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve an explicitly empty sender name.
not resolved_from_nametreatsfrom_name=""as omitted and replaces it with the send-as display name. Usefrom_name is Nonefor lookup/autofill checks so every explicitly supplied value takes precedence.Proposed fix
- if include_signature or not resolved_from_name: + if include_signature or from_name is None: signature_html, auto_display_name = await _get_send_as_settings_authenticated( user_google_email=user_google_email, from_email=sender_email, ) @@ - if not resolved_from_name and auto_display_name: + if resolved_from_name is None and auto_display_name: resolved_from_name = auto_display_name🤖 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 `@gmail/gmail_tools.py` around lines 2433 - 2442, Update the sender-name autofill condition in the flow around _get_send_as_settings_authenticated so the send-as display name is applied only when from_name is None, preserving an explicitly supplied empty string while retaining the existing auto_display_name behavior.
🤖 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.
Outside diff comments:
In `@gmail/gmail_tools.py`:
- Around line 2433-2442: Update the sender-name autofill condition in the flow
around _get_send_as_settings_authenticated so the send-as display name is
applied only when from_name is None, preserving an explicitly supplied empty
string while retaining the existing auto_display_name behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59e7a082-9a41-4936-a2cb-5b06abf5a54f
📒 Files selected for processing (2)
gmail/gmail_tools.pytests/gmail/test_draft_gmail_message.py
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.
Summary
Fix
send_gmail_messagefor Service Account + Domain-Wide Delegation mode by requestinggmail.settings.basicand auto-filling the From display name from Gmail send-as settings whenfrom_nameis omitted.Fixes #752
Motivation
In SA+DWD mode, credentials only include scopes declared on the tool decorator.
send_gmail_messagepreviously requested send/read scopes only, so the send-as settings lookup for signatures failed with an auth error and was silently ignored. Separately, when callers omittedfrom_name, outbound mail used a bare email address even though Gmail stores adisplayNameon the send-as entry.Changes
gmail_settings_basicto thesend_gmail_messagescope requirements._get_send_as_settings,_select_send_as_entry) so signature and display name come from one settings round trip.from_namefrom send-asdisplayNamewhen the caller does not provide one.Tests
python3 -m pytest tests/gmail/test_draft_gmail_message.py -k "send_gmail_message" -vResult: 8 passed
Notes
send_gmail_messagenow requiresgmail.settings.basiceven wheninclude_signature=Falseandfrom_nameis provided, so existing send-only grants may need DWD/OAuth re-consent.Summary by CodeRabbit