diff --git a/bot/bot.go b/bot/bot.go index 93ca669..f1023f6 100644 --- a/bot/bot.go +++ b/bot/bot.go @@ -81,101 +81,45 @@ const ( PointCheck ) -// detectPointOperation checks if the message contains a point operation (++, --, ==) -func (b *Bot) detectPointOperation(text string) PointOperation { - // Support both user mentions and emoji patterns - // User mention patterns: <@U123456>++ - userPlusPattern := `.*<@[A-Z0-9]+>[ 　]*\+\+.*` - userMinusPattern := `.*<@[A-Z0-9]+>[ 　]*\-\-.*` - userEqualsPattern := `.*<@[A-Z0-9]+>[ 　]*\=\=.*` - - // Emoji patterns: :emoji: ++ - emojiPlusPattern := `.*:[a-zA-Z0-9_+-]+:[ 　]*\+\+.*` - emojiMinusPattern := `.*:[a-zA-Z0-9_+-]+:[ 　]*\-\-.*` - emojiEqualsPattern := `.*:[a-zA-Z0-9_+-]+:[ 　]*\=\=.*` - - // Check for plus patterns - userPlusMatched, err := regexp.MatchString(userPlusPattern, text) - if err != nil { - b.logger.Error("Error matching user plus pattern", "error", err) - return NoOperation - } - emojiPlusMatched, err := regexp.MatchString(emojiPlusPattern, text) - if err != nil { - b.logger.Error("Error matching emoji plus pattern", "error", err) - return NoOperation - } - if userPlusMatched || emojiPlusMatched { - return PointUp - } +// Pre-compiled regexes for detecting point operations with targets +var ( + // User mention pattern: <@U123456> ++ (captures user ID and operator) + userOperationPattern = regexp.MustCompile(`<@([A-Z0-9]+)>[ 　]*(\+\+|-{2}|={2})`) + // Emoji pattern: :emoji: ++ (captures emoji name and operator) + emojiOperationPattern = regexp.MustCompile(`:([a-zA-Z0-9_+-]+):[ 　]*(\+\+|-{2}|={2})`) +) - // Check for minus patterns - userMinusMatched, err := regexp.MatchString(userMinusPattern, text) - if err != nil { - b.logger.Error("Error matching user minus pattern", "error", err) - return NoOperation - } - emojiMinusMatched, err := regexp.MatchString(emojiMinusPattern, text) - if err != nil { - b.logger.Error("Error matching emoji minus pattern", "error", err) - return NoOperation - } - if userMinusMatched || emojiMinusMatched { +// parseOperator converts an operator string to a PointOperation +func parseOperator(op string) PointOperation { + switch op { + case "++": + return PointUp + case "--": return PointDown - } - - // Check for equals patterns - userEqualsMatched, err := regexp.MatchString(userEqualsPattern, text) - if err != nil { - b.logger.Error("Error matching user equals pattern", "error", err) - return NoOperation - } - emojiEqualsMatched, err := regexp.MatchString(emojiEqualsPattern, text) - if err != nil { - b.logger.Error("Error matching emoji equals pattern", "error", err) - return NoOperation - } - if userEqualsMatched || emojiEqualsMatched { + case "==": return PointCheck + default: + return NoOperation } - - return NoOperation } -func extractUserID(text string) string { - re := regexp.MustCompile(`<@([A-Z0-9]+)>`) - matches := re.FindStringSubmatch(text) - if len(matches) < 2 { - return "" +// detectOperationAndTarget detects the point operation and extracts the target in one step. +// This ensures the detected operation is associated with the correct target. +func detectOperationAndTarget(text string) (PointOperation, string, bool) { + // User mentions have priority + if matches := userOperationPattern.FindStringSubmatch(text); len(matches) >= 3 { + return parseOperator(matches[2]), matches[1], true } - return matches[1] -} - -// extractEmojiName extracts emoji name from text (e.g., ":sake:" -> "sake") -func extractEmojiName(text string) string { - re := regexp.MustCompile(`:([a-zA-Z0-9_+-]+):`) - matches := re.FindStringSubmatch(text) - if len(matches) < 2 { - return "" + if matches := emojiOperationPattern.FindStringSubmatch(text); len(matches) >= 3 { + return parseOperator(matches[2]), matches[1], false } - return matches[1] + return NoOperation, "", false } -// extractTargetFromText extracts either user ID or emoji name from text -func extractTargetFromText(text string) (string, bool) { - // Try to extract user ID first - userID := extractUserID(text) - if userID != "" { - return userID, true // true indicates it's a user ID - } - - // If no user ID, try to extract emoji name - emojiName := extractEmojiName(text) - if emojiName != "" { - return emojiName, false // false indicates it's an emoji name - } - - return "", false +// detectPointOperation checks if the message contains a point operation (++, --, ==) +func (b *Bot) detectPointOperation(text string) PointOperation { + op, _, _ := detectOperationAndTarget(text) + return op } // isUser checks if the given user ID belongs to a user @@ -189,13 +133,7 @@ func (b *Bot) isUser(userID string) (bool, error) { } // handlePointChangeMessage processes a point up or down message -func (b *Bot) handlePointChangeMessage(ev *slackevents.MessageEvent, operation PointOperation) { - // Extract target (user ID or emoji name) from the message - target, isUser := extractTargetFromText(ev.Text) - if target == "" { - b.logger.Error("No target found in message") - return - } +func (b *Bot) handlePointChangeMessage(ev *slackevents.MessageEvent, operation PointOperation, target string, isUser bool) { // Check if user is trying to point themselves (only applies to user targets) if isUser && target == ev.User { @@ -258,14 +196,7 @@ func (b *Bot) handlePointChangeMessage(ev *slackevents.MessageEvent, operation P b.logger.Debug("Reply sent", "message", message) } -func (b *Bot) handlePointCheckMessage(ev *slackevents.MessageEvent) { - // Extract target (user ID or emoji name) from the message - target, isUser := extractTargetFromText(ev.Text) - if target == "" { - b.logger.Error("No target found in message") - return - } - +func (b *Bot) handlePointCheckMessage(ev *slackevents.MessageEvent, target string, isUser bool) { ctx := context.Background() points, err := b.repo.GetPoints(ctx, target) if err != nil { @@ -284,13 +215,13 @@ func (b *Bot) handlePointCheckMessage(ev *slackevents.MessageEvent) { // handleMessageEvent processes a message event func (b *Bot) handleMessageEvent(ev *slackevents.MessageEvent) { b.logger.Debug("Received message event", "event", ev) - operation := b.detectPointOperation(ev.Text) - if operation != NoOperation { - b.logger.Info("Point operation detected", "text", ev.Text) + operation, target, isUser := detectOperationAndTarget(ev.Text) + if operation != NoOperation && target != "" { + b.logger.Info("Point operation detected", "text", ev.Text, "target", target, "isUser", isUser) if operation == PointCheck { - b.handlePointCheckMessage(ev) + b.handlePointCheckMessage(ev, target, isUser) } else { - b.handlePointChangeMessage(ev, operation) + b.handlePointChangeMessage(ev, operation, target, isUser) } } } diff --git a/bot/bot_test.go b/bot/bot_test.go index d8f1a2c..5e87772 100644 --- a/bot/bot_test.go +++ b/bot/bot_test.go @@ -200,124 +200,204 @@ func TestDetectPointOperation(t *testing.T) { } } -func TestExtractUserID(t *testing.T) { +func TestDetectOperationAndTarget(t *testing.T) { tests := []struct { - name string - text string - want string + name string + text string + wantOp PointOperation + wantTarget string + wantIsUser bool }{ + // Normal cases: user mention { - name: "Valid user ID", - text: "<@U123456>++", - want: "U123456", + name: "User point up", + text: "<@U123456>++", + wantOp: PointUp, + wantTarget: "U123456", + wantIsUser: true, }, { - name: "No user ID", - text: "Hello world", - want: "", + name: "User point down", + text: "<@U123456>--", + wantOp: PointDown, + wantTarget: "U123456", + wantIsUser: true, }, { - name: "Invalid format", - text: "++", - want: "", + name: "User point check", + text: "<@U123456>==", + wantOp: PointCheck, + wantTarget: "U123456", + wantIsUser: true, }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := extractUserID(tt.text) - if got != tt.want { - t.Errorf("extractUserID() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestExtractEmojiName(t *testing.T) { - tests := []struct { - name string - text string - want string - }{ { - name: "Valid emoji", - text: ":sake: ++", - want: "sake", + name: "User with space", + text: "<@U123456> ++", + wantOp: PointUp, + wantTarget: "U123456", + wantIsUser: true, }, { - name: "Emoji with underscores", - text: ":beer_mug: ++", - want: "beer_mug", + name: "User with full-width space", + text: "<@U123456>　++", + wantOp: PointUp, + wantTarget: "U123456", + wantIsUser: true, }, + // Normal cases: emoji { - name: "Emoji with numbers", - text: ":beer2: ++", - want: "beer2", + name: "Emoji point up", + text: ":sake: ++", + wantOp: PointUp, + wantTarget: "sake", + wantIsUser: false, }, { - name: "No emoji", - text: "Hello world", - want: "", + name: "Emoji point down", + text: ":sake: --", + wantOp: PointDown, + wantTarget: "sake", + wantIsUser: false, }, { - name: "Invalid emoji format", - text: ":sake ++", - want: "", + name: "Emoji point check", + text: ":sake: ==", + wantOp: PointCheck, + wantTarget: "sake", + wantIsUser: false, + }, + { + name: "Emoji no space", + text: ":sake:++", + wantOp: PointUp, + wantTarget: "sake", + wantIsUser: false, + }, + { + name: "Emoji with underscores", + text: ":beer_mug: ++", + wantOp: PointUp, + wantTarget: "beer_mug", + wantIsUser: false, + }, + { + name: "Emoji with numbers", + text: ":beer2: ++", + wantOp: PointUp, + wantTarget: "beer2", + wantIsUser: false, + }, + { + name: "Emoji with hyphens", + text: ":haruotsu-no1: ++", + wantOp: PointUp, + wantTarget: "haruotsu-no1", + wantIsUser: false, + }, + // Text with characters between target and operator should NOT match + { + name: "Emoji with text before operator (plus)", + text: ":haruotsu-no1:hogehogehogege ++", + wantOp: NoOperation, + wantTarget: "", + wantIsUser: false, }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := extractEmojiName(tt.text) - if got != tt.want { - t.Errorf("extractEmojiName() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestExtractTargetFromText(t *testing.T) { - tests := []struct { - name string - text string - wantTarget string - wantIsUser bool - }{ { - name: "User mention", - text: "<@U123456>++", - wantTarget: "U123456", - wantIsUser: true, + name: "Emoji with text before operator (minus)", + text: ":haruotsu-no1:hogehogehogege --", + wantOp: NoOperation, + wantTarget: "", + wantIsUser: false, }, { - name: "Emoji", - text: ":sake: ++", - wantTarget: "sake", - wantIsUser: false, + name: "Emoji with text directly before operator", + text: ":sake:hogehoge++", + wantOp: NoOperation, + wantTarget: "", + wantIsUser: false, }, { - name: "No target", - text: "Hello world", - wantTarget: "", - wantIsUser: false, + name: "User with text before operator", + text: "<@U123456>hogehoge ++", + wantOp: NoOperation, + wantTarget: "", + wantIsUser: false, }, + // Multiple emoji patterns - should match the correct one { - name: "User mention has priority", - text: "<@U123456> :sake: ++", - wantTarget: "U123456", - wantIsUser: true, + name: "Multiple emojis, second one has operator", + text: "hello :foo:bar :baz: ++", + wantOp: PointUp, + wantTarget: "baz", + wantIsUser: false, + }, + // When both user and emoji exist, the one directly followed by the operator wins + { + name: "Emoji directly before operator takes precedence", + text: "<@U123456> :sake: ++", + wantOp: PointUp, + wantTarget: "sake", + wantIsUser: false, + }, + { + name: "User directly before operator takes precedence", + text: ":sake: <@U123456> ++", + wantOp: PointUp, + wantTarget: "U123456", + wantIsUser: true, + }, + // No operation + { + name: "No operation", + text: "Hello world", + wantOp: NoOperation, + wantTarget: "", + wantIsUser: false, + }, + { + name: "Newline between target and operator", + text: "<@U123456>\n++", + wantOp: NoOperation, + wantTarget: "", + wantIsUser: false, + }, + { + name: "Invalid emoji format", + text: ":sake +", + wantOp: NoOperation, + wantTarget: "", + wantIsUser: false, + }, + // Prefix text should be fine + { + name: "Text before emoji operation", + text: "nice work :sake: ++", + wantOp: PointUp, + wantTarget: "sake", + wantIsUser: false, + }, + { + name: "Text before user operation", + text: "good job <@U123456> ++", + wantOp: PointUp, + wantTarget: "U123456", + wantIsUser: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotTarget, gotIsUser := extractTargetFromText(tt.text) + gotOp, gotTarget, gotIsUser := detectOperationAndTarget(tt.text) + if gotOp != tt.wantOp { + t.Errorf("detectOperationAndTarget() op = %v, want %v", gotOp, tt.wantOp) + } if gotTarget != tt.wantTarget { - t.Errorf("extractTargetFromText() gotTarget = %v, want %v", gotTarget, tt.wantTarget) + t.Errorf("detectOperationAndTarget() target = %v, want %v", gotTarget, tt.wantTarget) } if gotIsUser != tt.wantIsUser { - t.Errorf("extractTargetFromText() gotIsUser = %v, want %v", gotIsUser, tt.wantIsUser) + t.Errorf("detectOperationAndTarget() isUser = %v, want %v", gotIsUser, tt.wantIsUser) } }) } } +