Add configurable wartime block lists for placeable, breakable, and interactable blocks - #1955
dmccoystephenson with Copilot wants to merge 4 commits into
Conversation
…teractable blocks Add war.items.placeable, war.items.breakable, and war.items.interactable config lists that allow server admins to specify which blocks can be placed, broken, or interacted with in enemy territory during war. - Add isWartimeBlockActionAllowed() to MfClaimService - Update BlockBreakListener to check war.items.breakable list - Update BlockPlaceListener to check war.items.placeable list - Update PlayerInteractListener to check war.items.interactable list - Add config entries to config.yml and document in CONFIG.md - Add tests for new wartime block action functionality Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com> Agent-Logs-Url: https://github.com/Dans-Plugins/Medieval-Factions/sessions/9a502860-9de7-449d-b5ae-49699b06b841
…ockPlaceListener Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com> Agent-Logs-Url: https://github.com/Dans-Plugins/Medieval-Factions/sessions/9a502860-9de7-449d-b5ae-49699b06b841
| `when`(claimService.isInteractionAllowed(mfPlayer.id, claim)).thenReturn(false) | ||
| `when`(claimService.isWartimeBlockActionAllowed(mfPlayer.id, claim, block.type.name, "war.items.breakable")).thenReturn(false) | ||
|
|
There was a problem hiding this comment.
block.type is never stubbed in this test fixture (see TestUtils.createMockBlock), so the new use of block.type.name will evaluate to null and can throw at stubbing time and/or when the listener executes. Stub block.type to a real Material (e.g., STONE) before referencing .name, or use a constant material name string in the mock setup.
| `when`(claimService.isInteractionAllowed(mfPlayer.id, claim)).thenReturn(false) | ||
| `when`(claimService.isWartimeBlockActionAllowed(mfPlayer.id, claim, block.type.name, "war.items.breakable")).thenReturn(true) | ||
|
|
There was a problem hiding this comment.
This test still depends on block.type.name, but block.type is not stubbed in the fixture. Because the listener now calls event.block.type.name to consult the wartime breakable list, the test will NPE unless you stub block.type to a concrete Material before invoking uut.onBlockBreak(event).
| fun isWartimeBlockActionAllowed(playerId: MfPlayerId, claim: MfClaimedChunk, blockTypeName: String, configPath: String): Boolean { | ||
| val allowedBlocks = plugin.config.getStringList(configPath) | ||
| if (!allowedBlocks.contains(blockTypeName)) return false | ||
| return isAtWarWith(playerId, claim.factionId) |
There was a problem hiding this comment.
isWartimeBlockActionAllowed pulls getStringList(configPath) and does a linear contains on every event call. Since this runs on hot interaction/break/place paths, consider normalizing and caching the configured materials (e.g., a Set<Material> per list) and/or at least trimming/uppercasing entries once; this also lets you warn on invalid material names (similar to MfGateService.loadRestrictedBlocksFromConfig).
| val isLadderAllowed = event.block.type == Material.LADDER && plugin.config.getBoolean("factions.laddersPlaceableInEnemyFactionTerritory") | ||
| val isInPlaceableList = claimService.isWartimeBlockActionAllowed(mfPlayer.id, claim, event.block.type.name, "war.items.placeable") | ||
| if (!isLadderAllowed && !isInPlaceableList) { | ||
| event.isCancelled = true | ||
| event.player.sendMessage("$RED${plugin.language["CannotPlaceBlockInFactionTerritory", claimFaction.name]}") | ||
| } |
There was a problem hiding this comment.
The new wartime placeable-list behavior (war.items.placeable) is introduced here, but there are no unit tests covering claimed-territory placement allow/deny based on this list (BlockPlaceListenerTest currently only covers gate/wilderness paths). Add tests for (1) a block in the placeable list being allowed during war and (2) a block not in the list being denied (including interaction with the ladder-specific toggle).
| } | ||
|
|
||
| @Test | ||
| fun onPlayerInteract_WartimeNonInteractableBlock_ShouldBlockInteraction() { |
There was a problem hiding this comment.
Test name is misleading: the clicked block in this fixture defaults to Material.OAK_DOOR (interactable). Consider renaming to something like "WartimeNonAllowedBlock" / "WartimeBlockNotInInteractableList" so the name reflects what the test actually verifies (deny when isWartimeBlockActionAllowed returns false).
| fun onPlayerInteract_WartimeNonInteractableBlock_ShouldBlockInteraction() { | |
| fun onPlayerInteract_WartimeBlockActionNotAllowed_ShouldBlockInteraction() { |
|
@copilot You are a coding agent tasked with resolving all open comments and review threads Instructions1. Discover all comments
2. For each comment, do the following:
3. Types of issues to handle:
4. Do NOT:
After making changes
Context
|
Currently wartime block control is all-or-nothing:
enableWartimeBlockDestructionallows destroying ALL blocks, andladdersPlaceableInEnemyFactionTerritoryonly covers ladders. There's no way to allow e.g. scaffolding placement + door interaction without opening up full block destruction.Adds three config lists under
war.itemsthat grant granular per-block-type permissions in enemy territory during war:Changes
MfClaimService— NewisWartimeBlockActionAllowed(playerId, claim, blockTypeName, configPath)method. Extracted shared war-status logic into privateisAtWarWith()helper (deduplicates fromisWartimeLadderPlacementAllowed).BlockBreakListener— Falls through towar.items.breakablecheck before cancellingBlockPlaceListener— Checkswar.items.placeablealongside existing ladder-specific logicPlayerInteractListener— Checkswar.items.interactableafter ladder placement checkconfig.yml— Newwar.itemssection with three empty lists (backwards compatible)CONFIG.md— Documents all three options with examplesThese lists work alongside the existing boolean toggles —
enableWartimeBlockDestructionstill overrides everything, andladdersPlaceableInEnemyFactionTerritorystill works independently.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
hub.spigotmc.org/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.invoke=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.base/java.nio.charset=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED -XX:MaxMetaspaceSize=256m(dns block)jitpack.io/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.invoke=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.base/java.nio.charset=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED -XX:MaxMetaspaceSize=256m(dns block)repo.dansplugins.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.invoke=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.base/java.nio.charset=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED -XX:MaxMetaspaceSize=256m(dns block)repo.extendedclip.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.invoke=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.base/java.nio.charset=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED -XX:MaxMetaspaceSize=256m(dns block)repo.mikeprimm.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.invoke=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.base/java.nio.charset=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED -XX:MaxMetaspaceSize=256m(dns block)repo.rpkit.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.invoke=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.prefs/java.util.prefs=ALL-UNNAMED --add-opens java.base/java.nio.charset=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED -XX:MaxMetaspaceSize=256m(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.