From 1b324a2055e4ee41d4101c60c7989970d704a17e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 04:01:04 +0000 Subject: [PATCH 1/3] Initial plan From b41b713c823d183e999e05e948b15ef30cd01bec Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 04:12:18 +0000 Subject: [PATCH 2/3] refactor: extract shared project config parsing helpers to eliminate duplication Extract parseProjectViews and parseProjectFieldDefinitions from the duplicated blocks in parseCreateProjectsConfig and parseUpdateProjectConfig into a shared pkg/workflow/project_config_parsing.go file. Both methods now delegate to the helpers, eliminating ~100 lines of duplicated code and ensuring a single source of truth for views/field-definitions parsing. Also adds parity tests in project_config_parsing_test.go covering the helpers directly and verifying identical behavior from both the create and update paths. Fixes #" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/create_project.go | 104 +------ pkg/workflow/project_config_parsing.go | 130 ++++++++ pkg/workflow/project_config_parsing_test.go | 326 ++++++++++++++++++++ pkg/workflow/update_project.go | 104 +------ 4 files changed, 460 insertions(+), 204 deletions(-) create mode 100644 pkg/workflow/project_config_parsing.go create mode 100644 pkg/workflow/project_config_parsing_test.go diff --git a/pkg/workflow/create_project.go b/pkg/workflow/create_project.go index 60924cd724d..7dd4995dda9 100644 --- a/pkg/workflow/create_project.go +++ b/pkg/workflow/create_project.go @@ -50,110 +50,10 @@ func (c *Compiler) parseCreateProjectsConfig(outputMap map[string]any) *CreatePr } // Parse views if specified - if viewsData, exists := configMap["views"]; exists { - if viewsList, ok := viewsData.([]any); ok { - for i, viewItem := range viewsList { - if viewMap, ok := viewItem.(map[string]any); ok { - view := ProjectView{} - - // Parse name (required) - if name, exists := viewMap["name"]; exists { - if nameStr, ok := name.(string); ok { - view.Name = nameStr - } - } - - // Parse layout (required) - if layout, exists := viewMap["layout"]; exists { - if layoutStr, ok := layout.(string); ok { - view.Layout = layoutStr - } - } - - // Parse filter (optional) - if filter, exists := viewMap["filter"]; exists { - if filterStr, ok := filter.(string); ok { - view.Filter = filterStr - } - } - - // Parse visible-fields (optional) - if visibleFields, exists := viewMap["visible-fields"]; exists { - if fieldsList, ok := visibleFields.([]any); ok { - for _, field := range fieldsList { - if fieldInt, ok := field.(int); ok { - view.VisibleFields = append(view.VisibleFields, fieldInt) - } - } - } - } - - // Parse description (optional) - if description, exists := viewMap["description"]; exists { - if descStr, ok := description.(string); ok { - view.Description = descStr - } - } - - // Only add view if it has required fields - if view.Name != "" && view.Layout != "" { - createProjectsConfig.Views = append(createProjectsConfig.Views, view) - createProjectLog.Printf("Parsed view %d: %s (%s)", i+1, view.Name, view.Layout) - } else { - createProjectLog.Printf("Skipping invalid view %d: missing required fields", i+1) - } - } - } - } - } + createProjectsConfig.Views = parseProjectViews(configMap, createProjectLog) // Parse field-definitions if specified - fieldsData, hasFields := configMap["field-definitions"] - if !hasFields { - // Allow underscore variant as well - fieldsData, hasFields = configMap["field_definitions"] - } - if hasFields { - if fieldsList, ok := fieldsData.([]any); ok { - for i, fieldItem := range fieldsList { - fieldMap, ok := fieldItem.(map[string]any) - if !ok { - continue - } - - field := ProjectFieldDefinition{} - - if name, exists := fieldMap["name"]; exists { - if nameStr, ok := name.(string); ok { - field.Name = nameStr - } - } - - dataType, hasDataType := fieldMap["data-type"] - if !hasDataType { - dataType = fieldMap["data_type"] - } - if dataTypeStr, ok := dataType.(string); ok { - field.DataType = dataTypeStr - } - - if options, exists := fieldMap["options"]; exists { - if optionsList, ok := options.([]any); ok { - for _, opt := range optionsList { - if optStr, ok := opt.(string); ok { - field.Options = append(field.Options, optStr) - } - } - } - } - - if field.Name != "" && field.DataType != "" { - createProjectsConfig.FieldDefinitions = append(createProjectsConfig.FieldDefinitions, field) - createProjectLog.Printf("Parsed field definition %d: %s (%s)", i+1, field.Name, field.DataType) - } - } - } - } + createProjectsConfig.FieldDefinitions = parseProjectFieldDefinitions(configMap, createProjectLog) } createProjectLog.Printf("Parsed create-project config: max=%d, hasCustomToken=%v, hasTargetOwner=%v, hasTitlePrefix=%v, viewCount=%d, fieldDefinitionCount=%d", diff --git a/pkg/workflow/project_config_parsing.go b/pkg/workflow/project_config_parsing.go new file mode 100644 index 00000000000..59e70164b55 --- /dev/null +++ b/pkg/workflow/project_config_parsing.go @@ -0,0 +1,130 @@ +package workflow + +import "github.com/github/gh-aw/pkg/logger" + +// parseProjectViews parses the "views" list from a project config map. +// Only views with both name and layout fields are included; invalid ones are skipped. +func parseProjectViews(configMap map[string]any, log *logger.Logger) []ProjectView { + viewsData, exists := configMap["views"] + if !exists { + return nil + } + viewsList, ok := viewsData.([]any) + if !ok { + return nil + } + + var views []ProjectView + for i, viewItem := range viewsList { + viewMap, ok := viewItem.(map[string]any) + if !ok { + continue + } + view := ProjectView{} + + // Parse name (required) + if name, exists := viewMap["name"]; exists { + if nameStr, ok := name.(string); ok { + view.Name = nameStr + } + } + + // Parse layout (required) + if layout, exists := viewMap["layout"]; exists { + if layoutStr, ok := layout.(string); ok { + view.Layout = layoutStr + } + } + + // Parse filter (optional) + if filter, exists := viewMap["filter"]; exists { + if filterStr, ok := filter.(string); ok { + view.Filter = filterStr + } + } + + // Parse visible-fields (optional) + if visibleFields, exists := viewMap["visible-fields"]; exists { + if fieldsList, ok := visibleFields.([]any); ok { + for _, field := range fieldsList { + if fieldInt, ok := field.(int); ok { + view.VisibleFields = append(view.VisibleFields, fieldInt) + } + } + } + } + + // Parse description (optional) + if description, exists := viewMap["description"]; exists { + if descStr, ok := description.(string); ok { + view.Description = descStr + } + } + + // Only add view if it has required fields + if view.Name != "" && view.Layout != "" { + views = append(views, view) + log.Printf("Parsed view %d: %s (%s)", i+1, view.Name, view.Layout) + } else { + log.Printf("Skipping invalid view %d: missing required fields", i+1) + } + } + return views +} + +// parseProjectFieldDefinitions parses the "field-definitions" (or "field_definitions") list +// from a project config map. Only fields with both name and data-type are included. +func parseProjectFieldDefinitions(configMap map[string]any, log *logger.Logger) []ProjectFieldDefinition { + fieldsData, hasFields := configMap["field-definitions"] + if !hasFields { + // Allow underscore variant as well + fieldsData, hasFields = configMap["field_definitions"] + } + if !hasFields { + return nil + } + fieldsList, ok := fieldsData.([]any) + if !ok { + return nil + } + + var fields []ProjectFieldDefinition + for i, fieldItem := range fieldsList { + fieldMap, ok := fieldItem.(map[string]any) + if !ok { + continue + } + + field := ProjectFieldDefinition{} + + if name, exists := fieldMap["name"]; exists { + if nameStr, ok := name.(string); ok { + field.Name = nameStr + } + } + + dataType, hasDataType := fieldMap["data-type"] + if !hasDataType { + dataType = fieldMap["data_type"] + } + if dataTypeStr, ok := dataType.(string); ok { + field.DataType = dataTypeStr + } + + if options, exists := fieldMap["options"]; exists { + if optionsList, ok := options.([]any); ok { + for _, opt := range optionsList { + if optStr, ok := opt.(string); ok { + field.Options = append(field.Options, optStr) + } + } + } + } + + if field.Name != "" && field.DataType != "" { + fields = append(fields, field) + log.Printf("Parsed field definition %d: %s (%s)", i+1, field.Name, field.DataType) + } + } + return fields +} diff --git a/pkg/workflow/project_config_parsing_test.go b/pkg/workflow/project_config_parsing_test.go new file mode 100644 index 00000000000..0cc1d1ff1a9 --- /dev/null +++ b/pkg/workflow/project_config_parsing_test.go @@ -0,0 +1,326 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/github/gh-aw/pkg/logger" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var testLog = logger.New("workflow:test") + +// TestParseProjectViews verifies that parseProjectViews behaves identically +// for the create-project and update-project call paths by using the shared +// helper directly with a neutral logger. +func TestParseProjectViews(t *testing.T) { + tests := []struct { + name string + input map[string]any + expected []ProjectView + }{ + { + name: "no views key", + input: map[string]any{}, + expected: nil, + }, + { + name: "empty views list", + input: map[string]any{ + "views": []any{}, + }, + expected: nil, + }, + { + name: "single valid view with required fields only", + input: map[string]any{ + "views": []any{ + map[string]any{ + "name": "Board", + "layout": "board", + }, + }, + }, + expected: []ProjectView{ + {Name: "Board", Layout: "board"}, + }, + }, + { + name: "view with all optional fields", + input: map[string]any{ + "views": []any{ + map[string]any{ + "name": "Task Board", + "layout": "board", + "filter": "is:issue", + "visible-fields": []any{1, 2, 3}, + "description": "Main board", + }, + }, + }, + expected: []ProjectView{ + { + Name: "Task Board", + Layout: "board", + Filter: "is:issue", + VisibleFields: []int{1, 2, 3}, + Description: "Main board", + }, + }, + }, + { + name: "view missing name is skipped", + input: map[string]any{ + "views": []any{ + map[string]any{"layout": "board"}, + }, + }, + expected: nil, + }, + { + name: "view missing layout is skipped", + input: map[string]any{ + "views": []any{ + map[string]any{"name": "Board"}, + }, + }, + expected: nil, + }, + { + name: "mix of valid and invalid views", + input: map[string]any{ + "views": []any{ + map[string]any{ + "name": "Valid", + "layout": "table", + }, + map[string]any{ + // Missing layout + "name": "Invalid", + }, + map[string]any{ + "name": "Also Valid", + "layout": "roadmap", + }, + }, + }, + expected: []ProjectView{ + {Name: "Valid", Layout: "table"}, + {Name: "Also Valid", Layout: "roadmap"}, + }, + }, + { + name: "non-map view item is skipped", + input: map[string]any{ + "views": []any{ + "not-a-map", + map[string]any{ + "name": "Good", + "layout": "board", + }, + }, + }, + expected: []ProjectView{ + {Name: "Good", Layout: "board"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := parseProjectViews(tt.input, testLog) + assert.Equal(t, tt.expected, result, "parseProjectViews result should match expected") + }) + } +} + +// TestParseProjectFieldDefinitions verifies that parseProjectFieldDefinitions +// behaves identically for both create-project and update-project paths. +func TestParseProjectFieldDefinitions(t *testing.T) { + tests := []struct { + name string + input map[string]any + expected []ProjectFieldDefinition + }{ + { + name: "no field-definitions key", + input: map[string]any{}, + expected: nil, + }, + { + name: "empty field-definitions list", + input: map[string]any{ + "field-definitions": []any{}, + }, + expected: nil, + }, + { + name: "single TEXT field using hyphen key", + input: map[string]any{ + "field-definitions": []any{ + map[string]any{ + "name": "Tracking Id", + "data-type": "TEXT", + }, + }, + }, + expected: []ProjectFieldDefinition{ + {Name: "Tracking Id", DataType: "TEXT"}, + }, + }, + { + name: "single TEXT field using underscore key variant", + input: map[string]any{ + "field_definitions": []any{ + map[string]any{ + "name": "Worker Workflow", + "data_type": "TEXT", + }, + }, + }, + expected: []ProjectFieldDefinition{ + {Name: "Worker Workflow", DataType: "TEXT"}, + }, + }, + { + name: "SINGLE_SELECT field with options", + input: map[string]any{ + "field-definitions": []any{ + map[string]any{ + "name": "Priority", + "data-type": "SINGLE_SELECT", + "options": []any{"High", "Medium", "Low"}, + }, + }, + }, + expected: []ProjectFieldDefinition{ + {Name: "Priority", DataType: "SINGLE_SELECT", Options: []string{"High", "Medium", "Low"}}, + }, + }, + { + name: "field missing name is skipped", + input: map[string]any{ + "field-definitions": []any{ + map[string]any{"data-type": "TEXT"}, + }, + }, + expected: nil, + }, + { + name: "field missing data-type is skipped", + input: map[string]any{ + "field-definitions": []any{ + map[string]any{"name": "MyField"}, + }, + }, + expected: nil, + }, + { + name: "non-map field item is skipped", + input: map[string]any{ + "field-definitions": []any{ + "not-a-map", + map[string]any{ + "name": "Good", + "data-type": "DATE", + }, + }, + }, + expected: []ProjectFieldDefinition{ + {Name: "Good", DataType: "DATE"}, + }, + }, + { + name: "multiple valid fields", + input: map[string]any{ + "field-definitions": []any{ + map[string]any{ + "name": "Tracking Id", + "data-type": "TEXT", + }, + map[string]any{ + "name": "Priority", + "data-type": "SINGLE_SELECT", + "options": []any{"High", "Medium", "Low"}, + }, + map[string]any{ + "name": "Start Date", + "data-type": "DATE", + }, + }, + }, + expected: []ProjectFieldDefinition{ + {Name: "Tracking Id", DataType: "TEXT"}, + {Name: "Priority", DataType: "SINGLE_SELECT", Options: []string{"High", "Medium", "Low"}}, + {Name: "Start Date", DataType: "DATE"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := parseProjectFieldDefinitions(tt.input, testLog) + assert.Equal(t, tt.expected, result, "parseProjectFieldDefinitions result should match expected") + }) + } +} + +// TestParseProjectViewsAndFieldDefinitionsParity verifies that create-project +// and update-project both use the shared helpers and produce identical results +// for the same views/field-definitions input. +func TestParseProjectViewsAndFieldDefinitionsParity(t *testing.T) { + viewsInput := []any{ + map[string]any{ + "name": "Task Board", + "layout": "board", + "filter": "is:open", + "visible-fields": []any{1, 2}, + "description": "Main board", + }, + } + fieldsInput := []any{ + map[string]any{ + "name": "Size", + "data-type": "SINGLE_SELECT", + "options": []any{"S", "M", "L"}, + }, + } + + configMap := map[string]any{ + "views": viewsInput, + "field-definitions": fieldsInput, + } + + compiler := NewCompiler() + + createOutputMap := map[string]any{ + "create-project": map[string]any{ + "views": viewsInput, + "field-definitions": fieldsInput, + }, + } + updateOutputMap := map[string]any{ + "update-project": map[string]any{ + "views": viewsInput, + "field-definitions": fieldsInput, + }, + } + + createConfig := compiler.parseCreateProjectsConfig(createOutputMap) + updateConfig := compiler.parseUpdateProjectConfig(updateOutputMap) + + require.NotNil(t, createConfig, "create config should not be nil") + require.NotNil(t, updateConfig, "update config should not be nil") + + // Views must be identical from both paths + expectedViews := parseProjectViews(configMap, testLog) + assert.Equal(t, expectedViews, createConfig.Views, "create-project views should match shared helper output") + assert.Equal(t, expectedViews, updateConfig.Views, "update-project views should match shared helper output") + + // Field definitions must be identical from both paths + expectedFields := parseProjectFieldDefinitions(configMap, testLog) + assert.Equal(t, expectedFields, createConfig.FieldDefinitions, "create-project field definitions should match shared helper output") + assert.Equal(t, expectedFields, updateConfig.FieldDefinitions, "update-project field definitions should match shared helper output") +} diff --git a/pkg/workflow/update_project.go b/pkg/workflow/update_project.go index 541edef7ba8..3efc4cf111a 100644 --- a/pkg/workflow/update_project.go +++ b/pkg/workflow/update_project.go @@ -58,110 +58,10 @@ func (c *Compiler) parseUpdateProjectConfig(outputMap map[string]any) *UpdatePro } // Parse views if specified - if viewsData, exists := configMap["views"]; exists { - if viewsList, ok := viewsData.([]any); ok { - for i, viewItem := range viewsList { - if viewMap, ok := viewItem.(map[string]any); ok { - view := ProjectView{} - - // Parse name (required) - if name, exists := viewMap["name"]; exists { - if nameStr, ok := name.(string); ok { - view.Name = nameStr - } - } - - // Parse layout (required) - if layout, exists := viewMap["layout"]; exists { - if layoutStr, ok := layout.(string); ok { - view.Layout = layoutStr - } - } - - // Parse filter (optional) - if filter, exists := viewMap["filter"]; exists { - if filterStr, ok := filter.(string); ok { - view.Filter = filterStr - } - } - - // Parse visible-fields (optional) - if visibleFields, exists := viewMap["visible-fields"]; exists { - if fieldsList, ok := visibleFields.([]any); ok { - for _, field := range fieldsList { - if fieldInt, ok := field.(int); ok { - view.VisibleFields = append(view.VisibleFields, fieldInt) - } - } - } - } - - // Parse description (optional) - if description, exists := viewMap["description"]; exists { - if descStr, ok := description.(string); ok { - view.Description = descStr - } - } - - // Only add view if it has required fields - if view.Name != "" && view.Layout != "" { - updateProjectConfig.Views = append(updateProjectConfig.Views, view) - updateProjectLog.Printf("Parsed view %d: %s (%s)", i+1, view.Name, view.Layout) - } else { - updateProjectLog.Printf("Skipping invalid view %d: missing required fields", i+1) - } - } - } - } - } + updateProjectConfig.Views = parseProjectViews(configMap, updateProjectLog) // Parse field-definitions if specified - fieldsData, hasFields := configMap["field-definitions"] - if !hasFields { - // Allow underscore variant as well - fieldsData, hasFields = configMap["field_definitions"] - } - if hasFields { - if fieldsList, ok := fieldsData.([]any); ok { - for i, fieldItem := range fieldsList { - fieldMap, ok := fieldItem.(map[string]any) - if !ok { - continue - } - - field := ProjectFieldDefinition{} - - if name, exists := fieldMap["name"]; exists { - if nameStr, ok := name.(string); ok { - field.Name = nameStr - } - } - - dataType, hasDataType := fieldMap["data-type"] - if !hasDataType { - dataType = fieldMap["data_type"] - } - if dataTypeStr, ok := dataType.(string); ok { - field.DataType = dataTypeStr - } - - if options, exists := fieldMap["options"]; exists { - if optionsList, ok := options.([]any); ok { - for _, opt := range optionsList { - if optStr, ok := opt.(string); ok { - field.Options = append(field.Options, optStr) - } - } - } - } - - if field.Name != "" && field.DataType != "" { - updateProjectConfig.FieldDefinitions = append(updateProjectConfig.FieldDefinitions, field) - updateProjectLog.Printf("Parsed field definition %d: %s (%s)", i+1, field.Name, field.DataType) - } - } - } - } + updateProjectConfig.FieldDefinitions = parseProjectFieldDefinitions(configMap, updateProjectLog) } updateProjectLog.Printf("Parsed update-project config: max=%d, hasCustomToken=%v, hasCustomProject=%v, viewCount=%d, fieldDefinitionCount=%d", From c398e7ceb991577f6ea62476901e5ee4e160adfe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 14:32:01 +0000 Subject: [PATCH 3/3] test: address review comments on project_config_parsing_test.go - Add wrong container type cases for views and field-definitions (string/map instead of []any) to lock in nil-early-return behavior - Add mixed-type visible-fields and options cases to document that non-int/non-string elements are silently skipped - Use assert.Empty for nil-expected cases to avoid brittle nil-vs-empty-slice comparisons Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/project_config_parsing_test.go | 70 ++++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/project_config_parsing_test.go b/pkg/workflow/project_config_parsing_test.go index 0cc1d1ff1a9..205c081e636 100644 --- a/pkg/workflow/project_config_parsing_test.go +++ b/pkg/workflow/project_config_parsing_test.go @@ -33,6 +33,20 @@ func TestParseProjectViews(t *testing.T) { }, expected: nil, }, + { + name: "views value is wrong type (string)", + input: map[string]any{ + "views": "not-a-list", + }, + expected: nil, + }, + { + name: "views value is wrong type (map)", + input: map[string]any{ + "views": map[string]any{"name": "Board", "layout": "board"}, + }, + expected: nil, + }, { name: "single valid view with required fields only", input: map[string]any{ @@ -70,6 +84,21 @@ func TestParseProjectViews(t *testing.T) { }, }, }, + { + name: "visible-fields with mixed types: non-int elements are ignored", + input: map[string]any{ + "views": []any{ + map[string]any{ + "name": "Mixed", + "layout": "table", + "visible-fields": []any{1, "bad", 3, nil}, + }, + }, + }, + expected: []ProjectView{ + {Name: "Mixed", Layout: "table", VisibleFields: []int{1, 3}}, + }, + }, { name: "view missing name is skipped", input: map[string]any{ @@ -131,7 +160,11 @@ func TestParseProjectViews(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := parseProjectViews(tt.input, testLog) - assert.Equal(t, tt.expected, result, "parseProjectViews result should match expected") + if tt.expected == nil { + assert.Empty(t, result, "parseProjectViews result should be empty") + } else { + assert.Equal(t, tt.expected, result, "parseProjectViews result should match expected") + } }) } } @@ -156,6 +189,20 @@ func TestParseProjectFieldDefinitions(t *testing.T) { }, expected: nil, }, + { + name: "field-definitions value is wrong type (string)", + input: map[string]any{ + "field-definitions": "not-a-list", + }, + expected: nil, + }, + { + name: "field-definitions value is wrong type (map)", + input: map[string]any{ + "field-definitions": map[string]any{"name": "Foo", "data-type": "TEXT"}, + }, + expected: nil, + }, { name: "single TEXT field using hyphen key", input: map[string]any{ @@ -199,6 +246,21 @@ func TestParseProjectFieldDefinitions(t *testing.T) { {Name: "Priority", DataType: "SINGLE_SELECT", Options: []string{"High", "Medium", "Low"}}, }, }, + { + name: "options with mixed types: non-string elements are ignored", + input: map[string]any{ + "field-definitions": []any{ + map[string]any{ + "name": "Size", + "data-type": "SINGLE_SELECT", + "options": []any{"S", 42, "L", nil}, + }, + }, + }, + expected: []ProjectFieldDefinition{ + {Name: "Size", DataType: "SINGLE_SELECT", Options: []string{"S", "L"}}, + }, + }, { name: "field missing name is skipped", input: map[string]any{ @@ -262,7 +324,11 @@ func TestParseProjectFieldDefinitions(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := parseProjectFieldDefinitions(tt.input, testLog) - assert.Equal(t, tt.expected, result, "parseProjectFieldDefinitions result should match expected") + if tt.expected == nil { + assert.Empty(t, result, "parseProjectFieldDefinitions result should be empty") + } else { + assert.Equal(t, tt.expected, result, "parseProjectFieldDefinitions result should match expected") + } }) } }