Skip to content

Commit c9643b4

Browse files
authored
Fix #167: Excel validation InputTitle/InputMessage not returned + dropdown list guidance (#168)
Root Cause: - Excel COM API requires validation.ShowInput=true BEFORE setting InputTitle/InputMessage - Otherwise Excel silently ignores these properties Fix Applied: - Reordered property assignments in ValidateRangeAsync (lines 67-72) - Refactored GetValidationAsync to read properties into variables first (defensive) Critical Discovery - List Validation Dropdowns: - Comma-separated strings do NOT create dropdown arrows in Excel - Only worksheet range references create actual dropdowns - Updated documentation to guide users: write values to range first, then reference it Documentation Updates: - ExcelRangeTool.cs: Updated formula1 parameter description for list validation - data_validation.md: Added step-by-step dropdown workflow with warning - excel_range.md: Added to common mistakes list Tests Added: - ValidateRange_WithInputMessage_ReturnsSuccess: End-to-end validation workflow - GetValidation_WithInputMessage_ReturnsInputTitleAndMessage: InputTitle/InputMessage verification - Both tests use proper dropdown syntax (worksheet range reference) - Tests pass (15s total, 0 warnings) User Impact: - InputTitle and InputMessage now work correctly - Clear guidance on creating proper dropdown lists - Prevents confusion about comma-separated vs range-based validation
1 parent 8665b97 commit c9643b4

File tree

5 files changed

+178
-16
lines changed

5 files changed

+178
-16
lines changed

src/ExcelMcp.Core/Commands/Range/RangeCommands.Validation.cs

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,9 @@ public async Task<OperationResult> ValidateRangeAsync(
6666
// Configure input message
6767
if (showInputMessage == true)
6868
{
69+
validation.ShowInput = true; // MUST set ShowInput=true BEFORE setting title/message
6970
validation.InputTitle = inputTitle ?? "";
7071
validation.InputMessage = inputMessage ?? "";
71-
validation.ShowInput = true;
7272
}
7373

7474
// Configure error alert
@@ -205,25 +205,40 @@ public async Task<RangeValidationResult> GetValidationAsync(
205205
};
206206
}
207207

208+
// Read all validation properties into local variables first
209+
// This ensures we're not affected by any COM state changes during object initialization
210+
var validationType = GetValidationTypeName(validation.Type);
211+
var validationOperator = GetValidationOperatorName(validation.Operator);
212+
var formula1 = validation.Formula1?.ToString() ?? string.Empty;
213+
var formula2 = validation.Formula2?.ToString() ?? string.Empty;
214+
var ignoreBlank = validation.IgnoreBlank ?? true;
215+
var showInputMessage = validation.ShowInput ?? false;
216+
var inputTitle = validation.InputTitle?.ToString() ?? string.Empty;
217+
var inputMessage = validation.InputMessage?.ToString() ?? string.Empty;
218+
var showErrorAlert = validation.ShowError ?? true;
219+
var errorStyle = GetErrorStyleName(validation.AlertStyle);
220+
var errorTitle = validation.ErrorTitle?.ToString() ?? string.Empty;
221+
var validationErrorMessage = validation.ErrorMessage?.ToString() ?? string.Empty;
222+
208223
return new RangeValidationResult
209224
{
210225
Success = true,
211226
FilePath = batch.WorkbookPath,
212227
SheetName = sheetName,
213228
RangeAddress = rangeAddress,
214229
HasValidation = true,
215-
ValidationType = GetValidationTypeName(validation.Type),
216-
ValidationOperator = GetValidationOperatorName(validation.Operator),
217-
Formula1 = validation.Formula1?.ToString() ?? string.Empty,
218-
Formula2 = validation.Formula2?.ToString() ?? string.Empty,
219-
IgnoreBlank = validation.IgnoreBlank ?? true,
220-
ShowInputMessage = validation.ShowInput ?? false,
221-
InputTitle = validation.InputTitle?.ToString() ?? string.Empty,
222-
InputMessage = validation.InputMessage?.ToString() ?? string.Empty,
223-
ShowErrorAlert = validation.ShowError ?? true,
224-
ErrorStyle = GetErrorStyleName(validation.AlertStyle),
225-
ErrorTitle = validation.ErrorTitle?.ToString() ?? string.Empty,
226-
ValidationErrorMessage = validation.ErrorMessage?.ToString() ?? string.Empty
230+
ValidationType = validationType,
231+
ValidationOperator = validationOperator,
232+
Formula1 = formula1,
233+
Formula2 = formula2,
234+
IgnoreBlank = ignoreBlank,
235+
ShowInputMessage = showInputMessage,
236+
InputTitle = inputTitle,
237+
InputMessage = inputMessage,
238+
ShowErrorAlert = showErrorAlert,
239+
ErrorStyle = errorStyle,
240+
ErrorTitle = errorTitle,
241+
ValidationErrorMessage = validationErrorMessage
227242
};
228243
}
229244
catch (Exception ex)

