diff --git a/cmd/generate_changelog/incoming/1860.txt b/cmd/generate_changelog/incoming/1860.txt new file mode 100644 index 00000000..bb410ef5 --- /dev/null +++ b/cmd/generate_changelog/incoming/1860.txt @@ -0,0 +1,7 @@ +### PR [#1860](https://github.com/danielmiessler/Fabric/pull/1860) by [ksylvan](https://github.com/ksylvan): fix: allow resetting required settings without validation errors + +- Fix: allow resetting required settings without validation errors +- Update `Ask` to detect reset command and bypass validation +- Refactor `OnAnswer` to support new `isReset` parameter logic +- Invoke `ConfigureCustom` in `Setup` to avoid redundant re-validation +- Add unit tests ensuring required fields can be reset diff --git a/internal/plugins/plugin.go b/internal/plugins/plugin.go index 2561f9a8..62717ca4 100644 --- a/internal/plugins/plugin.go +++ b/internal/plugins/plugin.go @@ -92,7 +92,11 @@ func (o *PluginBase) Setup() (err error) { return } - err = o.Configure() + // After Setup, run ConfigureCustom if present, but skip re-validation + // since Ask() already validated user input (or allowed explicit reset) + if o.ConfigureCustom != nil { + err = o.ConfigureCustom() + } return } @@ -198,16 +202,21 @@ func (o *SetupQuestion) Ask(label string) (err error) { var answer string fmt.Scanln(&answer) answer = strings.TrimRight(answer, "\n") + isReset := strings.ToLower(answer) == AnswerReset if answer == "" { answer = o.Value - } else if strings.ToLower(answer) == AnswerReset { + } else if isReset { answer = "" } - err = o.OnAnswer(answer) + err = o.OnAnswerWithReset(answer, isReset) return } func (o *SetupQuestion) OnAnswer(answer string) (err error) { + return o.OnAnswerWithReset(answer, false) +} + +func (o *SetupQuestion) OnAnswerWithReset(answer string, isReset bool) (err error) { if o.Type == SettingTypeBool { if answer == "" { o.Value = "" @@ -226,6 +235,11 @@ func (o *SetupQuestion) OnAnswer(answer string) (err error) { return } } + // Skip validation when explicitly resetting a value - the user intentionally + // wants to clear the value even if it's required + if isReset { + return nil + } err = o.IsValidErr() return } diff --git a/internal/plugins/plugin_test.go b/internal/plugins/plugin_test.go index 62f3ed89..1abae83c 100644 --- a/internal/plugins/plugin_test.go +++ b/internal/plugins/plugin_test.go @@ -116,6 +116,91 @@ func TestSetupQuestion_Ask(t *testing.T) { assert.Equal(t, "user_value", setting.Value) } +func TestSetupQuestion_Ask_Reset(t *testing.T) { + // Test that resetting a required field doesn't produce an error + setting := &Setting{ + EnvVariable: "TEST_RESET_SETTING", + Value: "existing_value", + Required: true, + } + question := &SetupQuestion{ + Setting: setting, + Question: "Enter test setting:", + } + input := "reset\n" + fmtInput := captureInput(input) + defer fmtInput() + err := question.Ask("TestConfigurable") + // Should NOT return an error even though the field is required + assert.NoError(t, err) + // Value should be cleared + assert.Equal(t, "", setting.Value) +} + +func TestSetupQuestion_OnAnswerWithReset(t *testing.T) { + tests := []struct { + name string + setting *Setting + answer string + isReset bool + expectError bool + expectValue string + }{ + { + name: "reset required field should not error", + setting: &Setting{ + EnvVariable: "TEST_SETTING", + Value: "old_value", + Required: true, + }, + answer: "", + isReset: true, + expectError: false, + expectValue: "", + }, + { + name: "empty answer on required field should error", + setting: &Setting{ + EnvVariable: "TEST_SETTING", + Value: "", + Required: true, + }, + answer: "", + isReset: false, + expectError: true, + expectValue: "", + }, + { + name: "valid answer on required field should not error", + setting: &Setting{ + EnvVariable: "TEST_SETTING", + Value: "", + Required: true, + }, + answer: "new_value", + isReset: false, + expectError: false, + expectValue: "new_value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + question := &SetupQuestion{ + Setting: tt.setting, + Question: "Test question", + } + err := question.OnAnswerWithReset(tt.answer, tt.isReset) + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.expectValue, tt.setting.Value) + }) + } +} + func TestSettings_IsConfigured(t *testing.T) { settings := Settings{ {EnvVariable: "TEST_SETTING1", Value: "value1", Required: true},