Cancel alliance requests if the recipient attacks (#1733)

Problem: attacking a player right before accepting an alliance request
is very effective since the requester can't fight back or reclaim his
territory without canceling the alliance and being penalized with the
traitor debuff.

Change:
- Attacking a player after he requested an alliance automatically
rejects the request
- No changes to existing attacks in both directions, only new attacks
affect the request

- [x] I have added screenshots for all UI updates
- [x] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [x] I have added relevant tests to the test directory
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced
- [x] I have read and accepted the CLA agreement (only required once).

regression is found:

IngloriousTom

src/client/graphics/layers/EventsDisplay.ts

@@ -470,7 +470,24 @@ export class EventsDisplay extends LitElement implements Layer {
 
   onAllianceRequestReplyEvent(update: AllianceRequestReplyUpdate) {
     const myPlayer = this.game.myPlayer();
-    if (!myPlayer || update.request.requestorID !== myPlayer.smallID()) {
+    if (!myPlayer) {
+      return;
+    }
+    // myPlayer can deny alliances without clicking on the button
+    if (update.request.recipientID === myPlayer.smallID()) {
+      // Remove alliance requests whose requestors are the same as the reply's requestor
+      // Noop unless the request was denied through other means (e.g attacking the requestor)
+      this.events = this.events.filter(
+        (event) =>
+          !(
+            event.type === MessageType.ALLIANCE_REQUEST &&
+            event.focusID === update.request.requestorID
+          ),
+      );
+      this.requestUpdate();
+      return;
+    }
+    if (update.request.requestorID !== myPlayer.smallID()) {
       return;
     }
 

src/core/execution/AttackExecution.ts

@@ -62,6 +62,12 @@ export class AttackExecution implements Execution {
         ? mg.terraNullius()
         : mg.player(this._targetID);
 
+    if (this._owner === this.target) {
+      console.error(`Player ${this._owner} cannot attack itself`);
+      this.active = false;
+      return;
+    }
+
     if (this.target && this.target.isPlayer()) {
       const targetPlayer = this.target as Player;
       if (
@@ -70,15 +76,10 @@ export class AttackExecution implements Execution {
       ) {
         // Don't let bots embargo since they can't trade anyway.
         targetPlayer.addEmbargo(this._owner, true);
+        this.rejectIncomingAllianceRequests(targetPlayer);
       }
     }
 
-    if (this._owner === this.target) {
-      console.error(`Player ${this._owner} cannot attack itself`);
-      this.active = false;
-      return;
-    }
-
     if (this.target.isPlayer()) {
       if (
         this.mg.config().numSpawnPhaseTurns() +
@@ -295,6 +296,15 @@ export class AttackExecution implements Execution {
     }
   }
 
+  private rejectIncomingAllianceRequests(target: Player) {
+    const request = this._owner
+      .incomingAllianceRequests()
+      .find((ar) => ar.requestor() === target);
+    if (request !== undefined) {
+      request.reject();
+    }
+  }
+
   private addNeighbors(tile: TileRef) {
     if (this.attack === null) {
       throw new Error("Attack not initialized");

tests/Attack.test.ts

@@ -113,9 +113,22 @@ describe("Attack", () => {
   });
 });
 
+let playerA: Player;
+let playerB: Player;
+
+function addPlayerToGame(
+  playerInfo: PlayerInfo,
+  game: Game,
+  tile: TileRef,
+): Player {
+  game.addPlayer(playerInfo);
+  game.addExecution(new SpawnExecution(playerInfo, tile));
+  return game.player(playerInfo.id);
+}
+
 describe("Attack race condition with alliance requests", () => {
-  it("should not mark attacker as traitor when alliance is formed after attack starts", async () => {
-    const game = await setup("ocean_and_land", {
+  beforeEach(async () => {
+    game = await setup("ocean_and_land", {
       infiniteGold: true,
       instantBuild: true,
       infiniteTroops: true,
@@ -127,32 +140,22 @@ describe("Attack race condition with alliance requests", () => {
       null,
       "playerA_id",
     );
+    playerA = addPlayerToGame(playerAInfo, game, game.ref(0, 10));
+
     const playerBInfo = new PlayerInfo(
       "playerB",
       PlayerType.Human,
       null,
       "playerB_id",
     );
-
-    game.addPlayer(playerAInfo);
-    game.addPlayer(playerBInfo);
-
-    const playerA = game.player(playerAInfo.id);
-    const playerB = game.player(playerBInfo.id);
-
-    // Spawn both players
-    const spawnA = game.ref(0, 10);
-    const spawnB = game.ref(0, 15);
-
-    game.addExecution(
-      new SpawnExecution(playerAInfo, spawnA),
-      new SpawnExecution(playerBInfo, spawnB),
-    );
+    playerB = addPlayerToGame(playerBInfo, game, game.ref(0, 10));
 
     while (game.inSpawnPhase()) {
       game.executeNextTick();
     }
+  });
 
+  it("should not mark attacker as traitor when alliance is formed after attack starts", async () => {
     // Player A sends alliance request to Player B
     const allianceRequest = playerA.createAllianceRequest(playerB);
     expect(allianceRequest).not.toBeNull();
@@ -194,44 +197,6 @@ describe("Attack race condition with alliance requests", () => {
   });
 
   it("should mark attacker as traitor when alliance existed before attack", async () => {
-    const game = await setup("ocean_and_land", {
-      infiniteGold: true,
-      instantBuild: true,
-      infiniteTroops: true,
-    });
-
-    const playerAInfo = new PlayerInfo(
-      "playerA",
-      PlayerType.Human,
-      null,
-      "playerA_id",
-    );
-    const playerBInfo = new PlayerInfo(
-      "playerB",
-      PlayerType.Human,
-      null,
-      "playerB_id",
-    );
-
-    game.addPlayer(playerAInfo);
-    game.addPlayer(playerBInfo);
-
-    const playerA = game.player(playerAInfo.id);
-    const playerB = game.player(playerBInfo.id);
-
-    // Spawn both players
-    const spawnA = game.ref(0, 10);
-    const spawnB = game.ref(0, 15);
-
-    game.addExecution(
-      new SpawnExecution(playerAInfo, spawnA),
-      new SpawnExecution(playerBInfo, spawnB),
-    );
-
-    while (game.inSpawnPhase()) {
-      game.executeNextTick();
-    }
-
     // Create an alliance between Player A and Player B
     const allianceRequest = playerA.createAllianceRequest(playerB);
     if (allianceRequest) {
@@ -255,4 +220,67 @@ describe("Attack race condition with alliance requests", () => {
     // Player A should be marked as traitor because they attacked an ally
     expect(playerA.isTraitor()).toBe(true);
   });
+
+  test("should cancel alliance requests if the recipient attacks", async () => {
+    // Player A sends alliance request to Player B
+    const allianceRequest = playerA.createAllianceRequest(playerB);
+    expect(allianceRequest).not.toBeNull();
+    expect(playerB.incomingAllianceRequests()).toHaveLength(1);
+
+    // Player B attacks Player A
+    const attackExecution = new AttackExecution(
+      null,
+      playerB,
+      playerA.id(),
+      null,
+    );
+    game.addExecution(attackExecution);
+
+    // Execute a few ticks to process the attacks
+    for (let i = 0; i < 5; i++) {
+      game.executeNextTick();
+    }
+    // Alliance request should be denied since player B attacked
+    expect(playerA.outgoingAllianceRequests()).toHaveLength(0);
+    expect(playerB.incomingAllianceRequests()).toHaveLength(0);
+  });
+
+  test("should cancel the proper alliance request among many", async () => {
+    // Add a new player to have more alliance requests
+    const playerCInfo = new PlayerInfo(
+      "playerB",
+      PlayerType.Human,
+      null,
+      "playerB_id",
+    );
+    const playerC = addPlayerToGame(playerCInfo, game, game.ref(10, 10));
+
+    // Player A sends alliance request to Player B
+    const allianceRequestAtoB = playerA.createAllianceRequest(playerB);
+    expect(allianceRequestAtoB).not.toBeNull();
+
+    // Player C also sends alliance request to Player B
+    const allianceRequestCtoB = playerC.createAllianceRequest(playerB);
+    expect(allianceRequestCtoB).not.toBeNull();
+
+    expect(playerB.incomingAllianceRequests()).toHaveLength(2);
+
+    // Player B attacks Player A
+    const attackExecution = new AttackExecution(
+      null,
+      playerB,
+      playerA.id(),
+      null,
+    );
+    game.addExecution(attackExecution);
+
+    // Execute a few ticks to process the attacks
+    for (let i = 0; i < 5; i++) {
+      game.executeNextTick();
+    }
+    // Alliance request A->B should be denied since player B attacked
+    expect(playerA.outgoingAllianceRequests()).toHaveLength(0);
+    // However C->B should remain
+    expect(playerB.incomingAllianceRequests()).toHaveLength(1);
+  });
 });