From 2abca9fb2307030723f15e05dd7eab44f6321068 Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Mon, 23 Feb 2026 23:26:46 +0100 Subject: [PATCH] refactor(guild): introduce service layer for guild member operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract business logic from handleMsgMhfOperateGuildMember into GuildService.OperateMember, establishing the handler→service→repo layering pattern. The handler is now ~20 lines of protocol glue (type-assert, map action, call service, send ACK, notify). GuildService owns authorization checks, repo coordination, mail composition, and best-effort mail delivery. It accepts plain Go types (no mhfpacket or Session imports), making it fully testable with mock repos. Cross-channel notification stays in the handler since it requires Session. Adds 7 table-driven service-level tests covering accept/reject/kick, authorization, repo errors, mail errors, and unknown actions. --- server/channelserver/handlers_guild_ops.go | 70 +++----- .../channelserver/handlers_guild_ops_test.go | 5 + server/channelserver/svc_guild.go | 106 +++++++++++ server/channelserver/svc_guild_test.go | 170 ++++++++++++++++++ server/channelserver/sys_channel_server.go | 3 + server/channelserver/test_helpers_test.go | 7 + 6 files changed, 313 insertions(+), 48 deletions(-) create mode 100644 server/channelserver/svc_guild.go create mode 100644 server/channelserver/svc_guild_test.go diff --git a/server/channelserver/handlers_guild_ops.go b/server/channelserver/handlers_guild_ops.go index 56a7b339a..5adc7f4a0 100644 --- a/server/channelserver/handlers_guild_ops.go +++ b/server/channelserver/handlers_guild_ops.go @@ -266,58 +266,32 @@ func handleAvoidLeadershipUpdate(s *Session, pkt *mhfpacket.MsgMhfOperateGuild, func handleMsgMhfOperateGuildMember(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfOperateGuildMember) - guild, err := s.server.guildRepo.GetByCharID(pkt.CharID) - - if err != nil || guild == nil { - doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) - return - } - - actorCharacter, err := s.server.guildRepo.GetCharacterMembership(s.charID) - - if err != nil || (!actorCharacter.IsSubLeader() && guild.LeaderCharID != s.charID) { - doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) - return - } - - var mail Mail - switch pkt.Action { - case mhfpacket.OPERATE_GUILD_MEMBER_ACTION_ACCEPT: - err = s.server.guildRepo.AcceptApplication(guild.ID, pkt.CharID) - mail = Mail{ - RecipientID: pkt.CharID, - Subject: "Accepted!", - Body: fmt.Sprintf("Your application to join 「%s」 was accepted.", guild.Name), - IsSystemMessage: true, - } - case mhfpacket.OPERATE_GUILD_MEMBER_ACTION_REJECT: - err = s.server.guildRepo.RejectApplication(guild.ID, pkt.CharID) - mail = Mail{ - RecipientID: pkt.CharID, - Subject: "Rejected", - Body: fmt.Sprintf("Your application to join 「%s」 was rejected.", guild.Name), - IsSystemMessage: true, - } - case mhfpacket.OPERATE_GUILD_MEMBER_ACTION_KICK: - err = s.server.guildRepo.RemoveCharacter(pkt.CharID) - mail = Mail{ - RecipientID: pkt.CharID, - Subject: "Kicked", - Body: fmt.Sprintf("You were kicked from 「%s」.", guild.Name), - IsSystemMessage: true, - } - default: - doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) + action, ok := mapMemberAction(pkt.Action) + if !ok { s.logger.Warn("Unhandled operateGuildMember action", zap.Uint8("action", pkt.Action)) + doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) + return } + result, err := s.server.guildService.OperateMember(s.charID, pkt.CharID, action) if err != nil { doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) - } else { - if err := s.server.mailRepo.SendMail(mail.SenderID, mail.RecipientID, mail.Subject, mail.Body, 0, 0, false, true); err != nil { - s.logger.Warn("Failed to send guild member operation mail", zap.Error(err)) - } - s.server.Registry.NotifyMailToCharID(pkt.CharID, s, &mail) - doAckSimpleSucceed(s, pkt.AckHandle, make([]byte, 4)) + return + } + + s.server.Registry.NotifyMailToCharID(result.MailRecipientID, s, &result.Mail) + doAckSimpleSucceed(s, pkt.AckHandle, make([]byte, 4)) +} + +func mapMemberAction(proto uint8) (GuildMemberAction, bool) { + switch proto { + case mhfpacket.OPERATE_GUILD_MEMBER_ACTION_ACCEPT: + return GuildMemberActionAccept, true + case mhfpacket.OPERATE_GUILD_MEMBER_ACTION_REJECT: + return GuildMemberActionReject, true + case mhfpacket.OPERATE_GUILD_MEMBER_ACTION_KICK: + return GuildMemberActionKick, true + default: + return 0, false } } diff --git a/server/channelserver/handlers_guild_ops_test.go b/server/channelserver/handlers_guild_ops_test.go index 33c118e2f..b9f841c10 100644 --- a/server/channelserver/handlers_guild_ops_test.go +++ b/server/channelserver/handlers_guild_ops_test.go @@ -448,6 +448,7 @@ func TestOperateGuildMember_Accept(t *testing.T) { guildMock.guild.LeaderCharID = 1 server.guildRepo = guildMock server.mailRepo = mailMock + ensureGuildService(server) session := createMockSession(1, server) pkt := &mhfpacket.MsgMhfOperateGuildMember{ @@ -486,6 +487,7 @@ func TestOperateGuildMember_Reject(t *testing.T) { guildMock.guild.LeaderCharID = 1 server.guildRepo = guildMock server.mailRepo = mailMock + ensureGuildService(server) session := createMockSession(1, server) pkt := &mhfpacket.MsgMhfOperateGuildMember{ @@ -515,6 +517,7 @@ func TestOperateGuildMember_Kick(t *testing.T) { guildMock.guild.LeaderCharID = 1 server.guildRepo = guildMock server.mailRepo = mailMock + ensureGuildService(server) session := createMockSession(1, server) pkt := &mhfpacket.MsgMhfOperateGuildMember{ @@ -544,6 +547,7 @@ func TestOperateGuildMember_MailError(t *testing.T) { guildMock.guild.LeaderCharID = 1 server.guildRepo = guildMock server.mailRepo = mailMock + ensureGuildService(server) session := createMockSession(1, server) pkt := &mhfpacket.MsgMhfOperateGuildMember{ @@ -572,6 +576,7 @@ func TestOperateGuildMember_NotLeaderOrSub(t *testing.T) { guildMock.guild.LeaderCharID = 999 // not the session char server.guildRepo = guildMock server.mailRepo = &mockMailRepo{} + ensureGuildService(server) session := createMockSession(1, server) pkt := &mhfpacket.MsgMhfOperateGuildMember{ diff --git a/server/channelserver/svc_guild.go b/server/channelserver/svc_guild.go new file mode 100644 index 000000000..7702ca6bf --- /dev/null +++ b/server/channelserver/svc_guild.go @@ -0,0 +1,106 @@ +package channelserver + +import ( + "errors" + "fmt" + + "go.uber.org/zap" +) + +// GuildMemberAction is a domain enum for guild member operations. +type GuildMemberAction uint8 + +const ( + GuildMemberActionAccept GuildMemberAction = iota + 1 + GuildMemberActionReject + GuildMemberActionKick +) + +// ErrUnauthorized is returned when the actor lacks permission for the operation. +var ErrUnauthorized = errors.New("unauthorized") + +// ErrUnknownAction is returned for unrecognized guild member actions. +var ErrUnknownAction = errors.New("unknown guild member action") + +// OperateMemberResult holds the outcome of a guild member operation. +type OperateMemberResult struct { + MailRecipientID uint32 + Mail Mail +} + +// GuildService encapsulates guild business logic, sitting between handlers and repos. +type GuildService struct { + guildRepo GuildRepo + mailRepo MailRepo + charRepo CharacterRepo + logger *zap.Logger +} + +// NewGuildService creates a new GuildService. +func NewGuildService(gr GuildRepo, mr MailRepo, cr CharacterRepo, log *zap.Logger) *GuildService { + return &GuildService{ + guildRepo: gr, + mailRepo: mr, + charRepo: cr, + logger: log, + } +} + +// OperateMember performs a guild member management action (accept/reject/kick). +// The actor must be the guild leader or a sub-leader. On success, a notification +// mail is sent (best-effort) and the result is returned for protocol-level notification. +func (svc *GuildService) OperateMember(actorCharID, targetCharID uint32, action GuildMemberAction) (*OperateMemberResult, error) { + guild, err := svc.guildRepo.GetByCharID(targetCharID) + if err != nil || guild == nil { + return nil, fmt.Errorf("guild lookup for char %d: %w", targetCharID, err) + } + + actorMember, err := svc.guildRepo.GetCharacterMembership(actorCharID) + if err != nil || (!actorMember.IsSubLeader() && guild.LeaderCharID != actorCharID) { + return nil, ErrUnauthorized + } + + var mail Mail + switch action { + case GuildMemberActionAccept: + err = svc.guildRepo.AcceptApplication(guild.ID, targetCharID) + mail = Mail{ + RecipientID: targetCharID, + Subject: "Accepted!", + Body: fmt.Sprintf("Your application to join 「%s」 was accepted.", guild.Name), + IsSystemMessage: true, + } + case GuildMemberActionReject: + err = svc.guildRepo.RejectApplication(guild.ID, targetCharID) + mail = Mail{ + RecipientID: targetCharID, + Subject: "Rejected", + Body: fmt.Sprintf("Your application to join 「%s」 was rejected.", guild.Name), + IsSystemMessage: true, + } + case GuildMemberActionKick: + err = svc.guildRepo.RemoveCharacter(targetCharID) + mail = Mail{ + RecipientID: targetCharID, + Subject: "Kicked", + Body: fmt.Sprintf("You were kicked from 「%s」.", guild.Name), + IsSystemMessage: true, + } + default: + return nil, ErrUnknownAction + } + + if err != nil { + return nil, fmt.Errorf("guild member action %d: %w", action, err) + } + + // Send mail best-effort + if mailErr := svc.mailRepo.SendMail(mail.SenderID, mail.RecipientID, mail.Subject, mail.Body, 0, 0, false, true); mailErr != nil { + svc.logger.Warn("Failed to send guild member operation mail", zap.Error(mailErr)) + } + + return &OperateMemberResult{ + MailRecipientID: targetCharID, + Mail: mail, + }, nil +} diff --git a/server/channelserver/svc_guild_test.go b/server/channelserver/svc_guild_test.go new file mode 100644 index 000000000..a8992f4e3 --- /dev/null +++ b/server/channelserver/svc_guild_test.go @@ -0,0 +1,170 @@ +package channelserver + +import ( + "errors" + "testing" + + "go.uber.org/zap" +) + +func newTestGuildService(gr GuildRepo, mr MailRepo) *GuildService { + logger, _ := zap.NewDevelopment() + return NewGuildService(gr, mr, nil, logger) +} + +func TestGuildService_OperateMember(t *testing.T) { + tests := []struct { + name string + actorCharID uint32 + targetCharID uint32 + action GuildMemberAction + guild *Guild + membership *GuildMember + acceptErr error + rejectErr error + removeErr error + sendErr error + wantErr bool + wantErrIs error + wantAccepted uint32 + wantRejected uint32 + wantRemoved uint32 + wantMailCount int + wantRecipient uint32 + wantMailSubj string + }{ + { + name: "accept application as leader", + actorCharID: 1, + targetCharID: 42, + action: GuildMemberActionAccept, + guild: &Guild{ID: 10, Name: "TestGuild", GuildLeader: GuildLeader{LeaderCharID: 1}}, + membership: &GuildMember{GuildID: 10, CharID: 1, IsLeader: true, OrderIndex: 1}, + wantAccepted: 42, + wantMailCount: 1, + wantRecipient: 42, + wantMailSubj: "Accepted!", + }, + { + name: "reject application as sub-leader", + actorCharID: 2, + targetCharID: 42, + action: GuildMemberActionReject, + guild: &Guild{ID: 10, Name: "TestGuild", GuildLeader: GuildLeader{LeaderCharID: 1}}, + membership: &GuildMember{GuildID: 10, CharID: 2, OrderIndex: 2}, // sub-leader + wantRejected: 42, + wantMailCount: 1, + wantRecipient: 42, + wantMailSubj: "Rejected", + }, + { + name: "kick member as leader", + actorCharID: 1, + targetCharID: 42, + action: GuildMemberActionKick, + guild: &Guild{ID: 10, Name: "TestGuild", GuildLeader: GuildLeader{LeaderCharID: 1}}, + membership: &GuildMember{GuildID: 10, CharID: 1, IsLeader: true, OrderIndex: 1}, + wantRemoved: 42, + wantMailCount: 1, + wantRecipient: 42, + wantMailSubj: "Kicked", + }, + { + name: "unauthorized - not leader or sub", + actorCharID: 5, + targetCharID: 42, + action: GuildMemberActionAccept, + guild: &Guild{ID: 10, Name: "TestGuild", GuildLeader: GuildLeader{LeaderCharID: 1}}, + membership: &GuildMember{GuildID: 10, CharID: 5, OrderIndex: 10}, + wantErr: true, + wantErrIs: ErrUnauthorized, + }, + { + name: "repo error on accept", + actorCharID: 1, + targetCharID: 42, + action: GuildMemberActionAccept, + guild: &Guild{ID: 10, Name: "TestGuild", GuildLeader: GuildLeader{LeaderCharID: 1}}, + membership: &GuildMember{GuildID: 10, CharID: 1, IsLeader: true, OrderIndex: 1}, + acceptErr: errors.New("db error"), + wantErr: true, + }, + { + name: "mail error is best-effort", + actorCharID: 1, + targetCharID: 42, + action: GuildMemberActionAccept, + guild: &Guild{ID: 10, Name: "TestGuild", GuildLeader: GuildLeader{LeaderCharID: 1}}, + membership: &GuildMember{GuildID: 10, CharID: 1, IsLeader: true, OrderIndex: 1}, + sendErr: errors.New("mail failed"), + wantAccepted: 42, + wantMailCount: 1, + wantRecipient: 42, + wantMailSubj: "Accepted!", + }, + { + name: "unknown action", + actorCharID: 1, + targetCharID: 42, + action: GuildMemberAction(99), + guild: &Guild{ID: 10, Name: "TestGuild", GuildLeader: GuildLeader{LeaderCharID: 1}}, + membership: &GuildMember{GuildID: 10, CharID: 1, IsLeader: true, OrderIndex: 1}, + wantErr: true, + wantErrIs: ErrUnknownAction, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + guildMock := &mockGuildRepoOps{ + membership: tt.membership, + acceptErr: tt.acceptErr, + rejectErr: tt.rejectErr, + removeErr: tt.removeErr, + } + guildMock.guild = tt.guild + mailMock := &mockMailRepo{sendErr: tt.sendErr} + + svc := newTestGuildService(guildMock, mailMock) + + result, err := svc.OperateMember(tt.actorCharID, tt.targetCharID, tt.action) + + if tt.wantErr { + if err == nil { + t.Fatal("Expected error, got nil") + } + if tt.wantErrIs != nil && !errors.Is(err, tt.wantErrIs) { + t.Errorf("Expected error %v, got %v", tt.wantErrIs, err) + } + return + } + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if tt.wantAccepted != 0 && guildMock.acceptedCharID != tt.wantAccepted { + t.Errorf("acceptedCharID = %d, want %d", guildMock.acceptedCharID, tt.wantAccepted) + } + if tt.wantRejected != 0 && guildMock.rejectedCharID != tt.wantRejected { + t.Errorf("rejectedCharID = %d, want %d", guildMock.rejectedCharID, tt.wantRejected) + } + if tt.wantRemoved != 0 && guildMock.removedCharID != tt.wantRemoved { + t.Errorf("removedCharID = %d, want %d", guildMock.removedCharID, tt.wantRemoved) + } + if len(mailMock.sentMails) != tt.wantMailCount { + t.Fatalf("sentMails count = %d, want %d", len(mailMock.sentMails), tt.wantMailCount) + } + if tt.wantMailCount > 0 { + if mailMock.sentMails[0].recipientID != tt.wantRecipient { + t.Errorf("mail recipientID = %d, want %d", mailMock.sentMails[0].recipientID, tt.wantRecipient) + } + if mailMock.sentMails[0].subject != tt.wantMailSubj { + t.Errorf("mail subject = %q, want %q", mailMock.sentMails[0].subject, tt.wantMailSubj) + } + } + if result.MailRecipientID != tt.targetCharID { + t.Errorf("result.MailRecipientID = %d, want %d", result.MailRecipientID, tt.targetCharID) + } + }) + } +} diff --git a/server/channelserver/sys_channel_server.go b/server/channelserver/sys_channel_server.go index 8820edfa6..6fdca87e7 100644 --- a/server/channelserver/sys_channel_server.go +++ b/server/channelserver/sys_channel_server.go @@ -71,6 +71,7 @@ type Server struct { miscRepo MiscRepo scenarioRepo ScenarioRepo mercenaryRepo MercenaryRepo + guildService *GuildService erupeConfig *cfg.Config acceptConns chan net.Conn deleteConns chan net.Conn @@ -153,6 +154,8 @@ func NewServer(config *Config) *Server { s.scenarioRepo = NewScenarioRepository(config.DB) s.mercenaryRepo = NewMercenaryRepository(config.DB) + s.guildService = NewGuildService(s.guildRepo, s.mailRepo, s.charRepo, s.logger) + // Mezeporta s.stages.Store("sl1Ns200p0a0u0", NewStage("sl1Ns200p0a0u0")) diff --git a/server/channelserver/test_helpers_test.go b/server/channelserver/test_helpers_test.go index b6a284459..5a46e6ff6 100644 --- a/server/channelserver/test_helpers_test.go +++ b/server/channelserver/test_helpers_test.go @@ -51,9 +51,16 @@ func createMockServer() *Server { } s.i18n = getLangStrings(s) s.Registry = NewLocalChannelRegistry([]*Server{s}) + // GuildService is wired lazily by tests that set repos then call ensureGuildService. return s } +// ensureGuildService wires the GuildService from the server's current repos. +// Call this after setting guildRepo, mailRepo, and charRepo on the mock server. +func ensureGuildService(s *Server) { + s.guildService = NewGuildService(s.guildRepo, s.mailRepo, s.charRepo, s.logger) +} + // createMockSession creates a minimal Session for testing. // Imported from v9.2.x-stable and adapted for main. func createMockSession(charID uint32, server *Server) *Session {