fix: make /guild code redeem consistent with /guild accept

docs/e2e-coverage.md

@@ -3,7 +3,7 @@
 Which Guilds feature is tested where, and what is deliberately left out. See `e2e-testing.md` for
 how to run any of it.
 
-141 tests across 12 spec files, plus 3 preflight tests that run before every suite and fail the run
+162 tests across 14 spec files, plus 3 preflight tests that run before every suite and fail the run
 if Guilds or any of its hard dependencies did not load.
 
 ## Preflight — `plugins/guilds-preflight.spec.ts`
@@ -17,12 +17,13 @@ meaningful.
 | The economy and permission providers Vault needs are registered | Vault's `Economy` and `Permission` services resolve |
 | Guilds commands are reachable and its language is English | `/guild` answers, and does so in English |
 
-## A. Guild lifecycle — `tests/lifecycle.spec.ts` (19)
+## A. Guild lifecycle — `tests/lifecycle.spec.ts` (21)
 
 | Requirement | Test |
 | --- | --- |
 | Successful guild creation | creates a guild, charges for it, and shows it in the guild list |
 | Creation confirmation and cancellation | cancelling the prompt leaves no guild and no charge |
+| The confirmation is checked again | refuses the confirmation when the player joined a guild in the meantime / refuses the confirmation when another guild took the name in the meantime |
 | Invalid guild names and prefixes | rejects a guild name that breaks the configured requirements / rejects a name too long to serve as its own prefix / rejects an explicit prefix that breaks the prefix requirements |
 | Duplicate guild names | rejects a guild name another guild already holds |
 | Insufficient funds | rejects creation when the player cannot afford it |
@@ -32,7 +33,7 @@ meaningful.
 | Status and information commands | toggles the guild between public and private / changes the guild prefix / sets, shows and clears the guild MOTD / reports guild membership through the guild list and rejects guild-scoped commands elsewhere |
 | Name blacklist | rejects a guild name that is a substring of a blacklisted word / accepts a guild name that contains a blacklisted word |
 
-## B. Membership management — `tests/membership.spec.ts` (19)
+## B. Membership management — `tests/membership.spec.ts` (21)
 
 | Requirement | Test |
 | --- | --- |
@@ -41,6 +42,7 @@ meaningful.
 | Declining an invitation | an invited player can decline and stays without a guild |
 | Preventing duplicate invitations | a player cannot be invited twice / a player who is already in a guild cannot be invited |
 | Preventing joining when already in another guild | a player cannot join a second guild while in one / a player cannot accept a guild they were never invited to / a player accepting a guild that does not exist is told the guild does not exist |
+| Finding a guild by name | a guild is found when its name is typed in a different case |
 | Leaving a guild | a member can leave and the guild is told |
 | Leaving and being invited back | a member can leave, be invited back and join the guild again |
 | Master leaving deletes the guild | a member leaving warns that the guild master deletes the guild by leaving |
@@ -51,7 +53,7 @@ meaningful.
 | Guild capacity | a full guild refuses another member |
 | Permissions after leaving or being removed | a player who leaves loses the permissions their guild granted them / a player who is kicked loses the permissions their guild granted them |
 
-## C. Roles and permissions — `tests/roles.spec.ts` (8), `tests/admin.spec.ts` (7)
+## C. Roles and permissions — `tests/roles.spec.ts` (8), `tests/admin.spec.ts` (8)
 
 | Requirement | Test |
 | --- | --- |
@@ -64,6 +66,7 @@ meaningful.
 | Tier permissions after upgrade | upgrading charges the tier price and moves the guild onto the new tier / **an upgrade leaves the master with the permissions the new tier grants** |
 | A player in no guild | a player in no guild is told so rather than refused by role |
 | Admin commands that name a guild and a second argument | an admin command that names a guild and a second argument reaches its handler / an admin command that names a guild which does not exist says so / an admin command that names a guild and a second argument is refused for an unknown player |
