feat(sheets): add data validation tools (set/clear + read dropdowns)#892
Open
emilio-S03 wants to merge 2 commits into
Open
feat(sheets): add data validation tools (set/clear + read dropdowns)#892emilio-S03 wants to merge 2 commits into
emilio-S03 wants to merge 2 commits into
Conversation
Adds a Google Sheets tool to set or clear data validation on a range: - ONE_OF_LIST dropdown from an explicit list of values - ONE_OF_RANGE dropdown sourced from another A1 range - clear validation via action="clear" Supports strict/warn enforcement, the dropdown chip (showCustomUi), and an optional input message. Mirrors the existing format_sheet_range / manage_conditional_formatting pattern. Registered under the sheets "complete" tier and uses the existing sheets write scope (no new scope). Includes unit tests.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds Sheets tools for setting, clearing, and reading data validation rules, registers them in the Sheets tool tier, and adds async tests covering both tool paths. ChangesSheets data validation tools
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 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 `@gsheets/sheets_tools.py`:
- Around line 2459-2494: The dropdown rule builder in the set/clear validation
flow accepts a whitespace-only source_range and only fails later when building
the ONE_OF_RANGE condition. In the function that normalizes action_normalized
and constructs dv_request/condition, reject blank source_range values up front
(after stripping) with a UserInputError before calling _parse_a1_range or
creating the userEnteredValue. Keep the existing exclusivity check for values vs
source_range, but add a validation path that treats empty or whitespace-only
source_range as invalid input.
🪄 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
Run ID: 5d153d14-c775-4704-ba34-5a448cf50d18
📒 Files selected for processing (3)
core/tool_tiers.yamlgsheets/sheets_tools.pytests/gsheets/test_manage_data_validation.py
Comment on lines
+2459
to
+2494
| if action_normalized == "set" and bool(values) == bool(source_range): | ||
| raise UserInputError( | ||
| "For action='set', provide exactly one of 'values' (a list of dropdown " | ||
| "options) or 'source_range' (an A1 range to source the options from)." | ||
| ) | ||
|
|
||
| metadata = await asyncio.to_thread( | ||
| service.spreadsheets() | ||
| .get( | ||
| spreadsheetId=spreadsheet_id, | ||
| fields="sheets(properties(sheetId,title))", | ||
| ) | ||
| .execute | ||
| ) | ||
| sheets = metadata.get("sheets", []) | ||
| grid_range = _parse_a1_range(range_name, sheets) | ||
|
|
||
| if action_normalized == "clear": | ||
| # Omitting 'rule' from setDataValidation clears validation on the range. | ||
| dv_request = {"range": grid_range} | ||
| summary = "cleared data validation" | ||
| else: | ||
| if values: | ||
| condition = { | ||
| "type": "ONE_OF_LIST", | ||
| "values": [{"userEnteredValue": str(v)} for v in values], | ||
| } | ||
| summary = f"set a dropdown with {len(values)} option(s)" | ||
| else: | ||
| ref = source_range.strip() | ||
| if not ref.startswith("="): | ||
| ref = "=" + ref | ||
| condition = { | ||
| "type": "ONE_OF_RANGE", | ||
| "values": [{"userEnteredValue": ref}], | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject blank source_range values before building the rule.
A whitespace-only source_range passes the Lines 2459-2463 exclusivity check, gets stripped to "" on Lines 2488-2490, and is then sent as userEnteredValue: "=". That turns a local input mistake into a Sheets 400 instead of a UserInputError.
Suggested fix
async def _manage_data_validation_impl(
service,
spreadsheet_id: str,
range_name: str,
@@
) -> dict:
"""Internal implementation for manage_data_validation (decorator-free for tests)."""
action_normalized = (action or "").strip().lower()
+ normalized_source_range = (
+ source_range.strip() if source_range is not None else None
+ )
+
if action_normalized not in ("set", "clear"):
raise UserInputError("action must be 'set' or 'clear'.")
- if action_normalized == "set" and bool(values) == bool(source_range):
+ if action_normalized == "set" and bool(values) == bool(normalized_source_range):
raise UserInputError(
"For action='set', provide exactly one of 'values' (a list of dropdown "
"options) or 'source_range' (an A1 range to source the options from)."
)
@@
if values:
condition = {
"type": "ONE_OF_LIST",
"values": [{"userEnteredValue": str(v)} for v in values],
}
summary = f"set a dropdown with {len(values)} option(s)"
else:
- ref = source_range.strip()
+ ref = normalized_source_range
+ if ref in ("", "="):
+ raise UserInputError(
+ "For action='set', 'source_range' must be a non-empty A1 range."
+ )
if not ref.startswith("="):
ref = "=" + ref📝 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 action_normalized == "set" and bool(values) == bool(source_range): | |
| raise UserInputError( | |
| "For action='set', provide exactly one of 'values' (a list of dropdown " | |
| "options) or 'source_range' (an A1 range to source the options from)." | |
| ) | |
| metadata = await asyncio.to_thread( | |
| service.spreadsheets() | |
| .get( | |
| spreadsheetId=spreadsheet_id, | |
| fields="sheets(properties(sheetId,title))", | |
| ) | |
| .execute | |
| ) | |
| sheets = metadata.get("sheets", []) | |
| grid_range = _parse_a1_range(range_name, sheets) | |
| if action_normalized == "clear": | |
| # Omitting 'rule' from setDataValidation clears validation on the range. | |
| dv_request = {"range": grid_range} | |
| summary = "cleared data validation" | |
| else: | |
| if values: | |
| condition = { | |
| "type": "ONE_OF_LIST", | |
| "values": [{"userEnteredValue": str(v)} for v in values], | |
| } | |
| summary = f"set a dropdown with {len(values)} option(s)" | |
| else: | |
| ref = source_range.strip() | |
| if not ref.startswith("="): | |
| ref = "=" + ref | |
| condition = { | |
| "type": "ONE_OF_RANGE", | |
| "values": [{"userEnteredValue": ref}], | |
| } | |
| action_normalized = (action or "").strip().lower() | |
| normalized_source_range = ( | |
| source_range.strip() if source_range is not None else None | |
| ) | |
| if action_normalized not in ("set", "clear"): | |
| raise UserInputError("action must be 'set' or 'clear'.") | |
| if action_normalized == "set" and bool(values) == bool(normalized_source_range): | |
| raise UserInputError( | |
| "For action='set', provide exactly one of 'values' (a list of dropdown " | |
| "options) or 'source_range' (an A1 range to source the options from)." | |
| ) | |
| metadata = await asyncio.to_thread( | |
| service.spreadsheets() | |
| .get( | |
| spreadsheetId=spreadsheet_id, | |
| fields="sheets(properties(sheetId,title))", | |
| ) | |
| .execute | |
| ) | |
| sheets = metadata.get("sheets", []) | |
| grid_range = _parse_a1_range(range_name, sheets) | |
| if action_normalized == "clear": | |
| # Omitting 'rule' from setDataValidation clears validation on the range. | |
| dv_request = {"range": grid_range} | |
| summary = "cleared data validation" | |
| else: | |
| if values: | |
| condition = { | |
| "type": "ONE_OF_LIST", | |
| "values": [{"userEnteredValue": str(v)} for v in values], | |
| } | |
| summary = f"set a dropdown with {len(values)} option(s)" | |
| else: | |
| ref = normalized_source_range | |
| if ref in ("", "="): | |
| raise UserInputError( | |
| "For action='set', 'source_range' must be a non-empty A1 range." | |
| ) | |
| if not ref.startswith("="): | |
| ref = "=" + ref | |
| condition = { | |
| "type": "ONE_OF_RANGE", | |
| "values": [{"userEnteredValue": ref}], | |
| } |
🤖 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 `@gsheets/sheets_tools.py` around lines 2459 - 2494, The dropdown rule builder
in the set/clear validation flow accepts a whitespace-only source_range and only
fails later when building the ONE_OF_RANGE condition. In the function that
normalizes action_normalized and constructs dv_request/condition, reject blank
source_range values up front (after stripping) with a UserInputError before
calling _parse_a1_range or creating the userEnteredValue. Keep the existing
exclusivity check for values vs source_range, but add a validation path that
treats empty or whitespace-only source_range as invalid input.
Adds a read-only companion to manage_data_validation. read_data_validation inspects a range and reports the data validation rule on each cell that has one: condition type, list options or source range, strict, and whether the dropdown chip is shown. Uses spreadsheets.get with the dataValidation field (sheets read scope only). Includes unit tests.
emilio-S03
changed the title
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.
What
Adds two complementary Google Sheets data-validation (dropdown) tools:
manage_data_validation— set or clear validation on a rangeread_data_validation— read back the validation rule on each cell (read-only)Why
The server can format cells (
format_sheet_range) and manage conditional formatting (manage_conditional_formatting), but there was no way to add a data-validation dropdown, nor to read one back to verify it. These fill that gap and pair naturally: set a dropdown, then confirm it.Behavior
manage_data_validation(write)action="set"+values=[...]→ fixed-list dropdown (ONE_OF_LIST)action="set"+source_range="Lists!A1:A20"→ range-sourced dropdown (ONE_OF_RANGE; a leading=is added if omitted)action="clear"→ removes validation on the rangestrict(reject vs. warn),show_dropdown(the chip /showCustomUi),input_messageread_data_validation(read-only)strict, and whether the dropdown chip is shown.Implementation notes
_impl+ decorated tool,_parse_a1_range/_format_a1_cellhelpers,asyncio.to_thread(...).read_data_validationusesspreadsheets.getwithfields=…dataValidation…(sheets read scope only);manage_data_validationuses the existing sheets write scope. No new OAuth scopes.sheets→completetier incore/tool_tiers.yaml.tests/gsheets/test_manage_data_validation.pyandtests/gsheets/test_read_data_validation.py.Testing
uv run pytest tests/gsheets/test_manage_data_validation.py tests/gsheets/test_read_data_validation.py→ 7 passed.Summary by CodeRabbit
New Features
Bug Fixes
Tests