diff --git a/actions/setup/js/create_discussion.cjs b/actions/setup/js/create_discussion.cjs index 07bea7b435..083a702c93 100644 --- a/actions/setup/js/create_discussion.cjs +++ b/actions/setup/js/create_discussion.cjs @@ -63,18 +63,22 @@ function resolveCategoryId(categoryConfig, itemCategory, categories) { const categoryToMatch = itemCategory || categoryConfig; if (categoryToMatch) { - // Try to match against category IDs first + // Try to match against category IDs first (exact match, case-sensitive) const categoryById = categories.find(cat => cat.id === categoryToMatch); if (categoryById) { return { id: categoryById.id, matchType: "id", name: categoryById.name }; } - // Try to match against category names - const categoryByName = categories.find(cat => cat.name === categoryToMatch); + + // Normalize the category to match for case-insensitive comparison + const normalizedCategoryToMatch = categoryToMatch.toLowerCase(); + + // Try to match against category names (case-insensitive) + const categoryByName = categories.find(cat => cat.name.toLowerCase() === normalizedCategoryToMatch); if (categoryByName) { return { id: categoryByName.id, matchType: "name", name: categoryByName.name }; } - // Try to match against category slugs (routes) - const categoryBySlug = categories.find(cat => cat.slug === categoryToMatch); + // Try to match against category slugs (routes, case-insensitive) + const categoryBySlug = categories.find(cat => cat.slug.toLowerCase() === normalizedCategoryToMatch); if (categoryBySlug) { return { id: categoryBySlug.id, matchType: "slug", name: categoryBySlug.name }; } diff --git a/actions/setup/js/create_discussion_category_normalization.test.cjs b/actions/setup/js/create_discussion_category_normalization.test.cjs new file mode 100644 index 0000000000..ed5e0abf30 --- /dev/null +++ b/actions/setup/js/create_discussion_category_normalization.test.cjs @@ -0,0 +1,274 @@ +// @ts-check +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { createRequire } from "module"; + +const require = createRequire(import.meta.url); +const { main: createDiscussionMain } = require("./create_discussion.cjs"); + +describe("create_discussion category normalization", () => { + let mockGithub; + let mockCore; + let mockContext; + let mockExec; + let originalEnv; + + beforeEach(() => { + // Save original environment + originalEnv = { ...process.env }; + + // Mock GitHub API with discussion categories + mockGithub = { + rest: {}, + graphql: vi.fn().mockImplementation((query, variables) => { + // Handle repository query (fetch categories) + if (query.includes("discussionCategories")) { + return Promise.resolve({ + repository: { + id: "R_test123", + discussionCategories: { + nodes: [ + { + id: "DIC_kwDOGFsHUM4BsUn1", + name: "General", + slug: "general", + description: "General discussions", + }, + { + id: "DIC_kwDOGFsHUM4BsUn2", + name: "Audits", + slug: "audits", + description: "Audit reports", + }, + { + id: "DIC_kwDOGFsHUM4BsUn3", + name: "Research", + slug: "research", + description: "Research discussions", + }, + ], + }, + }, + }); + } + // Handle create discussion mutation + if (query.includes("createDiscussion")) { + return Promise.resolve({ + createDiscussion: { + discussion: { + id: "D_test456", + number: 42, + title: variables.title, + url: "https://github.com/test-owner/test-repo/discussions/42", + }, + }, + }); + } + return Promise.reject(new Error("Unknown GraphQL query")); + }), + }; + + // Mock Core + mockCore = { + info: vi.fn(), + warning: vi.fn(), + error: vi.fn(), + setOutput: vi.fn(), + }; + + // Mock Context + mockContext = { + repo: { owner: "test-owner", repo: "test-repo" }, + runId: 12345, + payload: { + repository: { + html_url: "https://github.com/test-owner/test-repo", + }, + }, + }; + + // Mock Exec + mockExec = { + exec: vi.fn().mockResolvedValue(0), + }; + + // Set globals + global.github = mockGithub; + global.core = mockCore; + global.context = mockContext; + global.exec = mockExec; + + // Set required environment variables + process.env.GH_AW_WORKFLOW_NAME = "Test Workflow"; + process.env.GH_AW_WORKFLOW_ID = "test-workflow"; + process.env.GH_AW_WORKFLOW_SOURCE_URL = "https://github.com/owner/repo/blob/main/workflow.md"; + process.env.GITHUB_SERVER_URL = "https://github.com"; + }); + + afterEach(() => { + // Restore environment + process.env = originalEnv; + vi.clearAllMocks(); + }); + + it("should match category name case-insensitively (lowercase config, capitalized repo)", async () => { + const handler = await createDiscussionMain({ + max: 5, + category: "audits", // lowercase config + }); + + const result = await handler( + { + title: "Test Discussion", + body: "This is a test discussion.", + }, + {} + ); + + expect(result.success).toBe(true); + expect(result.number).toBe(42); + + // Verify the correct category ID was used (Audits with capital A) + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + expect(createMutationCall[1].categoryId).toBe("DIC_kwDOGFsHUM4BsUn2"); // Audits category + }); + + it("should match category name case-insensitively (capitalized config, capitalized repo)", async () => { + const handler = await createDiscussionMain({ + max: 5, + category: "Audits", // Capitalized config (user error) + }); + + const result = await handler( + { + title: "Test Discussion", + body: "This is a test discussion.", + }, + {} + ); + + expect(result.success).toBe(true); + expect(result.number).toBe(42); + + // Verify the correct category ID was used + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + expect(createMutationCall[1].categoryId).toBe("DIC_kwDOGFsHUM4BsUn2"); // Audits category + }); + + it("should match category name case-insensitively (mixed case config)", async () => { + const handler = await createDiscussionMain({ + max: 5, + category: "AuDiTs", // Mixed case (should still match) + }); + + const result = await handler( + { + title: "Test Discussion", + body: "This is a test discussion.", + }, + {} + ); + + expect(result.success).toBe(true); + expect(result.number).toBe(42); + + // Verify the correct category ID was used + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + expect(createMutationCall[1].categoryId).toBe("DIC_kwDOGFsHUM4BsUn2"); // Audits category + }); + + it("should match category slug case-insensitively", async () => { + const handler = await createDiscussionMain({ + max: 5, + category: "RESEARCH", // Uppercase slug + }); + + const result = await handler( + { + title: "Test Discussion", + body: "This is a test discussion.", + }, + {} + ); + + expect(result.success).toBe(true); + expect(result.number).toBe(42); + + // Verify the correct category ID was used (Research) + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + expect(createMutationCall[1].categoryId).toBe("DIC_kwDOGFsHUM4BsUn3"); // Research category + }); + + it("should preserve category IDs (exact match, case-sensitive)", async () => { + const handler = await createDiscussionMain({ + max: 5, + category: "DIC_kwDOGFsHUM4BsUn3", // Direct category ID + }); + + const result = await handler( + { + title: "Test Discussion", + body: "This is a test discussion.", + }, + {} + ); + + expect(result.success).toBe(true); + expect(result.number).toBe(42); + + // Verify the exact category ID was used + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + expect(createMutationCall[1].categoryId).toBe("DIC_kwDOGFsHUM4BsUn3"); + }); + + it("should use item category over config category (case-insensitive)", async () => { + const handler = await createDiscussionMain({ + max: 5, + category: "general", // Config says general + }); + + const result = await handler( + { + title: "Test Discussion", + body: "This is a test discussion.", + category: "AUDITS", // Item overrides with uppercase + }, + {} + ); + + expect(result.success).toBe(true); + expect(result.number).toBe(42); + + // Verify Audits category was used (from item, not config) + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + expect(createMutationCall[1].categoryId).toBe("DIC_kwDOGFsHUM4BsUn2"); // Audits category + }); + + it("should fallback to first category when no match found", async () => { + const handler = await createDiscussionMain({ + max: 5, + category: "NonExistentCategory", + }); + + const result = await handler( + { + title: "Test Discussion", + body: "This is a test discussion.", + }, + {} + ); + + expect(result.success).toBe(true); + expect(result.number).toBe(42); + + // Verify fallback to first category (General) + const createMutationCall = mockGithub.graphql.mock.calls.find(call => call[0].includes("createDiscussion")); + expect(createMutationCall).toBeDefined(); + expect(createMutationCall[1].categoryId).toBe("DIC_kwDOGFsHUM4BsUn1"); // General (first) + }); +}); diff --git a/pkg/workflow/create_discussion.go b/pkg/workflow/create_discussion.go index 029e4901b4..52610b4d09 100644 --- a/pkg/workflow/create_discussion.go +++ b/pkg/workflow/create_discussion.go @@ -92,10 +92,8 @@ func (c *Compiler) parseDiscussionsConfig(outputMap map[string]any) *CreateDiscu return nil // Invalid configuration, return nil to cause validation error } - // Validate category naming convention (lowercase, preferably plural) - if validateDiscussionCategory(config.Category, discussionLog, c.markdownPath) { - return nil // Invalid configuration, return nil to cause validation error - } + // Normalize and validate category naming convention + config.Category = normalizeDiscussionCategory(config.Category, discussionLog, c.markdownPath) // Log configured values if config.TitlePrefix != "" { @@ -201,18 +199,18 @@ func (c *Compiler) buildCreateOutputDiscussionJob(data *WorkflowData, mainJobNam }) } -// validateDiscussionCategory validates discussion category naming conventions -// Categories should be lowercase and preferably plural -// Returns true if validation fails (invalid), false if valid -func validateDiscussionCategory(category string, log *logger.Logger, markdownPath string) bool { +// normalizeDiscussionCategory normalizes discussion category to lowercase +// and provides warnings about naming conventions +// Returns normalized category (or original if it's a category ID) +func normalizeDiscussionCategory(category string, log *logger.Logger, markdownPath string) string { // Empty category is allowed (GitHub Discussions will use default) if category == "" { - return false + return category } - // GitHub Discussion category IDs start with "DIC_" - these are valid + // GitHub Discussion category IDs start with "DIC_" - don't normalize these if strings.HasPrefix(category, "DIC_") { - return false + return category } // List of known category naming issues and their corrections @@ -223,26 +221,25 @@ func validateDiscussionCategory(category string, log *logger.Logger, markdownPat "Research": "research", } - // Check if category has uppercase letters - if category != strings.ToLower(category) { + // Check if category has uppercase letters and normalize + normalizedCategory := strings.ToLower(category) + if category != normalizedCategory { var message string // Check if we have a known correction if corrected, exists := categoryCorrections[category]; exists { - message = fmt.Sprintf("Discussion category %q should use lowercase: %q", category, corrected) + message = fmt.Sprintf("Discussion category %q normalized to lowercase: %q", category, corrected) if log != nil { - log.Printf("Invalid discussion category %q: should use lowercase: %q", category, corrected) + log.Printf("Normalized discussion category %q to lowercase: %q", category, corrected) } } else { - message = fmt.Sprintf("Discussion category %q should use lowercase", category) + message = fmt.Sprintf("Discussion category %q normalized to lowercase: %q", category, normalizedCategory) if log != nil { - log.Printf("Invalid discussion category %q: should use lowercase", category) + log.Printf("Normalized discussion category %q to lowercase: %q", category, normalizedCategory) } } - // Print formatted warning to stderr - fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", message)) - - return true // Validation failed + // Print formatted info message to stderr + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "info", message)) } // Warn about singular forms of common categories @@ -251,11 +248,11 @@ func validateDiscussionCategory(category string, log *logger.Logger, markdownPat "report": "reports", } - if plural, isSingular := singularToPlural[category]; isSingular { + if plural, isSingular := singularToPlural[normalizedCategory]; isSingular { if log != nil { - log.Printf("⚠ Discussion category %q is singular; consider using plural form %q for consistency", category, plural) + log.Printf("⚠ Discussion category %q is singular; consider using plural form %q for consistency", normalizedCategory, plural) } } - return false // Validation passed + return normalizedCategory } diff --git a/pkg/workflow/create_discussion_validation_test.go b/pkg/workflow/create_discussion_validation_test.go index 64b7353b44..434f539f05 100644 --- a/pkg/workflow/create_discussion_validation_test.go +++ b/pkg/workflow/create_discussion_validation_test.go @@ -9,120 +9,123 @@ import ( "github.com/stretchr/testify/assert" ) -func TestValidateDiscussionCategory(t *testing.T) { +func TestNormalizeDiscussionCategory(t *testing.T) { tests := []struct { - name string - category string - expectInvalid bool + name string + category string + expectedCategory string }{ { - name: "empty category is valid", - category: "", - expectInvalid: false, + name: "empty category unchanged", + category: "", + expectedCategory: "", }, { - name: "lowercase category is valid", - category: "audits", - expectInvalid: false, + name: "lowercase category unchanged", + category: "audits", + expectedCategory: "audits", }, { - name: "lowercase plural is valid", - category: "reports", - expectInvalid: false, + name: "lowercase plural unchanged", + category: "reports", + expectedCategory: "reports", }, { - name: "lowercase research is valid", - category: "research", - expectInvalid: false, + name: "lowercase research unchanged", + category: "research", + expectedCategory: "research", }, { - name: "general lowercase is valid", - category: "general", - expectInvalid: false, + name: "general lowercase unchanged", + category: "general", + expectedCategory: "general", }, { - name: "capitalized Audits fails", - category: "Audits", - expectInvalid: true, + name: "capitalized Audits normalized to lowercase", + category: "Audits", + expectedCategory: "audits", }, { - name: "capitalized General fails", - category: "General", - expectInvalid: true, + name: "capitalized General normalized to lowercase", + category: "General", + expectedCategory: "general", }, { - name: "capitalized Reports fails", - category: "Reports", - expectInvalid: true, + name: "capitalized Reports normalized to lowercase", + category: "Reports", + expectedCategory: "reports", }, { - name: "capitalized Research fails", - category: "Research", - expectInvalid: true, + name: "capitalized Research normalized to lowercase", + category: "Research", + expectedCategory: "research", }, { - name: "unknown capitalized category fails", - category: "MyCategory", - expectInvalid: true, + name: "unknown capitalized category normalized", + category: "MyCategory", + expectedCategory: "mycategory", }, { - name: "mixed case fails", - category: "AuDiTs", - expectInvalid: true, + name: "mixed case normalized", + category: "AuDiTs", + expectedCategory: "audits", }, { - name: "singular audit is valid but logged as warning", - category: "audit", - expectInvalid: false, - // Note: This will log a warning, but not fail + name: "singular audit unchanged (but warns)", + category: "audit", + expectedCategory: "audit", }, { - name: "singular report is valid but logged as warning", - category: "report", - expectInvalid: false, - // Note: This will log a warning, but not fail + name: "singular report unchanged (but warns)", + category: "report", + expectedCategory: "report", }, { - name: "category ID is valid", - category: "DIC_kwDOGFsHUM4BsUn3", - expectInvalid: false, + name: "category ID unchanged", + category: "DIC_kwDOGFsHUM4BsUn3", + expectedCategory: "DIC_kwDOGFsHUM4BsUn3", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { log := logger.New("test:discussion_validation") - isInvalid := validateDiscussionCategory(tt.category, log, "test.md") - - if tt.expectInvalid { - assert.True(t, isInvalid, "Expected validation to fail for category %q", tt.category) - } else { - assert.False(t, isInvalid, "Expected validation to pass for category %q", tt.category) - } + normalized := normalizeDiscussionCategory(tt.category, log, "test.md") + assert.Equal(t, tt.expectedCategory, normalized, "Expected category %q to be normalized to %q", tt.category, tt.expectedCategory) }) } } -func TestParseDiscussionsConfigValidation(t *testing.T) { +func TestParseDiscussionsConfigNormalization(t *testing.T) { tests := []struct { - name string - category string - expectNilResult bool + name string + category string + expectedCategory string + expectNonNilResult bool }{ { - name: "valid lowercase category returns config", - category: "audits", - expectNilResult: false, + name: "valid lowercase category returns config", + category: "audits", + expectedCategory: "audits", + expectNonNilResult: true, }, { - name: "capitalized category returns nil", - category: "Audits", - expectNilResult: true, + name: "capitalized category normalized and returns config", + category: "Audits", + expectedCategory: "audits", + expectNonNilResult: true, }, { - name: "General category returns nil", - category: "General", - expectNilResult: true, + name: "General category normalized and returns config", + category: "General", + expectedCategory: "general", + expectNonNilResult: true, + }, + { + name: "mixed case category normalized", + category: "MyCategory", + expectedCategory: "mycategory", + expectNonNilResult: true, }, } @@ -137,10 +140,11 @@ func TestParseDiscussionsConfigValidation(t *testing.T) { result := compiler.parseDiscussionsConfig(outputMap) - if tt.expectNilResult { - assert.Nil(t, result, "Expected nil result for invalid category %q", tt.category) + if tt.expectNonNilResult { + assert.NotNil(t, result, "Expected non-nil result for category %q", tt.category) + assert.Equal(t, tt.expectedCategory, result.Category, "Expected category to be normalized to %q", tt.expectedCategory) } else { - assert.NotNil(t, result, "Expected non-nil result for valid category %q", tt.category) + assert.Nil(t, result, "Expected nil result for invalid category %q", tt.category) } }) }