+| Admin commands that name a guild in a different case | an admin command finds its guild when the name is typed in a different case |
 | Admin commands that act on a player | an admin command can put a player into a guild / an admin command can hand a player upgrade tickets / an admin command can hand a guild to one of its members |
 | Admin commands that act on an arena | an admin command can teleport to an arena spawn point |
 | The player-facing bank commands | the player-facing bank commands work |
@@ -128,15 +131,19 @@ meaningful.
 | Alliance chat between guilds | `chat.spec.ts`, ally chat reaches an allied guild and not the third party |
 | Cleanup on deletion | a deleted guild drops out of its allies |
 
-## H. Invite codes — `tests/codes.spec.ts` (12, of which 5 are one `describe.serial` chain)
+## H. Invite codes — `tests/codes.spec.ts` (18, of which 5 are one `describe.serial` chain)
 
 | Requirement | Test |
 | --- | --- |
 | Creating invite codes | a code can be created, listed and deleted |
 | Viewing and listing codes | a code can be created, listed and deleted / *and it is listed for the guild master* |
 | Redeeming a code | a code can be redeemed and puts the player in the guild |
+| Redeeming a code more than once | a code with two uses can be redeemed twice |
+| Redeeming a code grants what the guild grants | a player who joins by code gets the permissions their guild grants them |
+| The join cooldown applies to a code | a code cannot be redeemed while the join cooldown is running |
 | Deleting codes | a code can be created, listed and deleted |
 | Invalid codes | an invalid code is refused |
+| Use counts that could never be redeemed | a code with no uses is refused and nothing is stored / a code with a negative number of uses is refused and nothing is stored / a refused code does not use up the guild allowance of codes |
 | Reusing codes where restricted | a code runs out of uses and then says so |
 | Permission restrictions | code information is restricted by role / *a member can be added and cannot manage codes* |
 | Full lifecycle | *one code through its whole life* — a `describe.serial` chain: create → listed → redeem → deleted, asserting membership at each step rather than the command's reply |
