fix unit upgrade not considering cost (#1434)

## Description:

bugfix: The price was not considered when checking it upgrading a unit
was allowed.
Also refacted and cleaned up the code a bit.

## Please complete the following:

- [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 understand that submitting code with bugs that could have been
caught through manual testing blocks releases and new features for all
contributors

## Please put your Discord username so you can be contacted if a bug or
regression is found:

evan

src/core/execution/UpgradeStructureExecution.ts

@@ -18,12 +18,11 @@ export class UpgradeStructureExecution implements Execution {
       console.warn(`structure is undefined`);
       return;
     }
-    if (!this.structure.info().upgradable) {
-      console.warn(`unit type ${this.structure} cannot be upgraded`);
-      return;
-    }
-    this.cost = this.structure.info().cost(this.player);
-    if (this.player.gold() < this.cost) {
+
+    if (!this.player.canUpgradeUnit(this.structure.type())) {
+      console.warn(
+        `[UpgradeStructureExecution] unit type ${this.structure.type()} cannot be upgraded`,
+      );
       return;
     }
     this.player.upgradeUnit(this.structure);

src/core/game/Game.ts

@@ -530,8 +530,13 @@ export interface Player {
     params: UnitParams<T>,
   ): Unit;
 
+  // Returns the existing unit that can be upgraded,
+  // or false if it cannot be upgraded.
+  // New units of the same type can upgrade existing units.
+  // e.g. if a place a new city here, can it upgrade an existing city?
+  findUnitToUpgrade(type: UnitType, targetTile: TileRef): Unit | false;
+  canUpgradeUnit(unitType: UnitType): boolean;
   upgradeUnit(unit: Unit): void;
-
   captureUnit(unit: Unit): void;
 
   // Relations & Diplomacy

src/core/game/PlayerImpl.ts

@@ -814,25 +814,32 @@ export class PlayerImpl implements Player {
     return b;
   }
 
-  // Returns the existing unit that can be upgraded,
-  // or false if it cannot be upgraded.
-  // New units of the same type can upgrade existing units.
-  // e.g. if a place a new city here, can it upgrade an existing city?
-  private canUpgradeExistingUnit(
-    type: UnitType,
-    targetTile: TileRef,
-  ): Unit | false {
-    if (!this.mg.config().unitInfo(type).upgradable) {
-      return false;
-    }
+  public findUnitToUpgrade(type: UnitType, targetTile: TileRef): Unit | false {
     const range = this.mg.config().structureMinDist();
     const existing = this.mg
       .nearbyUnits(targetTile, range, type)
       .sort((a, b) => a.distSquared - b.distSquared);
-    if (existing.length > 0) {
-      return existing[0].unit;
+    if (existing.length === 0) {
+      return false;
     }
-    return false;
+    const unit = existing[0].unit;
+    if (!this.canUpgradeUnit(unit.type())) {
+      return false;
+    }
+    return unit;
+  }
+
+  public canUpgradeUnit(unitType: UnitType): boolean {
+    if (!this.mg.config().unitInfo(unitType).upgradable) {
+      return false;
+    }
+    if (this.mg.config().isUnitDisabled(unitType)) {
+      return false;
+    }
+    if (this._gold < this.mg.config().unitInfo(unitType).cost(this)) {
+      return false;
+    }
+    return true;
   }
 
   upgradeUnit(unit: Unit) {
@@ -847,7 +854,7 @@ export class PlayerImpl implements Player {
     return Object.values(UnitType).map((u) => {
       let canUpgrade: number | false = false;
       if (!this.mg.inSpawnPhase()) {
-        const existingUnit = this.canUpgradeExistingUnit(u, tile);
+        const existingUnit = this.findUnitToUpgrade(u, tile);
         if (existingUnit !== false) {
           canUpgrade = existingUnit.id();
         }

tests/PlayerImpl.test.ts

@@ -15,7 +15,6 @@ describe("PlayerImpl", () => {
     game = await setup(
       "plains",
       {
-        infiniteGold: true,
         instantBuild: true,
       },
       [new PlayerInfo("player", PlayerType.Human, null, "player_id")],
@@ -26,6 +25,9 @@ describe("PlayerImpl", () => {
     }
 
     player = game.player("player_id");
+    player.addGold(BigInt(1000000));
+
+    game.config().structureMinDist = () => 10;
   });
 
   test("City can be upgraded", () => {
@@ -45,4 +47,34 @@ describe("PlayerImpl", () => {
     expect(buDefensePost).toBeDefined();
     expect(buDefensePost!.canUpgrade).toBeFalsy();
   });
+
+  test("City can be upgraded from another city", () => {
+    const city = player.buildUnit(UnitType.City, game.ref(0, 0), {});
+    const cityToUpgrade = player.findUnitToUpgrade(
+      UnitType.City,
+      game.ref(0, 1),
+    );
+    expect(cityToUpgrade).toBeTruthy();
+    if (cityToUpgrade === false) {
+      return;
+    }
+    expect(cityToUpgrade.id()).toBe(city.id());
+  });
+  test("City cannot be upgraded when too far away", () => {
+    player.buildUnit(UnitType.City, game.ref(0, 0), {});
+    const cityToUpgrade = player.findUnitToUpgrade(
+      UnitType.City,
+      game.ref(50, 50),
+    );
+    expect(cityToUpgrade).toBe(false);
+  });
+  test("Unit cannot be upgraded when not enough gold", () => {
+    player.buildUnit(UnitType.City, game.ref(0, 0), {});
+    player.removeGold(BigInt(1000000));
+    const cityToUpgrade = player.findUnitToUpgrade(
+      UnitType.City,
+      game.ref(0, 1),
+    );
+    expect(cityToUpgrade).toBe(false);
+  });
 });