Add team images for challenges - #1272
Merged
Merged
Conversation
Closed
CollinBeczak
force-pushed
the
collin/team-images
branch
from
September 8, 2026 17:34
7e95f32 to
0d9c162
Compare
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.
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.
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.
CollinBeczak
force-pushed
the
collin/team-images
branch
from
September 8, 2026 17:59
0d9c162 to
dc4e190
Compare
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, and dense regions came out looking like graph paper. The pyramid becomes the *input* to clustering rather than the output. A tile reads micro-aggregates from a level below its display zoom, runs k-means over them in Web Mercator, then merges any centroids closer together than 25 screen pixels. Markers follow the data instead of the grid, and cluster extent adapts to local density. The merge is what makes zooming out behave: k-means returns exactly k clusters however tightly packed its input is, so k is only a ceiling and consolidation comes from the separation pass. Read depth is DETAIL_BITS = 2 rather than coarsening the grid. Both would bound the k-means input at 4096 cells per tile, but the pyramid stops at MAX_CELL_ZOOM, so near the top the display zoom eats into the depth available and a tile falls back on CELL_BITS alone. Leaving CELL_BITS at 4 keeps z=11 at 256 cells of 16px, where k-means still has more input than MAX_CLUSTERS to partition and the cells are finer than the merge distance. Coarsening to CELL_BITS = 3 would have left 64 cells of 32px there: k equal to the input size makes k-means an identity, and cells wider than the merge distance make the separation pass a no-op, putting the top cluster zoom back to one marker per grid cell. Tuning the read depth instead needs no migration and no pyramid rebuild -- level z+2 already exists. 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 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.
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.
exploreChallenges filtered on challenges.bounding, the envelope of every task in a challenge. A challenge with tasks on two continents overlaps nearly any box, so searching Wichita returned USA-wide challenges. - The envelope is now only an index prefilter. A challenge matches when it has a task inside the requested area, checked with an EXISTS over tasks. - The POST form of the route takes a GeoJSON Polygon/MultiPolygon as `polygon` in the body and ANDs it with `bounds` on the same task, so a match has to be both in view and inside the place rather than in either. The geometry travels in the body because a city boundary from Nominatim runs to tens of kilobytes, past any practical URL length; the route parses with its own 2MB limit and rejects a non-polygon geometry with a 400. - `global` now defaults to false, matching taskTilesMvt and the UI toggle, which rendered "off" while the list still included global challenges. - sortBy accepts featured, tag_fix and cooperative. These group a kind of challenge to the front rather than ordering by the column, so each falls back to name to keep the ordering inside a group stable across pages.
Challenges had no card image of their own. Teams can now supply one, kept under review so an image on a public challenge card has been looked at. An image is owned by a team and moderated: any active member uploads one as a request (evolution 124 stores the bytes), a superuser approves or rejects it, and from then on any member of that team can attach it to their challenges through the new `teamImageId` on a challenge. - Attaching is validated on create and update. Image ids are plain numbers on the wire, so without this anyone could borrow another team's image, or one still awaiting review, by guessing an id. - An update that omits `teamImageId` leaves the current image alone and an explicit null detaches it, so a save that never touched the image picker cannot silently clear it. - Challenge json carries `teamImageId` plus a derived `avatarUrl`, keeping url construction in one place rather than in each client. - An unapproved image is served to the people with a reason to see it -- a superuser working the review queue, and members of the owning team -- as private/no-cache. To everyone else it stays a 404. Approved images are served anonymously with an ETag, since the url feeds plain img tags.
CollinBeczak
force-pushed
the
collin/team-images
branch
from
September 8, 2026 18:54
dc4e190 to
d37a1ea
Compare
CollinBeczak
marked this pull request as ready for review
September 14, 2026 20:49
CollinBeczak
marked this pull request as draft
September 14, 2026 20:55
Teams gain a fourth role above admin, so the four are Owner, Admin, Manager and Member. They are the generic grant roles under team-facing names, which keeps the ordering the permission system relies on - lowest number, most privilege - and lets every existing `role <= ROLE_ADMIN` check pass an owner through unchanged. Managers run the content of a team, admins also run its membership, and only an owner can delete the team or make someone else an owner. A team is never left without one: its last owner can be neither demoted nor removed. Evolution 124 promotes the oldest admin grant on each existing team, which is its creator wherever they are still around. A challenge can now be owned by a team. That hands the owners, admins and managers of that team the run of it wherever their grants on the parent project would not have reached; everyone else still gets in through the project exactly as before. Owning the challenge is also what puts an image on its card, so the stored image reference on challenges is gone. The card url is addressed by team rather than by image, which means a challenge always shows whatever its team currently has approved and no card can be left pointing at an image that has been replaced. A team with no approved image answers 404 there, which is how a client tells there is simply no picture to show.
A challenge has carried an owner_team_id since teams were given ownership, but nothing ever queried it, and the team-to-project grants could only be read in the inverse direction -- GET /teams/projectManagers/:projectId answers "who manages this project", with no way to ask what a given team has. So there was no way to show a team what it holds. Adds two endpoints: GET /team/:id/projects the projects the team has a role on GET /team/:id/challenges the challenges given to the team Both filter to what the requesting user may see. Projects follow the rule their own listings use: a disabled project is shown only to someone granted a role on it. Challenges follow ChallengeService's challengeVisibilityFilter -- visible when the challenge and its parent project are both enabled, or when the user holds a role on that parent -- applied in Scala rather than as a filter, since that filter joins the projects table and ChallengeRepository.query does not. The challenges response embeds each parent project through ParentMixin, as the challenge listing endpoints do, so a client can name the project without a second round trip.
Challenge access came entirely from the parent project: to let someone at a single challenge you had to hand them the project it lives in, and everything else in it with it. The only narrower option was giving the challenge to a team, which is a different thing -- it re-credits the challenge and moves its image. A challenge can now be a grant target in its own right, so a user can be granted Admin, Write or Read on one challenge and nothing else: GET /challenge/:id/managers POST /challenge/:id/user/:userId/:role DELETE /challenge/:id/user/:userId Permission treats such a grant the way it already treats an owning team: another way in, checked before falling back to the parent project. Roles are ordered with the lowest number the most powerful, so a stronger role satisfies a weaker requirement, and a Write grant does not stand in for the Admin that handing out roles itself takes. A user holds one role per challenge -- granting replaces rather than accumulates, so revoking is one delete and not a search for every role they might have picked up. Grants ride on the cached copy of a user, so both granting and revoking clear that cache. Without it a role is written to the database and the user keeps their old access until the entry ages out; the service spec covers this, and caught it. Also unquotes a colon out of a swagger description: it parses as YAML during `dist`, so `sbt compile` passes and the docker build does not.
|
CollinBeczak
marked this pull request as ready for review
September 15, 2026 15:39
CollinBeczak
added a commit
that referenced
this pull request
Sep 15, 2026
* Host team avatars instead of only linking them Replays the team avatar work onto main, which now carries the team images feature from #1272. That landed as a squash, so the avatar branch's own copy of team images conflicted with the shipped version rather than merging with it; only the avatar half is unique and is what is replayed here. The avatar migration moves from 124 to 125, since main's 124 is the team owner role and challenge ownership. It only references groups and users, so it transplants unchanged. * Reuse the team image plumbing for avatars The avatar endpoints were written before the team images feature landed and ended up restating pieces of it. Where the two now sit side by side, the avatar side defers to what is already there. The repository drops its hand-written row parser for the macro parser the query's aliases already suit, and its inline copy of TeamImageRepository's dataParser for the shared one. TeamAvatar's aliases for the format and size rules are gone; callers read them off TeamImage, which is where the single rule lives. The multipart parser bound moves to TeamImage.MAX_UPLOAD_BYTES. The image endpoint had reasoned its way to MAX_SIZE_BYTES * 2 and written down why; the avatar endpoint had no bound at all and fell back to the 15MB disk buffer, spooling an oversized upload in full before rejecting it for exceeding 2MB. Serving an avatar now builds the ETag from the metadata row and only reads the bytes on a cache miss, so revalidating no longer pulls up to 2MB out of Postgres to answer 304. That query existed already and was simply never wired up. Deleting an avatar commits the delete and the url clear together, as uploading already did, and the serving path is built once rather than spelled out in both urlFor and isStoredAvatarUrl. * Give team avatars a service, and share the image mechanics Four things that were wrong with the avatar endpoints, all of them visible only once the team images feature landed beside them. updateTeam announced itself over the websocket even when running inside a caller's transaction, so a rollback broadcast an update that never committed. Old callers always committed immediately, but both avatar paths now wrap it, which makes the bug reachable. The broadcast is now the caller's to make once its transaction has committed. Reading an upload and writing image bytes back out were spelled out twice, once per controller, down to three identical error strings and the ETag and cache headers. They move to a mixin both controllers use. Duplicating the two security headers is the part worth avoiding: a change made in one place and missed in the other fails silently. The avatar rules - check ordering, the transaction, which url is ours - lived in Action bodies where nothing else could reach them and no test could reach them either. They move to TeamAvatarService, matching the route the image endpoints already take through TeamImageService. Removing an avatar decided whether the stored url was ours by matching a route prefix, which the frontend then had to reimplement. It now reconstructs the url from the stored row and compares exactly, so a team that has since typed their own url keeps it. Serving an avatar is also cacheable indefinitely rather than for a day, since the url carries the upload timestamp and the bytes behind one can never change.
CollinBeczak
added a commit
that referenced
this pull request
Sep 15, 2026
Main squash-merged the team images (#1272) and team avatars (#1280) work this branch was sitting on top of, so its own copies of both collided with the shipped versions rather than merging with them. Every one of those conflicts resolves to main's side: the branch carried the pre-review originals, and main has the evolved versions, including the avatar service and shared image mixin that landed with #1280. The challenge reports evolution moves from 125 to 126, since main's 125 is now the team_avatars table. The only genuine merges were the three test registries where both sides had added an entry - the master suite, the helper's tags and TestSpec's service mocks - which take both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split from #1268
dependent on: #1273