@@ -188,6 +195,10 @@ the server answering in another language for the rest of the run. ACF's `usePerI
 false)` is what makes that safe, and one player switching language leaves everyone else in English
 asserts it rather than assuming it.
 
+## L. Startup and the guild data — `tests/startup-loading.spec.ts` (4)
+
+## Runtime smoke coverage — `tests/smoke.spec.ts` (7)
+
 ## Fixture self-check — `tests/bank.spec.ts`
 
 `STARTING_BALANCE is what the fixtures actually fund` is not a Guilds test. It checks that the

src/main/java/me/glaremasters/guilds/guild/Guild.java

@@ -180,23 +180,43 @@
 
     /**
      * Add player to a guild
+     *
      * @param player the player being added
      * @param guildHandler guild handler
      */
     public void addMember(OfflinePlayer player, GuildHandler guildHandler) {
-        GuildMember member = new GuildMember(player.getUniqueId(), guildHandler.getLowestGuildRole());
-        if (members.contains(member)) return;
-        removeInvitedMember(member.getUuid());
-        members.add(member);
-        member.setJoinDate(System.currentTimeMillis());
+        addMember(new GuildMember(player.getUniqueId(), guildHandler.getLowestGuildRole()));
     }
 
     /**
      * Invite a member by guild code
-     * @param guildMember
+     *
+     * <p>The same registration as {@link #addMember(OfflinePlayer, GuildHandler)}: a player is only
+     * in the guild once, a stale invite is dropped, and the member carries the date they joined.
+     * The role is whatever the caller decided, which for a code is the lowest role in the guild.
+     *
+     * @param guildMember the member to add
      */
     public void addMemberByCode(GuildMember guildMember) {
-        members.add(guildMember);
+        addMember(guildMember);
+    }
+
+    /**
+     * Registers a member with the guild.
+     *
+     * <p>A member is identified by their uuid, not by the object: {@link GuildMember} does not
+     * override {@code equals}, so comparing objects would never find the member that is already in
+     * the guild and every caller would be free to add a second entry for the same player.
+     *
+     * @param member the member to register
+     */
+    private void addMember(GuildMember member) {
+        if (getMember(member.getUuid()) != null) return;
+        removeInvitedMember(member.getUuid());
+        members.add(member);
+        if (member.getJoinDate() == 0L) {
+            member.setJoinDate(System.currentTimeMillis());
+        }
     }
 
     /**
@@ -435,7 +455,7 @@
      * @param key the message to send
      * @param replacements any args we need to handle
      */
     public void sendMessage(CommandManager manager, Messages key, String... replacements) {
         getOnlineMembers().forEach(m -> {
             final Player player = Bukkit.getPlayer(m.getUuid());
             if (player != null) {

src/main/java/me/glaremasters/guilds/guild/GuildHandler.java

@@ -6,6 +6,7 @@
 import co.aikar.commands.ACFUtil;
 import co.aikar.commands.PaperCommandManager;
 import me.glaremasters.guilds.Guilds;
+import me.glaremasters.guilds.api.events.GuildJoinEvent;
 import me.glaremasters.guilds.configuration.sections.GuildSettings;
 import me.glaremasters.guilds.configuration.sections.GuildVaultSettings;
 import me.glaremasters.guilds.configuration.sections.PluginSettings;
@@ -1107,6 +1108,11 @@ public void removeRolePermsFromAll(final Permission permission, final Guild guil
     /**
      * Handle inviting player when redeeming a code
      *
+     * <p>A code is a way into a guild, so this does what {@code /guild accept} does: it fires the
+     * cancellable {@link GuildJoinEvent} before anything is consumed, registers the member the
+     * same way an invite does, and hands the member the permissions their guild and role grant.
+     * A plugin that cancels the join event stops the redeem, and the code keeps its use.
+     *
      * @param manager command manager
      * @param player  the player redeeming the code
      * @param guild   the guild they are trying to join
@@ -1116,11 +1122,24 @@ public void handleInvite(PaperCommandManager manager, Player player, Guild guild
         if (code.getUses() <= 0)
             ACFUtil.sneaky(new ExpectationNotMet(Messages.CODES__OUT));
 
+        final GuildJoinEvent event = new GuildJoinEvent(player, guild);
+        Bukkit.getPluginManager().callEvent(event);
+
+        if (event.isCancelled()) {
+            return;
+        }
+
         code.addRedeemer(player);
 
-        guild.addMemberByCode(new GuildMember(player.getUniqueId(), getLowestGuildRole()));
+        guild.addMember(player, this);
         addToMemberCache(player.getUniqueId(), guild.getId());
 
+        final Permission permission = getGuildsPlugin().getPermissions();
+        if (permission != null) {
+            addGuildPerms(permission, player);
+            addRolePerm(permission, player);
+        }
+
         if (ClaimUtils.isEnabled(settingsManager)) {
             WorldGuardWrapper wrapper = WorldGuardWrapper.getInstance();
             ClaimUtils.getGuildClaim(wrapper, player, guild).ifPresent(region -> ClaimUtils.addMember(region, player));

src/main/kotlin/me/glaremasters/guilds/commands/codes/CommandCode.kt

@@ -15,6 +15,8 @@ import co.aikar.commands.annotation.Syntax
 import co.aikar.commands.annotation.Values
 import me.glaremasters.guilds.Guilds
 import me.glaremasters.guilds.configuration.sections.CodeSettings
+import me.glaremasters.guilds.cooldowns.Cooldown
+import me.glaremasters.guilds.cooldowns.CooldownHandler
 import me.glaremasters.guilds.exceptions.ExpectationNotMet
 import me.glaremasters.guilds.guild.Guild
 import me.glaremasters.guilds.guild.GuildHandler
@@ -32,6 +34,8 @@ internal class CommandCode : BaseCommand() {
     lateinit var settingsManager: SettingsManager
     @Dependency
     lateinit var guildHandler: GuildHandler
+    @Dependency
+    lateinit var cooldownHandler: CooldownHandler
 
     @Subcommand("code create")
     @Description("{@@descriptions.code-create}")
@@ -98,6 +102,14 @@ internal class CommandCode : BaseCommand() {
     fun redeem(@Conditions("NoGuild") player: Player, code: String) {
         val guild = guildHandler.getGuildByCode(code) ?: throw ExpectationNotMet(Messages.CODES__INVALID_CODE)
 
+        // A code is a way into a guild, so it is refused while the join cooldown runs, the same
+        // way `/guild accept` and `/guild create` refuse it.
+        val cooldown = Cooldown.Type.Join.name
+
+        if (cooldownHandler.hasCooldown(cooldown, player.uniqueId)) {
+            throw ExpectationNotMet(Messages.ACCEPT__COOLDOWN, "{amount}", cooldownHandler.getRemaining(cooldown, player.uniqueId).toString())
+        }
+
         if (guildHandler.checkIfFull(guild)) {
             throw ExpectationNotMet(Messages.ACCEPT__GUILD_FULL)
         }

src/test/e2e/tests/codes.spec.ts

@@ -1,7 +1,8 @@
 import { describe, expect, test } from '@plugwright/runner';
 import { commandWithout, expectReceived } from '../plugins/support/assertions.js';
 import { MSG } from '../plugins/support/expected.js';
-import { guildList } from '../plugins/support/state.js';
+import { ROLE } from '../plugins/support/fixtures.js';
+import { guildList, guildMembers } from '../plugins/support/state.js';
 import { stripColors } from '../plugins/support/text.js';
 
 /**
@@ -63,6 +64,65 @@ test('a code can be redeemed and puts the player in the guild', async ({ player,
 
     const entry = (await guildList(joiner)).find(item => item.name === guild)!;
     expect(entry.memberCount).toBe(2);
+
+    // The roster is the one an invited member appears on, with the same role.
+    const roster = await guildMembers(player);
+    expect(roster.map(member => member.name).sort()).toEqual([player.username, joiner.username].sort());
+    expect(roster.find(member => member.name === joiner.username)!.role).toBe(ROLE.member);
+});
+
+test('a player who joins by code gets the permissions their guild grants them', async ({
+    player,
+    createPlayer,
+    guilds,
+}) => {
+    const guild = await guilds.createGuild(player);
+    const created = player.getMessageBufferIndex();
+    await guilds.run(player, '/guild code create 2');
+    await expectReceived(player, MSG.codeCreated, created);
+    const code = parseCode(player.messageBuffer.slice(created).join('\n'))!;
+
+    const joiner = await createPlayer({ username: guilds.playerName() });
+    const joining = joiner.getMessageBufferIndex();
+    await guilds.run(joiner, `/guild code redeem ${code}`);
+    await expectReceived(joiner, MSG.codeJoined(guild), joining);
+
+    // tiers.yml grants guilds.command.admin to every member, and the runner denies that node on
+    // LuckPerms' default group, so an admin command answering at all is the proof that the guild
+    // handed the node over. LuckPerms applies it to an online player on its own schedule, so the
+    // probe is re-issued until it works.
+    const asMember = joiner.getMessageBufferIndex();
+    try {
+        await guilds.awaitEffect(joiner, `/guild admin motd ${guild}`, MSG.motdNotSet);
+    } catch {
+        const told = joiner.messageBuffer.slice(asMember).map(stripColors).join(' | ');
+        throw new Error(`the joiner never got the permissions their guild grants; they were told: ${told}`);
+    }
+});
+
+test('a code cannot be redeemed while the join cooldown is running', async ({ player, createPlayer, guilds }) => {
+    const guild = await guilds.createGuild(player);
+    const created = player.getMessageBufferIndex();
+    await guilds.run(player, '/guild code create 2');
+    await expectReceived(player, MSG.codeCreated, created);
+    const code = parseCode(player.messageBuffer.slice(created).join('\n'))!;
+
+    const joiner = await createPlayer({ username: guilds.playerName() });
+    const joining = joiner.getMessageBufferIndex();
+    await guilds.run(joiner, `/guild code redeem ${code}`);
+    await expectReceived(joiner, MSG.codeJoined(guild), joining);
+
+    // Leaving starts the join cooldown, and joining a guild is what that cooldown is about. A code
+    // is a way into a guild, so it is refused the same way an invite is.
+    await guilds.run(joiner, '/guild leave');
+    await expect(joiner).toHaveReceivedMessage(MSG.leaveWarning);
+    await guilds.confirm(joiner);
+    await expect(joiner).toHaveReceivedMessage(MSG.leaveSuccess);
+
+    const blocked = joiner.getMessageBufferIndex();
+    await guilds.run(joiner, `/guild code redeem ${code}`);
+    await expectReceived(joiner, MSG.acceptCooldown, blocked);
+    await expect(joiner).not.toHaveReceivedMessage(MSG.codeJoined(guild), { since: blocked });
 });
 
 test('an invalid code is refused', async ({ player, createPlayer, guilds }) => {

src/test/java/me/glaremasters/guilds/guild/GuildCodeTest.java

@@ -0,0 +1,81 @@
+package me.glaremasters.guilds.guild;
+
+import org.bukkit.entity.Player;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+
+/**
+ * Covers what one use of an invite code means.
+ *
+ * <p>A code is a counter, a list of the players who spent it, and the player who made it. Those
+ * three things are the whole of its stored shape, and the code redeem path is not allowed to
+ * change any of them: a redeem spends exactly one use, records exactly one redeemer, and the
+ * creator and the remaining count are what the next reader sees.
+ */
+class GuildCodeTest {
+
+    private static final String ID = "ABC1234";
+
+    private GuildCode code;
+    private UUID creatorId;
+
+    @BeforeEach
+    void setUp() {
+        creatorId = UUID.randomUUID();
+        code = new GuildCode(ID, 3, creatorId, new ArrayList<>());
+    }
+
+    @Test
+    @DisplayName("a redeem spends one use and records one redeemer")
+    void aRedeemSpendsOneUse() {
+        final UUID redeemer = UUID.randomUUID();
+
+        code.addRedeemer(player(redeemer));
+
+        assertEquals(2, code.getUses());
+        assertEquals(Arrays.asList(redeemer), code.getRedeemers());
+    }
+
+    @Test
+    @DisplayName("a redeem leaves the creator and the id alone")
+    void aRedeemKeepsTheCreator() {
+        code.addRedeemer(player(UUID.randomUUID()));
+
+        assertEquals(ID, code.getId());
+        assertSame(creatorId, code.getCreator());
+    }
+
+    @Test
+    @DisplayName("the same player can spend the code twice, and it costs two uses")
+    void theSamePlayerCanRedeemTwice() {
+        // Documented, not endorsed: a player who leaves the guild they joined with a code can
+        // come back and spend the code again, once the join cooldown has run out. The code
+        // charges them for it, and the redeemers list names them twice.
+        final UUID redeemer = UUID.randomUUID();
+        final Player player = player(redeemer);
+
+        code.addRedeemer(player);
+        code.addRedeemer(player);
+
+        assertEquals(1, code.getUses());
+        assertEquals(Arrays.asList(redeemer, redeemer), code.getRedeemers());
+    }
+
+    /**
+     * A player with the given uuid.
+     */
+    private static Player player(final UUID uuid) {
+        final Player player = Mockito.mock(Player.class);
+        Mockito.when(player.getUniqueId()).thenReturn(uuid);
+        return player;
+    }
+}