docs(log_set): say that exercise_id is the movement performed - #19
Open
wromansky wants to merge 2 commits into
Open
docs(log_set): say that exercise_id is the movement performed#19wromansky wants to merge 2 commits into
wromansky wants to merge 2 commits into
Conversation
log_set documents routine_id, slot_entry_id, iteration, reps_unit, the *_target fields, session_id and next_log_id. exercise_id is the one parameter the docstring never mentions, and the ids paragraph tells the caller to "get all three from get_workout_for_date" — which returns exercise_id and slot_entry_id together inside each planned entry. The result is that the only exercise ids a caller holds are the planned ones, and nothing says they may not be the right ones. Substituted work gets filed under the planned exercise: a rope pushdown stored as a machine pushdown, a cable lateral raise stored as a machine lateral raise. The measurements are right and the movement is wrong, and no later reading of the log can tell. The next prescription for the substitute then sees no history at all, because its own id was never written. wger accepts the correct call. manager_workoutlog has no constraint tying exercise_id to slot_entry_id, and WorkoutLogSerializer declares exercise and slot_entry as independent fields with no cross-field validation, so the substitute's exercise_id can be passed while routine_id, slot_entry_id and iteration still point at the planned slot. That keeps the set attached to the plan and the history honest. Docstring only; no behaviour change.
The previous wording told callers to read exercise_id off the movement performed and 'not off the planned entry they came from'. Read literally that forbids the common case: with no substitution the planned entry's exercise_id IS the movement performed, and a caller following the letter would search for an id it already holds — an extra round trip per set, and a near-neighbour match would reintroduce a milder form of the mis-filing this paragraph exists to prevent. Say instead that get_workout_for_date's planned entries carry an exercise_id of their own, that it is the movement PLANNED, and that it should be passed only when it is also the one performed. That names the trap more explicitly than before while leaving the common path free.
wromansky
marked this pull request as ready for review
September 1, 2026 19:30
This was referenced Sep 1, 2026
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.
log_setdocumentsroutine_id,slot_entry_id,iteration,reps_unit, the*_targetfields,session_idandnext_log_id.exercise_idis the one parameter it never mentions — and the ids paragraph tells the caller to "get all three fromget_workout_for_date", which returnsexercise_idandslot_entry_idtogether inside each planned entry.So the only exercise ids a caller holds are the planned ones, and nothing says they may not be the right ones.
What goes wrong
Substituted work gets filed under the planned exercise. A machine is occupied, the trainee does the cable or rope version, and the set is written with the slot's planned
exercise_id:The measurements are correct and the movement is wrong, and no later reading of the log can tell the two apart. Two things break downstream:
wger accepts the correct call
Verified rather than assumed:
manager_workoutloghas no constraint tyingexercise_idtoslot_entry_id— they are separate foreign keys.WorkoutLogSerializer(wger/manager/api/serializers.py) declaresexerciseandslot_entryas independent fields, with novalidate()and no cross-field check.So the substitute's
exercise_idcan be passed whileroutine_id,slot_entry_idanditerationstill point at the planned slot. The set stays attached to the plan, and the history stays true to what was lifted. That combination was always available; nothing in the docstring said so.The change
Docstring only, no behaviour change.
exercise_idis the movement actually performed, names the substitution case, and says to keep the plan ids pointing at the planned slot while passing the substitute's own id.get_workout_for_date's planned entries carry anexercise_idof their own, that it is the movement planned, and that it should be passed only when it is also the one performed. The old "get all three fromget_workout_for_date" phrasing is what invited copying the whole bundle across.search_exercisesfor finding the substitute's id, since that call is the step a caller has no reason to make otherwise.Tests
No new tests: the change is documentation and there is no behaviour to assert. Full suite unchanged at 247 passed.
ruff checkandruff format --checkclean on the touched file.Note
#18 also edits the
log_setdocstring, in a different paragraph (reps_unit). Checked withgit merge-tree: the hunks do not overlap and the two branches merge cleanly, so neither blocks the other.