fix: show when a session was actually completed - #89
Merged
Conversation
A session finished by an activity from another day rendered exactly like one finished on time, so the plan quietly implied the work happened on the date it was prescribed for. Reading the week back, a ride done on Friday against a Saturday prescription made Friday look empty. The completing activity's own date is shown when it differs from the target. Full date including the year: plan all spans years, so a bare month-day would be ambiguous exactly where the log is longest. Closes #84 follow-up.
There was a problem hiding this comment.
Pull request overview
This PR fixes a CLI plan rendering bug where a planned session completed by an activity from a different date previously rendered indistinguishably from an on-time completion. It surfaces the completing activity’s actual date in the status column when it differs from the session’s target date, while keeping on-time sessions as plain done.
Changes:
- Extend the plan query to fetch the completing activity’s local date (
done_date) and renderdone (Dow YYYY-MM-DD)whendone_date != target_date. - Update the human
plan/plan alltable rendering to display the new derivedstatus_shown. - Add an e2e regression test covering both the cross-date completion case and the on-time completion case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Plan.roc |
Adds done_date to plan rows and renders a derived status_shown when completion occurred on a different date. |
tests/e2e.roc |
Adds an e2e regression test asserting the human plan table shows the real completion date only when it differs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+421
to
+429
| # Activity 101 lives on ctx.d1, so target a fixed date it cannot coincide with. | ||
| _ = sql!(ctx.db, "INSERT INTO planned_sessions (created_at, target_date, session_type, detail, rationale, status) VALUES ('0','2025-01-15','endurance','early ride','r','open');") | ||
| early_id = Str.trim(sql!(ctx.db, "SELECT MAX(id) FROM planned_sessions;")) | ||
| _ = stride!(ctx.bin, ctx.home, ["complete", early_id, "101"]) | ||
| date_101 = Str.trim(sql!(ctx.db, "SELECT substr(start_local,1,10) FROM activities WHERE id=101;")) | ||
| plan_early = stride_human!(ctx.bin, ctx.home, ["plan", "all"]) | ||
| check!("a session finished on another day shows that day", Str.contains(plan_early, "done (") and Str.contains(plan_early, date_101))? | ||
| check!("a session finished on its target date just says done", Str.contains(plan_early, "│ done "))? | ||
| _ = sql!(ctx.db, "DELETE FROM planned_sessions WHERE target_date = '2025-01-15';") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/e2e.roc:428
- This assertion is a false positive:
"│ done "also matches the early-completed row ("│ done (Fri …)"), so the test would still pass even if on-time sessions incorrectly started rendering with the extra date too. Make the check validate a known on-time session’s rendered status (e.g., id 3) is exactlydone.
plan_early = stride_human!(ctx.bin, ctx.home, ["plan", "all"])
check!("a session finished on another day shows that day", Str.contains(plan_early, "done (") and Str.contains(plan_early, date_101))?
check!("a session finished on its target date just says done", Str.contains(plan_early, "│ done "))?
The control asserted the output contained "| done ", which is a prefix of "| done (Fri ..." — so it passed even when every row carried a date. It now completes a session on its own target date and checks that row's status_shown by id is exactly done, with the early one still carrying its real date. Verified against a build that dates every completed row.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/e2e.roc:439
- Test cleanup deletes by
target_date, which can remove unrelated planned_sessions if any other test data uses that date (or if this test is extended later). Since you already capturedearly_id, delete the specific row by id to keep the scenario isolated and avoid flakiness.
_ = sql!(ctx.db, "DELETE FROM planned_sessions WHERE target_date = '2025-01-15';")
tests/e2e.roc:427
- The human-output assertion is a false positive: earlier in this scenario session id 2 is completed with activity 101, so
plan allcan already containdone (anddate_101even if the newly insertedearly_idrow does not render the completion date. Tie the assertion to the specificearly_idrow (e.g., via.status_shownselection) so the test guards the intended behavior.
date_101 = Str.trim(sql!(ctx.db, "SELECT substr(start_local,1,10) FROM activities WHERE id=101;"))
plan_early = stride_human!(ctx.bin, ctx.home, ["plan", "all"])
check!("a session finished on another day shows that day", Str.contains(plan_early, "done (") and Str.contains(plan_early, date_101))?
The remaining human-output assertion was a second false positive: session
2 is completed with activity 101 earlier in the same scenario, so the
plan already contains "done (" and that date no matter what this row
renders. Both directions now read status_shown for their own id.
Cleanup deleted by target_date, which would take any other row sharing
the date; it deletes the id it created.
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
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.
The bug
A session completed by an activity from a different day rendered exactly like one completed on time. The plan quietly implied the work happened on the date it was prescribed for.
Concretely: a long ride done on Friday against a Saturday prescription made Friday's row look empty and Saturday read a plain
done. Reading the week back, it was impossible to tell the ride had happened a day early — the data was incompleted_activity_idthe whole time, just never surfaced.The fix
Shown only when the completing activity's date differs from the target; an on-time session still reads
done.The year is deliberately kept. I first trimmed it to
done (Fri 08-07)to keep the status column narrow, which was the wrong trade —plan allspans years, so a bare month-day is ambiguous exactly where the log is longest. The wider cell costs one line of wrapping in the detail column.Tests
e2e completes a session with an activity from another date and asserts the row carries that date, while an on-time session still renders a plain
done. Verified as a real guard by disabling the branch and rebuilding — the check fails.The test lives at the END of the plan scenario on purpose: inserting a planned session mid-scenario shifts the auto-increment and breaks the later fixed-id assertions.