Repository navigation
Task pool board card + pin the current task on every claim - #264
Conversation
Adds TASK_POOL, a baseline board card carrying the whole live starter-work pool ranked for the hire (same ranking and reasons as GET /me/matches), so a task can be browsed and grabbed without going through the buddy. Claiming now pins the CURRENT_TASK card inside UserGoalService.claimForMe instead of only in the buddy's claim_goal action. That card is mentor-placed, so a hire grabbing by hand via POST /me/goal would otherwise have a task and no card showing it. V20 drops the Hibernate-generated board_cards_kind_check: ddl-auto update never widens it, and a baseline kind the column rejects would fail every board read on an existing database. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BabuPlk
left a comment
There was a problem hiding this comment.
Read the diff at 6a5e545 against dev (bd64967, branched off the current tip), together with the frontend side (SprintStartProject/sprintstart-frontend#278, reviewed there). The contract lines up field by field: TaskPoolContent / BoardPoolTaskResponse match TaskPoolContent / BoardPoolTask in board/types.ts, the TaskType values match the frontend's TYPE_LABELS, and POST /me/goal?projectId=… with { taskId } is what myStarterWorkService.claim sends. CI is green.
The code itself is small and clean. One thing needs sorting out before this can merge, though, so I'm requesting changes for that.
1. Nothing runs V20, so existing databases break on the first board read
Flyway isn't in the build. It was added and then removed again (2982075c, "Delete Flyway dependency"), and nothing else in the repo applies src/main/resources/db/migration: not application.yaml, not the Dockerfile, not docker-compose.yaml, not CI. So V20__relax_board_card_kind_check.sql is a file, not a migration. Nothing will execute it on deploy.
The PR description spells out what happens then: TASK_POOL is baseline, so it's inserted on every board read, and on any database whose board_cards table predates this change the Hibernate-generated board_cards_kind_check rejects it, which takes the whole board down. That's every existing environment, including the deployed one. CI can't catch it because tests start from a fresh schema, where Hibernate writes the constraint with TASK_POOL already in it.
It also comes back for the next kind: every database created after this gets a fresh CHECK listing today's kinds, and the next new baseline kind breaks it again.
What I'd want before merge (any of these works):
- Drop the constraint from the application itself, idempotently, on startup (e.g. an
ApplicationRunnerexecuting exactly theALTER TABLE IF EXISTS … DROP CONSTRAINT IF EXISTSfrom V20). That fixes existing databases and keeps working if it's re-created. - Or stop Hibernate from generating the enum check for this column in the first place, if that's possible with our setup (worth checking with an explicit
columnDefinitiononBoardCard.kind), plus the one-off drop above for databases that already have it. - Or, at minimum, a team decision that someone applies V20 by hand on every environment before this deploys, written down in the PR. As far as I can see that's the only way the SQL files get applied today, which is fragile for a change whose failure mode is "no board for anyone".
Side note: V20 is also taken on three other branches (fix/team-mode-areas-across-turns, feature/305-bitbucket-repository-connector-v0, KB-updates-final), and dev already has two V18s. Without Flyway nothing enforces the numbering, which is one more reason not to rely on the file alone.
2. Claiming doesn't re-pin a dismissed current-task card
claimForMe now calls boardService.place(userId, projectId, CURRENT_TASK) and ignores the result. place returns DISMISSED_BY_HIRE without doing anything if the hire ever dismissed that card. So after a hand grab the hire has a task and the board doesn't show it, which is exactly the case this change is meant to fix. The buddy's confirm message also still promises "It's on your board too".
On the frontend this shows up as a small inconsistency: the pool marks "You're on this one" (from currentTaskId), but the grab confirm asks "Make this your task?" because the CURRENT_TASK card isn't there.
I think grabbing is a strong enough signal to bring the card back (the hire just said "this is what I'm working on"). If "a dismissal is final" should win here too, then the buddy message shouldn't promise the board, and the frontend shouldn't rely on the card. Either way it's worth a test: claim with a dismissed CURRENT_TASK row.
3. The ranking now runs twice on every board read, for every hire
suggestedTasksContent and taskPoolContent each call matchForUserId. That's findAllByStatus(LIVE) over the whole pool, buildProfile and artifactIngestionApi.getRepositoryResponsiveness(projectId), and it happens twice per read. Because TASK_POOL is baseline, every board pays it, not only boards the mentor put suggestions on. Computing the ranked list once per hydrate pass and handing it to both cards would halve that and also guarantees they agree.
4. Small things
bestFitvs. "Good next tasks".bestFitisindex < 3 && score > 0, whilesuggestedTasksContenttakes the top three regardless of score. If fewer than three tasks score above zero, a task can be in "Good next tasks" without being "Best fit" in the pool. It's probably intended (a zero score really isn't a fit), but then "Good next tasks" is the one that's off, and it's worth saying so in a comment.- The 409 message. The frontend shows "That task just closed where it lives" for any 409. Here a 409 means "not
LIVE", which also covers retired or rejected tasks. Not a backend change, just flagging it since the two PRs go together. - Removing
BoardServicefromBuddyActionServiceand moving the pinning intoUserGoalServiceis the right place for it. A test that a rejected claim (409) does not place the card would pin that the order insideclaimForMestays correct.
What's good here
- The pinning moved to where the claim happens, so the buddy route and the hand route can't drift apart. The comment says why.
- The pool reuses
matchForUserId, so the card,GET /me/matchesandget_suggested_taskscan't disagree about ranking or reasons, and the score is kept off the wire. currentTaskIdcomes fromCurrentTaskReader, so a task-zero assignment counts as "current" as well, not only a claimed goal.- The PR description explains the constraint problem better than most commit logs would. Point 1 is really only about making sure the fix actually runs.
Requesting changes for 1. 2 and 3 are quick, and the rest can be follow-ups.
…ve, one ranking - Replace V20 with BoardCardKindConstraintRelaxer, an ApplicationRunner that drops board_cards_kind_check idempotently on every startup. Flyway was removed (2982075), so nothing ever executed the SQL file: every existing database would have rejected TASK_POOL on the first board read. Running on every start also covers databases created later, whose fresh constraint would break on the next new kind. Failures are logged, not thrown. Verified on Postgres in a rolled-back transaction. - Claiming now revives a dismissed CURRENT_TASK card (BoardService.placeOrRevive) instead of silently leaving it gone. The mentor's place() still respects the dismissal. - Rank the pool once per board read and share it between the suggestions and the pool card (lazy, so boards with neither pay nothing). The two card builders move to BoardTaskCards, which also keeps BoardService under detekt's LargeClass limit and documents why "Good next tasks" takes the top three regardless of score. - Tests: relaxer (SQL, failure tolerated), revive vs. place, a 409 claim pins nothing, one ranking per board read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, point 1 was a real miss on my side: I didn't check whether anything still runs the migration files. All of it is addressed in 1. Nothing runs
2. Dismissed current-task card. 3. Ranking twice. 4. The comment on "Good next tasks" now says why it takes the top three regardless of score, while
🤖 Generated with Claude Code |
BabuPlk
left a comment
There was a problem hiding this comment.
Checked d2532dfb. All of it is addressed:
- 1. Constraint drop that actually runs:
BoardCardKindConstraintRelaxeris the right shape. It's idempotent, runs on every start so it also covers databases created later, and it only logs if the database refuses instead of failing. RemovingV20also resolves the numbering clash. One detail, just so it's said: anApplicationRunnerruns after the web server has started, so on the very first deploy there is a moment where a board read could still hit the old constraint. In practice that's negligible, and I wouldn't change it for that. - 2. Dismissed current-task card:
placeOrRevivelimited to the hire's own grab, with the mentor'splace()left alone, is a clean way to draw that line. The tests cover both sides and the 409 case (verify(exactly = 0)). - 3. One ranking per read: the
lazyingetBoard, shared by both cards, plusverify(exactly = 1). Moving the builders intoBoardTaskCardsalso reads better. - 4. The comment on "Good next tasks" vs.
bestFitsays exactly what was unclear.
Contract with frontend#278 at bcbd3182 still matches field for field; that side is green on tsc, lint, build and the full unit suite. I couldn't run ./gradlew check myself (Gradle and Maven Central are blocked where I work), so for the backend I'm relying on CI, which is green, and your local run.
Approving, and taking back the change request.
Summary
Backend half of letting a hire browse the starter-work pool and grab a task by hand, instead of only through the buddy. Frontend counterpart: SprintStartProject/sprintstart-frontend#278.
TASK_POOL: carries the whole live pool ranked for the hire (the same ranking and reasons asGET /me/matchesand the buddy'sget_suggested_tasks), uncapped, pluscurrentTaskIdso the client can mark the task the hire is already on. Each task carriesbestFit(top 3 with a positive score),taskType,reasons,summary,rationale,sourceHasAssignee, and never the score.BoardTaskCards), so the two can't disagree and a board with neither card pays nothing.CURRENT_TASKinUserGoalService.claimForMe, not only in the buddy'sclaim_goalaction, viaBoardService.placeOrRevive. That also brings back a current-task card the hire dismissed earlier: grabbing a task is them saying "this is what I'm working on". The mentor'splace()still respects dismissals.BuddyActionServiceno longer places the card itself and drops its now-unusedBoardServicedependency.BoardCardKindConstraintRelaxer(ApplicationRunner) drops the Hibernate-generatedboard_cards_kind_checkon every startup.Why the constraint drop, and why at startup
board_cardsis created byddl-auto: update, which writes aCHECKlisting everyBoardCardKindat table creation and never widens it.TASK_POOLis baseline, so it's inserted on every board read. On any database created before this change the insert would fail and take the whole board down.This was first a
V20migration file, but Flyway was removed (2982075c) and nothing appliesdb/migration, so it would never have run. The runner executesALTER TABLE IF EXISTS board_cards DROP CONSTRAINT IF EXISTS board_cards_kind_checkon every start:Verification
./gradlew check: ktlint and detekt are green. 3410 tests, all green except the two known local-onlyOnDiskOperationsTest > Execfailures (they assume%TEMP%is not inside a git repo; unaffected on CI).BoardServiceTest: baseline set includesTASK_POOL; the pool card lists the whole pool in rank order and marksbestFitand the current task; one ranking per board read;placeOrReviverevives a dismissed card whileplacedoesn't.UserGoalServiceTest: claiming pins viaplaceOrRevive; a 409 claim pins nothing.BoardCardKindConstraintRelaxerTest: exact SQL, and a refusing database doesn't stop startup.🤖 Generated with Claude Code