Fix 404 when a player doesnt have federation (#113)

modules/db/src/main/scala/Db.scala

@@ -153,10 +153,10 @@ private object Sql:
        """.command
 
   lazy val playerById: Query[PlayerId, PlayerInfo] =
-    sql"$allPlayersFragment AND p.id = $int4".query(playerInfo)
+    sql"$allPlayersFragment WHERE p.id = $int4".query(playerInfo)
 
   lazy val playersByFederationId: Query[FederationId, PlayerInfo] =
-    sql"$allPlayersFragment AND p.federation_id = $text".query(playerInfo)
+    sql"$allPlayersFragment WHERE p.federation_id = $text".query(playerInfo)
 
   lazy val upsertFederation: Command[NewFederation] =
     sql"$insertIntoFederation VALUES ($newFederation) $onConflictDoNothing".command
@@ -172,16 +172,17 @@ private object Sql:
        """.query(federationInfo)
 
   def allPlayers(sorting: Sorting, page: Pagination, filter: PlayerFilter): AppliedFragment =
-    allPlayersFragment(Void) |+| filterFragment(filter) |+|
+    allPlayersFragment(Void) |+| filterFragment(filter).fold(void)(where |+| _) |+|
       sortingFragment(sorting) |+| pagingFragment(page)
 
   def playersByName(name: String, sorting: Sorting, page: Pagination, filter: PlayerFilter): AppliedFragment =
-    allPlayersFragment(Void) |+| nameLikeFragment(name) |+| filterFragment(filter) |+|
-      sortingFragment(sorting) |+| pagingFragment(page)
+    val whereQuery =
+      where |+| filterFragment(filter).fold(nameLikeFragment(name))(nameLikeFragment(name) |+| and |+| _)
+    allPlayersFragment(Void) |+| whereQuery |+| sortingFragment(sorting) |+| pagingFragment(page)
 
   def playersByIds(n: Int): Fragment[List[Int]] =
     val ids = int4.values.list(n)
-    sql"$allPlayersFragment AND p.id IN ($ids)"
+    sql"$allPlayersFragment WHERE p.id IN ($ids)"
 
   def allFederationsSummary(paging: Pagination): AppliedFragment =
     allFederationsSummaryFragment(Void) |+| pagingFragment(paging)
@@ -190,28 +191,38 @@ private object Sql:
     sql"""$allFederationsSummaryFragment
         WHERE id = $text""".query(federationSummary)
 
-  private val void: AppliedFragment = sql"".apply(Void)
+  private val void: AppliedFragment  = sql"".apply(Void)
+  private val and: AppliedFragment   = sql"AND ".apply(Void)
+  private val where: AppliedFragment = sql"WHERE ".apply(Void)
 
-  private def between(column: String, min: Option[Rating], max: Option[Rating]): AppliedFragment =
+  private def between(column: String, range: RatingRange): Option[AppliedFragment] =
     val _column = s"p.$column"
-    (min, max) match
+    (range.min, range.max) match
       case (Some(min), Some(max)) =>
         sql"""
-            AND #$_column BETWEEN ${int4} AND ${int4}""".apply(min, max)
+            #$_column BETWEEN ${int4} AND ${int4}""".apply(min, max).some
       case (Some(min), None) =>
         sql"""
-            AND #$_column >= ${int4}""".apply(min)
+            #$_column >= ${int4}""".apply(min).some
       case (None, Some(max)) =>
         sql"""
-            AND #$_column <= ${int4}""".apply(max)
-      case (None, None) => void
+            #$_column <= ${int4}""".apply(max).some
+      case (None, None) => none
+
+  private def filterFragment(filter: PlayerFilter): Option[AppliedFragment] =
+    val bw: Option[AppliedFragment] = List(
+      between("standard", filter.standard),
+      between("rapid", filter.rapid),
+      between("blitz", filter.blitz)
+    ).flatten.foldLeft(none)((a, b) => a.fold(b)(_ |+| and |+| b).some)
 
-  private def filterFragment(filter: PlayerFilter): AppliedFragment =
-    val bw = between("standard", filter.standard.min, filter.standard.max) |+|
-      between("rapid", filter.rapid.min, filter.rapid.max) |+|
-      between("blitz", filter.blitz.min, filter.blitz.max)
-    val f = filter.isActive.fold(bw)(bw |+| filterActive(_))
-    filter.federationId.fold(f)(f |+| federationIdFragment(_))
+    val f = filter.isActive.fold(bw): x =>
+      val a = filterActive(x)
+      bw.fold(a)(_ |+| a).some
+
+    filter.federationId.fold(f): x =>
+      val a = federationIdFragment(x)
+      f.fold(a)(_ |+| a).some
 
   private lazy val filterActive: Fragment[Boolean] =
     sql"""
@@ -231,7 +242,7 @@ private object Sql:
 
   private def nameLikeFragment(name: String): AppliedFragment =
     sql"""
-        AND p.name % $text""".apply(name)
+        p.name % $text""".apply(name)
 
   private def pagingFragment(page: Pagination): AppliedFragment =
     sql"""
@@ -250,8 +261,8 @@ private object Sql:
   private lazy val allPlayersFragment: Fragment[Void] =
     sql"""
         SELECT p.id, p.name, p.title, p.women_title, p.other_titles, p.standard, p.rapid, p.blitz, p.sex, p.birth_year, p.active, p.updated_at, p.created_at, f.id, f.name
-        FROM players AS p, federations AS f
-        WHERE p.federation_id = f.id"""
+        FROM players AS p LEFT JOIN federations AS f ON p.federation_id = f.id
+      """
 
   private lazy val allFederationsSummaryFragment: Fragment[Void] =
     sql"""

modules/db/src/test/scala/DbSuite.scala

@@ -20,7 +20,7 @@ object DbSuite extends SimpleIOSuite:
     Containers.createResource.map(x => Db(x.postgres) -> KVStore(x.postgres))
   private def resource: Resource[IO, Db] = resourceP.map(_._1)
 
-  val newPlayer = NewPlayer(
+  val newPlayer1 = NewPlayer(
     1,
     "John",
     Title.GM.some,
@@ -29,6 +29,20 @@ object DbSuite extends SimpleIOSuite:
     2700.some,
     2700.some,
     2700.some,
+    Sex.Male.some,
+    1990.some,
+    true
+  )
+
+  val newPlayer2 = NewPlayer(
+    2,
+    "Jane",
+    Title.GM.some,
+    Title.WGM.some,
+    List(OtherTitle.IA, OtherTitle.DI),
+    2700.some,
+    2700.some,
+    2700.some,
     Sex.Female.some,
     1990.some,
     true
@@ -39,32 +53,44 @@ object DbSuite extends SimpleIOSuite:
     "FIDE"
   )
 
+  val newPlayers = List(newPlayer1 -> newFederation.some, newPlayer2 -> none)
+
   test("create player success"):
     resource
-      .use(_.upsert(newPlayer, newFederation.some).map(_ => expect(true)))
+      .use(_.upsert(newPlayer1, newFederation.some).map(_ => expect(true)))
 
   test("create players success"):
-    val player2 = newPlayer.copy(name = "Jane", id = 2)
+    val player2 = newPlayer1.copy(name = "Jane", id = 2)
     resource
       .use:
-        _.upsert(List(newPlayer -> newFederation.some, player2 -> newFederation.some)).map(_ => expect(true))
+        _.upsert(List(newPlayer1 -> newFederation.some, player2 -> none)).map(_ => expect(true))
 
   test("create and query player success"):
     resource.use: db =>
       for
-        _      <- db.upsert(newPlayer, newFederation.some)
+        _      <- db.upsert(newPlayers)
         result <- db.playerById(1)
         found = result.get
       yield expect(
-        found.to[NewPlayer] == newPlayer && found.federation.get.to[NewFederation] == newFederation
+        found.to[NewPlayer] == newPlayer1 && found.federation.get.to[NewFederation] == newFederation
+      )
+
+  test("create and query player without federation success"):
+    resource.use: db =>
+      for
+        _      <- db.upsert(newPlayers)
+        result <- db.playerById(2)
+        found = result.get
+      yield expect(
+        found.to[NewPlayer] == newPlayer2 && found.federation.isEmpty
       )
 
   test("overwriting player success"):
-    val player2     = newPlayer.copy(name = "Jane")
+    val player2     = newPlayer1.copy(name = "Jane")
     val federation2 = NewFederation("Lichess", "lichess")
     resource.use: db =>
       for
-        _      <- db.upsert(newPlayer, newFederation.some)
+        _      <- db.upsert(newPlayer1, newFederation.some)
         _      <- db.upsert(player2, federation2.some)
         result <- db.playerById(1)
         found = result.get
@@ -74,57 +100,57 @@ object DbSuite extends SimpleIOSuite:
 
   val defaultSorting = Sorting(SortBy.Name, Order.Asc)
   val defaultPage    = Pagination(10, 0)
+
   test("search playersByName success"):
     resource.use: db =>
       for
-        _       <- db.upsert(newPlayer, newFederation.some)
+        _       <- db.upsert(newPlayers)
         players <- db.playersByName("Jo", defaultSorting, defaultPage, PlayerFilter.default)
       yield expect(
-        players.length == 1 && players.head.to[NewPlayer] == newPlayer && players.head.federation.get
+        players.length == 1 && players.head.to[NewPlayer] == newPlayer1 && players.head.federation.get
           .to[NewFederation] == newFederation
       )
 
-  test("allPlayers sortBy name success"):
-    val player2 = newPlayer.copy(id = 2, name = "A")
+  test("allPlayers with default filter"):
+    val player2 = newPlayer1.copy(id = 2, name = "A")
     resource.use: db =>
       for
-        _       <- db.upsert(newPlayer, newFederation.some)
+        _       <- db.upsert(newPlayer1, none)
         _       <- db.upsert(player2, newFederation.some)
         players <- db.allPlayers(defaultSorting, defaultPage, PlayerFilter.default)
       yield expect(players.length == 2 && players.head.name == "A")
 
   test("search playersByFederationId success"):
     resource.use: db =>
       for
-        _       <- db.upsert(newPlayer, newFederation.some)
+        _       <- db.upsert(newPlayers)
         players <- db.playersByFederationId("fide")
       yield expect(
-        players.length == 1 && players.head.to[NewPlayer] == newPlayer && players.head.federation.get
+        players.length == 1 && players.head.to[NewPlayer] == newPlayer1 && players.head.federation.get
           .to[NewFederation] == newFederation
       )
 
-  test("search playersByIds success"):
+  test("playersByIds success"):
     resource.use: db =>
       for
-        _       <- db.upsert(newPlayer, newFederation.some)
-        players <- db.playersByIds(Set(1, 2))
+        _       <- db.upsert(newPlayers)
+        players <- db.playersByIds(Set(2, 3))
       yield expect(
-        players.length == 1 && players.head.to[NewPlayer] == newPlayer && players.head.federation.get
-          .to[NewFederation] == newFederation
+        players.length == 1 && players.head.to[NewPlayer] == newPlayer2 && players.head.federation.isEmpty
       )
 
   test("query federation summary success"):
     resourceP.use: (db, kv) =>
       for
-        _      <- db.upsert(newPlayer, newFederation.some)
+        _      <- db.upsert(newPlayer1, newFederation.some)
         _      <- kv.put("fide_last_update_key", "2021-01-01")
         result <- db.allFederationsSummary(defaultPage)
       yield expect(result.size == 1)
 
   test("query federation summary byId success"):
     resourceP.use: (db, kv) =>
       for
-        _      <- db.upsert(newPlayer, newFederation.some)
+        _      <- db.upsert(newPlayer1, newFederation.some)
         _      <- kv.put("fide_last_update_key", "2021-01-01")
         result <- db.federationSummaryById("fide")
       yield expect(result.isDefined)