hide unavailable work from explore challenges results (#1274)

* Match explore keywords with a semi-join, not a tag join

A keyword names a tag on the *challenge*, but the queries matched it by
joining `tags_on_challenges` into a per-task query. That returns every task
of a challenge once per requested tag the challenge carries, so anything
downstream that counts or clusters rows sees the same task several times.

Demonstrated on a 166k-task database by giving an eligible challenge a
second real keyword and asking for both: the join returns 22,430 rows for
11,215 tasks, the semi-join 11,215. Any challenge tagged with two of the
selected categories is enough, which is ordinary -- challenges routinely
carry several keywords and the explore UI lets more than one be selected.

What the duplication reached:
- Cluster counts on the keyword-filtered explore map were inflated, and
  cluster centroids were pulled toward multi-tagged challenges, since the
  duplicate rows inflate the centroid weight as well as the count.
- At zoom 12, where features are grouped by location, a task counted twice
  reported 2 and rendered as an overlap stack rather than a single
  clickable task, with its id repeated in `task_ids_str`.
- queryTaskMarkersWithOverlaps returned each task once per matching tag, and
  since it groups by location with DBSCAN, the duplicates of one task became
  a phantom overlap group.

exploreChallenges had the same join, hidden behind a SELECT DISTINCT. It
moves to the semi-join and drops the DISTINCT, which nothing else needs: the
only other join there is many-to-one on the parent project. That also lifts
a restriction the DISTINCT imposed -- Postgres requires every ORDER BY
expression to appear in the select list under SELECT DISTINCT, so a sort
could not order by an expression.

The other three keyword call sites in TaskClusterRepository were already
protected by SELECT DISTINCT / COUNT(DISTINCT), so they were correct; they
move to the semi-join too, to leave one spelling of the question in the
file.

This is a correctness fix, not a performance one -- the two forms measure
the same. Best of 3 at zoom 0 on that database: 9.19ms vs 9.17ms for a
keyword matching one challenge of 31, and 38.0ms vs 36.0ms where the join
was duplicating every row. Cost there is dominated by the scan over tasks,
which neither form avoids at low zoom.

Not covered: ProjectRepository.getSearchedClusteredPoints joins the tag
tables the same way with no DISTINCT, so it can still return a challenge's
clustered point once per matching tag.

* Claim a review bundle with a single lock row

A reviewer claiming a bundle locked each member task individually, which
left one `locked` row per task. Locking.lockBundle already models a bundle
as one row on the primary task with the members in `bundled_tasks`, so the
two representations disagreed and the singleOpt lookups in
resolveLockHolder/resolveLockBundle could see two rows covering the same
task.

- claimTaskReview now takes the whole list through lockBundle with
  reviewClaim = true, so one row covers the bundle. Review claims stay
  exempt from the one-lock-per-user invariant, which is what the new
  reviewClaim parameter on lockBundle carries through.
- lockBundle checks every task the row will cover, not just the primary,
  and folds any of our own overlapping rows into that one row. Without this
  a claim over tasks already covered by another of our rows would leave two
  rows covering the same task.
- claimTaskReview drops leftover rows from a previous claim. The unclaim it
  runs first only clears task_review, so those rows accumulated and kept
  their tasks locked.
- unclaimTaskReview clears the claim on every member of the bundle, since
  releasing the single row releases them all, and runs its unlock inside the
  surrounding transaction.
- A lock conflict response now carries parentId alongside parentName, so a
  client can link to the challenge holding the conflicting lock.

* Cluster explore tiles with k-means and coarsen the grid

Tile clusters were the grid bins themselves, so a cluster's position was
its cell centroid and its size was fixed by the grid. Neighbouring cells
holding a handful of tasks each stayed separate markers no matter how far
apart their tasks actually were.

The repository now runs k-means over the cells feeding a tile and returns
the resulting clusters, so markers land on where the tasks are and nearby
cells merge into one cluster instead of sitting side by side. A separation
pass then collapses centroids closer together than 25 screen pixels, which
is what makes zooming out consolidate clusters: k-means returns exactly k
clusters however tightly packed its input is, so k is only a ceiling.

Evolution 121 coarsens the grid to match: CELL_BITS 4 -> 3, moving the leaf
level from slippy zoom 15 to 14 so a display tile holds 8x8 = 64 cells
instead of 16x16 = 256, each cell twice as wide. Only the cell<->slippy-zoom
mapping changes -- the roll-up is untouched -- so the leaf functions from
evolution 107 are redefined at zoom 14 and the pyramid is rebuilt. The Downs
restores the zoom 15 leaf and rebuilds again.

Cluster weight is the filtered count, not the cell total. A cell's stored
sums cover every task in it, so its centroid is the right position either
way, but carrying the total into the merge let a cell drag a cluster in
proportion to the tasks the filter had just excluded -- a marker reporting a
handful of expert tasks could sit on top of a thousand easy ones. This did
not arise before, when every cell was its own marker and a mis-weighting
could not cross cell boundaries. Measured against the base tables on a
166k-task database under a difficulty filter, mean cluster displacement
drops from 563m to 2m at z=8 (worst 2345m -> 4m) and from 68m to 3m at z=11
(worst 298m -> 12m).

A new `clusterMarkers` shares the whole src -> k-means -> separation chain
with the MVT paths and returns the markers as plain numbers, so a test can
assert where one landed without decoding protobuf. The repeated-request test
now seeds more cells than MAX_CLUSTERS, since at or below the ceiling
k-means returns one cluster per input and the clustering step is an identity.

* Stop surfacing work that cannot be worked

A paused challenge's tasks cannot be locked, completed or reviewed until it
is resumed, and a finished challenge has no tasks left at all. Both still
showed up as available work.

- Paused challenges drop out of all four tile paths: this repository's live
  MVT queries, and the cached pyramid's rebuild_leaf_cell and
  rebuild_all_tile_cells (evolution 122). That evolution also widens the
  challenge dirty-marking trigger from evolution 107 to fire on `paused`,
  without which a pause never reaches the pyramid, and marks the cells of
  already-paused challenges stale so the scheduled drain recomputes them
  rather than rebuilding everything.
- Paused challenges also drop out of the task cluster queries and out of
  exploreChallenges, which additionally omits STATUS_FINISHED challenges.
  NULL status predates the column and counts as unfinished.
- Locking a task or a bundle in a paused challenge is rejected: there is no
  work to hold a lock for.
- Evolution 121 reconciles challenges left at READY while showing 100%
  complete. updateFinishedStatus keeps status in sync as tasks are worked,
  but paths that never run it (bulk status changes, deletions, restores)
  could leave a done challenge listed as work.

Author: CollinBeczak

app/org/maproulette/controllers/api/TaskController.scala

@@ -213,10 +213,11 @@ class TaskController @Inject() (
   }
 
   /**
-    * Start on task (lock it). An error will be returned if someone else has the lock.
-    * If the calling user already holds a lock on a different task, a 409 Conflict is
-    * returned describing that lock - the client should call the release endpoint on
-    * that task before retrying to lock this one.
+    * Start on task (lock it). An error will be returned if someone else has the lock,
+    * or if the parent challenge is paused - no work can happen on a paused challenge, so
+    * there is nothing to hold a lock for. If the calling user already holds a lock on a
+    * different task, a 409 Conflict is returned describing that lock - the client should
+    * call the release endpoint on that task before retrying to lock this one.
     *
     * @param taskId     Id of task that you wish to start
     * @return
@@ -243,6 +244,12 @@ class TaskController @Inject() (
           )
       }
 
+      if (challenge.extra.paused) {
+        throw new InvalidException(
+          "This challenge is currently paused. Tasks cannot be locked until it is resumed."
+        )
+      }
+
       try {
         val lockerId = this.dal.lockItem(user, task)
         if (lockerId != user.id) {
@@ -343,6 +350,12 @@ class TaskController @Inject() (
             )
         }
 
+        if (challenge.extra.paused) {
+          throw new InvalidException(
+            "This challenge is currently paused. Tasks cannot be locked until it is resumed."
+          )
+        }
+
         try {
           val lockerId = this.dal.lockBundle(user, task, taskIds)
           if (lockerId != user.id) {

app/org/maproulette/framework/repository/TaskClusterRepository.scala

@@ -385,6 +385,7 @@ WITH eligible_challenges AS MATERIALIZED (
   WHERE c.deleted = false
     AND c.enabled = true
     AND c.is_archived = false
+    AND c.paused = false
     AND p.deleted = false
     AND p.enabled = true
     ${if (!global) "AND c.is_global = false" else ""}
@@ -466,6 +467,7 @@ ORDER BY kmeans;
         WHERE c.deleted = false
         AND c.enabled = true
         AND c.is_archived = false
+        AND c.paused = false
         AND p.deleted = false
         AND p.enabled = true
         AND tasks.location IS NOT NULL
@@ -572,6 +574,7 @@ ORDER BY kmeans;
         WHERE c.deleted = false
           AND c.enabled = true
           AND c.is_archived = false
+          AND c.paused = false
           AND p.deleted = false
           AND p.enabled = true
           AND tasks.location IS NOT NULL
@@ -660,6 +663,7 @@ ORDER BY kmeans;
         WHERE c.deleted = false
         AND c.enabled = true
         AND c.is_archived = false
+        AND c.paused = false
         AND p.deleted = false
         AND p.enabled = true
         AND tasks.location IS NOT NULL

app/org/maproulette/framework/repository/TileAggregateRepository.scala

@@ -56,12 +56,14 @@ import play.api.db.Database
   *     (z, x, y, filters), which is what tile caching requires.
   *
   * All four code paths (this repository's live queries, plus `rebuild_leaf_cell`
-  * and `rebuild_all_tile_cells` in evolution 107) share one eligibility filter:
-  * a task is available work when it has a valid location, `status IN (0,3,6)`,
-  * is not archived, and its challenge/project are enabled and not deleted or
-  * archived. `enabled` is MapRoulette's "discoverable" flag, so requiring it on
-  * both challenge and project keeps hidden work off the explore map. Keep all
-  * four paths in sync.
+  * and `rebuild_all_tile_cells`, last redefined in evolution 122) share one
+  * eligibility filter: a task is available work when it has a valid location,
+  * `status IN (0,3,6)`, is not archived, and its challenge/project are enabled
+  * and not deleted or archived, with the challenge not paused. `enabled` is
+  * MapRoulette's "discoverable" flag, so requiring it on both challenge and
+  * project keeps hidden work off the explore map; a paused challenge has work
+  * that cannot be locked or completed, so it is off the map too. Keep all four
+  * paths in sync.
   */
 @Singleton
 class TileAggregateRepository @Inject() (override val db: Database) extends RepositoryMixin {
@@ -447,6 +449,7 @@ class TileAggregateRepository @Inject() (override val db: Database) extends Repo
       s"""AND t.status IN (0, 3, 6)
           AND t.archived = false
           AND c.deleted = false AND c.enabled = true AND c.is_archived = false
+          AND c.paused = false
           AND p.deleted = false AND p.enabled = true
           $globalClause
           $difficultyClause

app/org/maproulette/models/dal/ChallengeDAL.scala

@@ -2642,6 +2642,11 @@ class ChallengeDAL @Inject() (
     * Location filtering is bounding-box based: the client resolves any named place
     * (e.g. via Nominatim on the frontend) to a bbox and passes it as `boundingBox`.
     *
+    * Challenges marked STATUS_FINISHED are omitted: there is no work left in
+    * them, so they are not something to discover. Paused challenges are
+    * omitted for the same reason -- their tasks cannot be locked, completed or
+    * reviewed until the challenge is resumed.
+    *
     * @param includeGlobal Whether to include challenges marked as global
     * @param boundingBox Optional bounding box to filter by challenge location (left, bottom, right, top)
     * @param sortBy Column to sort by (name, created, modified, popularity, difficulty)
@@ -2680,7 +2685,12 @@ class ChallengeDAL @Inject() (
       }
 
       query += " WHERE c.deleted = false AND c.enabled = true AND c.is_archived = false"
+      query += " AND c.paused = false"
       query += " AND p.deleted = false AND p.enabled = true"
+      // A finished challenge has no tasks left to work on, so it is not
+      // something to discover here. NULL status predates the column and is
+      // treated as unfinished.
+      query += s" AND (c.status IS NULL OR c.status <> ${Challenge.STATUS_FINISHED})"
 
       if (!includeGlobal) {
         query += " AND c.is_global = false"

conf/evolutions/default/121.sql

@@ -0,0 +1,30 @@
+# --- MapRoulette Scheme
+
+# --- !Ups
+-- Reconcile challenge status with the work actually left in each challenge.
+--
+-- A challenge is FINISHED (5) once it has tasks and none of them are CREATED
+-- (0) or SKIPPED (3). ChallengeDAL.updateFinishedStatus keeps this in sync as
+-- tasks are worked, but challenges whose tasks were completed through a path
+-- that never ran it (bulk status changes, task deletions, restores) can be
+-- left sitting at READY while showing 100% complete. Explore now hides
+-- finished challenges, so a stale status keeps a done challenge in the list.
+
+-- Mark as FINISHED. Restricted to statuses that mean "loaded and workable"
+-- (NA, READY, PARTIALLY_LOADED) so a challenge mid-build, failed, or deleting
+-- its tasks is not misreported as finished.
+UPDATE challenges c SET status = 5
+WHERE c.deleted = false AND
+      (c.status IS NULL OR c.status IN (0, 3, 4)) AND
+      0 < (SELECT COUNT(*) FROM tasks WHERE tasks.parent_id = c.id) AND
+      0 = (SELECT COUNT(*) FROM tasks
+           WHERE tasks.parent_id = c.id AND tasks.status IN (0, 3));;
+
+-- Back to READY for anything marked finished that still has work left, the
+-- same correction updateReadyStatus makes when a task returns to created.
+UPDATE challenges c SET status = 3
+WHERE c.status = 5 AND
+      0 < (SELECT COUNT(*) FROM tasks
+           WHERE tasks.parent_id = c.id AND tasks.status IN (0, 3));;
+
+# --- !Downs