Moderated team images for challenge cards - #1268
Open
CollinBeczak wants to merge 7 commits into
Open
Conversation
Teams can now upload an image to use on their challenge cards. An upload starts as a request (status 0), a superuser approves or rejects it, and once approved any active member of that team can attach it to their challenges via challenges.team_image_id. Deleting an image detaches it from every challenge using it, so revoking an image takes effect on the cards that showed it. Challenge json gains teamImageId plus a derived avatarUrl so clients don't have to build the file url themselves. Create and update bodies are validated: an id must exist, be approved, and belong to a team the caller is an active member of - otherwise anyone could borrow another team's image by guessing an id. Also included: - Review claims now take a single bundle lock on the primary task instead of one lock row per task, matching how edit locks work. Unclaiming resolves the bundle and clears the claim on every member, and stale review-claim lock rows are dropped when a new claim is made instead of accumulating. - lockBundle folds any of the caller's own rows that overlap the bundle into the primary row, so two rows can never cover the same task and break the singleOpt lookups in resolveLockHolder/resolveLockBundle. - Locking a task in a paused challenge is rejected - there is no work to hold a lock for. - Lock conflict responses include parentId alongside parentName so clients can link straight to the blocking task's challenge.
Team images: - POST/DELETE /team/:teamId/avatar store and clear an avatar we host, pointing the team's avatar_url at bytes of our own so the rest of the app keeps treating an avatar as a plain url (evolution 122). Unlike team images these aren't moderated: an admin could already link any image on the internet, so reviewing only uploads would gate the safer of the two paths. GET /team/:teamId/avatar/file serves them anonymously with an ETag, since the url feeds plain img tags. - An unapproved team image is now 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. Explore location filtering: - exploreChallenges matched on challenges.bounding, the envelope of every task in a challenge, so one with tasks on two continents overlapped nearly any box and 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. - The POST form of the route takes a GeoJSON boundary 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. The geometry travels in the body because a city boundary from Nominatim runs to tens of kilobytes; the route parses with its own 2MB limit and rejects a non-polygon with a 400. - The global default is false, matching taskTilesMvt and the UI toggle, which rendered "off" while the list still included global challenges. Work that can't be worked no longer reads as available: - Paused challenges drop out of the live tile queries, the cached tile pyramid (evolution 124 redefines both rebuild functions and widens the dirty-marking trigger), the task cluster queries and the challenge list. All four tile paths stay in sync. - Evolution 123 reconciles challenges left at READY while showing 100% complete, which Explore would otherwise keep listing as work.
The explore/extendedFind challenge listing only supported ordering by a single column (name, created, modified, popularity, difficulty). Add three taxonomy sorts that surface a kind of challenge first rather than ordering by the column itself: - featured: featured challenges first - tag_fix: cooperative tag-fix challenges first - cooperative: any cooperative challenge first, ordered by cooperative type Each falls back to c.name ASC so ordering within a group stays stable across pages. Update the Swagger enums in challenge.api and the scaladoc on both the controller and DAL to list the new values.
Reports are complaints about a challenge's design -- "this challenge is poorly designed and is causing incorrect edits" -- as opposed to bugs or feature requests. They were filed as issues in a public GitHub repo, which meant shipping a write-scoped GitHub token to the browser and publishing the reporter's identity alongside the complaint. Evolution 125 stores them instead, so triage happens inside MapRoulette and the reporter's contact details stay private to superusers. - POST /challenge/:challengeId/report files a report; GET /challenge/:challengeId/report/mine returns the caller's own. - GET /challenge/reports and /challenge/report/:id back the superuser triage queue, and PUT /challenge/report/:id/status resolves a report as actioned (1) or dismissed (2). A report is never deleted, only resolved. - reporter_id is ON DELETE SET NULL and reporter_email is nullable: the row has to outlive its reporter, since an admin may still need to act on a report after the account is gone. - An email is optional, trimmed and pattern-checked, and is a contact the reporter volunteers for this filing rather than a copy of their account address -- it is never exposed on the challenge itself. - One open report per reporter per challenge, so a single mapper can't flood the queue while an earlier report is still awaiting review. challengereport.api is ordered ahead of challenge.api in build.sbt because the literal /challenge/reports and /challenge/report/:id paths would otherwise be swallowed by that file's GET /challenge/:id.
The foreign key added in evolution 121 had no index behind it, so Postgres scans challenges to enforce it on every team_images delete, and the lookups that find and detach the challenges using an image -- run whenever an image is rejected or removed -- scan as well. These are rare admin operations against a table of tens of thousands of rows, so this is not a hot path today; but the same evolution already indexes its other foreign key columns, and it will not be cheaper to add later. Evolution 121 is unreleased, so the index goes there rather than into a new evolution -- anyone who has applied it locally will re-run it.
uploadAvatar stored the image and then pointed the team's avatar_url at it in two separate transactions. A failure in between left the bytes stranded with the team still on its previous avatar, and because the url carries a ?v=<modified> stamp, the stale value would sit in browser caches until somebody uploaded again. TeamAvatarRepository.upsert took no connection parameter, so a caller's transaction could never reach it -- withMRTransaction resolves its implicit at the method body, not the call site, and quietly opened its own. Thread an optional connection through upsert, GroupService.updateGroup and TeamService.updateTeam, the pattern GroupRepository.update already follows, so the controller can run both writes under one db.withTransaction. Every added parameter defaults to None, so no existing call site changes. The teamUpdate websocket message still fires inside the transaction, so a rollback would leave a notification already sent. That predates this change and holds for every updateTeam caller; it only prompts a refetch.
|
Contributor
|
Let's discuss this at our next internal sync meeting. I'm not sure we want to continue expanding the "teams" feature in MapRoulette; I view this as a legacy feature that should eventually be replaced by integration with e.g. https://mapping.team/ However, I do definitely think we should have better tools for indicating which organization, company, or group is the owner of a challenge (better accountability for organized editing guidelines, etc) and it sounds like this would be a step in that direction, so maybe it's worth pursuing. Just want to understand the tradeoffs better before we commit to it. |
Contributor
Author
|
Sounds good, i think that that is a good direction at first thought. Will still probably need most of the teams related changes, but yeah, might just swap out where the teams themselves actually live to there. |
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
2 participants
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.
What
Adds team-owned images that can be displayed on challenge cards, with a
request/approve moderation flow.
A team member uploads an image, which lands as a pending request. A superuser
approves or rejects it. Once approved, any active member of that team can
attach it to their challenges. Deleting an image detaches it from every
challenge that used it, so revoking an image actually clears the cards showing
it.
New endpoints (
conf/v2_route/teamimage.api):/team/:teamId/image/team/:teamId/images/teamImages/available/teamImages/pending/teamImage/:id/approve/teamImage/:id/reject/teamImage/:id/teamImage/:id/fileChallenge json now carries
teamImageIdand a derivedavatarUrl.Also in this PR
Lock fixes that this work depends on:
wrote one
lockedrow per task; it now writes a single row on the primarytask with the members in
bundled_tasks, the same shape edit locks use.Unclaiming resolves the bundle and clears the claim on every member, and
leftover review-claim rows from a previous claim are dropped rather than
left holding tasks locked.
lockBundleno longer allows overlapping rows. It folds any of thecaller's own rows covering a bundle member into the primary row; two rows
covering the same task would break the
singleOptlookups inresolveLockHolder/resolveLockBundle.startOnTaskand the bundle variantreturn an error when the parent challenge is paused.
parentIdnext toparentName, so theclient can link to the blocking task's challenge instead of only naming it.
Migration
conf/evolutions/default/121.sql— createsteam_imagesand addschallenges.team_image_id(FK,ON DELETE SET NULL). Downs drop both.Testing
TeamImageSpec— model/status behaviorTeamImageRepositorySpec— repository CRUD and status transitions