src/ExcelMcp.McpServer/Prompts/Content/Elicitations/data_validation.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,14 @@ REQUIRED:
1515

1616
TYPE-SPECIFIC INFO:
1717

18-
FOR LIST VALIDATION:
19-
☐ List values (comma-separated, e.g., 'Active,Inactive,Pending')
18+
FOR LIST VALIDATION (creates dropdown):
19+
☐ List values (e.g., ['Active', 'Inactive', 'Pending'])
2020
☐ Show dropdown? (true recommended)
21+
⚠️ WORKFLOW:
22+
1. First write values to a worksheet range (e.g., Sheet1!$Z$1:$Z$10)
23+
2. Then set formula1 to reference that range (e.g., '=$Z$1:$Z$10')
24+
3. This creates a proper dropdown that users can select from
25+
☐ Note: Comma-separated strings do NOT create dropdowns - only range references do!
2126

2227
FOR NUMBER VALIDATION (decimal/whole):
2328
☐ Operator (between, greaterThan, lessThan, greaterThanOrEqual, lessThanOrEqual, equal, notEqual)

src/ExcelMcp.McpServer/Prompts/Content/excel_range.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
- Using sheetName for named ranges → Use sheetName="" for named ranges
4444
- Not using batch mode for multiple operations → 75-90% slower
4545
- Setting number format per cell → Use set-number-format for entire range instead
46+
- **List validation with comma-separated values → Only range references create dropdowns! Write values to range first, then reference it.**
4647

4748
**Workflow optimization**:
4849
- Multiple range operations? Use begin_excel_batch first

src/ExcelMcp.McpServer/Tools/ExcelRangeTool.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ public static async Task<string> ExcelRange(
183183
[Description("Data validation operator (for validate-range: between, notBetween, equal, notEqual, greaterThan, lessThan, greaterThanOrEqual, lessThanOrEqual)")]
184184
string? validationOperator = null,
185185

186-
[Description("Validation formula1 (for validate-range, first value/formula)")]
186+
[Description("Validation formula1 (for validate-range). For 'list' type: MUST be worksheet range reference like '=$A$1:$A$10' to create dropdown. For other types: value/formula for comparison.")]
187187
string? validationFormula1 = null,
188188

189189
[Description("Validation formula2 (for validate-range, second value/formula for between/notBetween)")]
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
// Copyright (c) Stefan Broenne. All rights reserved.
2+
3+
using Sbroenne.ExcelMcp.ComInterop.Session;
4+
using Sbroenne.ExcelMcp.Core.Tests.Helpers;
5+
using Xunit;
6+
7+
namespace Sbroenne.ExcelMcp.Core.Tests.Commands.Range;
8+
9+
public partial class RangeCommandsTests
10+
{
11+
[Fact]
12+
public async Task ValidateRange_WithInputMessage_ReturnsSuccess()
13+
{
14+
// Arrange
15+
var testFile = await CoreTestHelper.CreateUniqueTestFileAsync(
16+
nameof(RangeCommandsTests),
17+
nameof(ValidateRange_WithInputMessage_ReturnsSuccess),
18+
_tempDir);
19+
20+
// Act - First write list values to worksheet (required for dropdown)
21+
await using var batch = await ExcelSession.BeginBatchAsync(testFile);
22+
23+
await _commands.SetValuesAsync(
24+
batch,
25+
"Sheet1",
26+
"B1:B3",
27+
new List<List<object?>>
28+
{
29+
new() { "Option1" },
30+
new() { "Option2" },
31+
new() { "Option3" }
32+
});
33+
34+
// Apply validation referencing the range (creates dropdown)
35+
var applyResult = await _commands.ValidateRangeAsync(
36+
batch,
37+
"Sheet1",
38+
"A1",
39+
validationType: "list",
40+
validationOperator: null,
41+
formula1: "=$B$1:$B$3", // Reference to worksheet range creates dropdown
42+
formula2: null,
43+
showInputMessage: true,
44+
inputTitle: "My Input Title",
45+
inputMessage: "My helpful input message",
46+
showErrorAlert: true,
47+
errorStyle: "stop",
48+
errorTitle: "My Error Title",
49+
errorMessage: "My error message",
50+
ignoreBlank: true,
51+
showDropdown: true);
52+
53+
// Assert - Validation applied successfully
54+
Assert.True(applyResult.Success, $"Apply validation failed: {applyResult.ErrorMessage}");
55+
56+
// Verify validation is retrieved correctly (same batch)
57+
var getResult = await _commands.GetValidationAsync(batch, "Sheet1", "A1");
58+
59+
// Assert - Validation retrieved successfully
60+
Assert.True(getResult.Success, $"Get validation failed: {getResult.ErrorMessage}");
61+
Assert.True(getResult.HasValidation, "Range should have validation");
62+
63+
// Assert - Validation type and formula are correct
64+
Assert.Equal("list", getResult.ValidationType);
65+
Assert.Equal("=$B$1:$B$3", getResult.Formula1);
66+
67+
// Assert - Input message properties are returned
68+
Assert.Equal("My Input Title", getResult.InputTitle);
69+
Assert.Equal("My helpful input message", getResult.InputMessage);
70+
71+
// Assert - Error message properties are returned (these work according to the bug report)
72+
Assert.Equal("My Error Title", getResult.ErrorTitle);
73+
Assert.Equal("My error message", getResult.ValidationErrorMessage);
74+
}
75+
76+
[Fact]
77+
public async Task GetValidation_WithInputMessage_ReturnsInputTitleAndMessage()
78+
{
79+
// Arrange
80+
var testFile = await CoreTestHelper.CreateUniqueTestFileAsync(
81+
nameof(RangeCommandsTests),
82+
nameof(GetValidation_WithInputMessage_ReturnsInputTitleAndMessage),
83+
_tempDir);
84+
85+
// Act - First write list values to worksheet (required for dropdown)
86+
await using var batch = await ExcelSession.BeginBatchAsync(testFile);
87+
88+
await _commands.SetValuesAsync(
89+
batch,
90+
"Sheet1",
91+
"B1:B3",
92+
new List<List<object?>>
93+
{
94+
new() { "Option1" },
95+
new() { "Option2" },
96+
new() { "Option3" }
97+
});
98+
99+
// Apply validation using the ValidateRangeAsync API
100+
var validateResult = await _commands.ValidateRangeAsync(
101+
batch,
102+
"Sheet1",
103+
"A1",
104+
validationType: "list",
105+
validationOperator: null, // Not used for list type
106+
formula1: "=$B$1:$B$3", // Reference to worksheet range creates dropdown
107+
formula2: null,
108+
showInputMessage: true,
109+
inputTitle: "My Input Title",
110+
inputMessage: "My helpful input message",
111+
showErrorAlert: true,
112+
errorStyle: "stop",
113+
errorTitle: "My Error Title",
114+
errorMessage: "My error message",
115+
ignoreBlank: true,
116+
showDropdown: true);
117+
118+
Assert.True(validateResult.Success, $"Validate failed: {validateResult.ErrorMessage}");
119+
120+
// Act - Get validation to verify InputTitle/InputMessage are returned
121+
var result = await _commands.GetValidationAsync(batch, "Sheet1", "A1");
122+
123+
// Assert
124+
Assert.True(result.Success, $"Get validation failed: {result.ErrorMessage}");
125+
Assert.True(result.HasValidation, "Range should have validation");
126+
127+
// Assert - Validation type and formula create dropdown with 3 values
128+
Assert.Equal("list", result.ValidationType);
129+
Assert.Equal("=$B$1:$B$3", result.Formula1);
130+
131+
// CRITICAL: These assertions test the bug fix
132+
Assert.NotEmpty(result.InputTitle ?? string.Empty);
133+
Assert.Equal("My Input Title", result.InputTitle);
134+
Assert.NotEmpty(result.InputMessage ?? string.Empty);
135+
Assert.Equal("My helpful input message", result.InputMessage);
136+
137+
// These should work (error properties worked before the fix)
138+
Assert.Equal("My Error Title", result.ErrorTitle);
139+
Assert.Equal("My error message", result.ValidationErrorMessage);
140+
}
141+
}

0 commit comments

Comments
 (0)