Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the load_and_authorize_remix method deterministic by explicitly ordering remix records when multiple records exist with the same (remixed_from_id, user_id) pair. The fix addresses a production issue where Experience CS was receiving inconsistent project identifiers due to nondeterministic query results. The change ensures that users always receive their most recently created remix instead of a random one.
Changes:
- Modified
RemixesController#load_and_authorize_remixto use.where().order().first!instead offind_by!, ordering bycreated_at: :desc, updated_at: :descto deterministically return the most recent remix
fspeirs
force-pushed
the
1186-editor-api-remix-lookup-should-deterministically-return-the-most-recently-created-record
branch
2 times, most recently
from
529bb80 to
dac5345
Compare
Test coverage89.74% line coverage reported by SimpleCov. |
fspeirs
force-pushed
the
1186-editor-api-remix-lookup-should-deterministically-return-the-most-recently-created-record
branch
from
2e4a84f to
052c5fc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
This commit changes RemixesController#load_and_authorize_remix to deterministically always return the most recent Project record based on created_at and, secondarily, updated_at dates. While, in principle, there should not be multiple records where (remixed_from_id:, user_id:) are the same, in practice there are and there is no database constraint to prevent there being. The previous code would nondeterministically return one of possibly many records, causing downstream errors in Experience CS since each would have a different identifier, not matching the one stored in Experience CS. Overarching epic: RaspberryPiFoundation/experience-cs#1826
This commit factors out the remix selection logic that was common between LessonsController#user_remix and RemixesController#load_and_authorize_remix into a new concern called RemixSelection that is used in both controllers. RemixSelection instantiates the rule that, for any user, the canonical remix of a project is the remix which was created first. Any subsequent remixes will not be accessible.
fspeirs
force-pushed
the
1186-editor-api-remix-lookup-should-deterministically-return-the-most-recently-created-record
branch
from
052c5fc to
4cf245d
Compare
zetter-rpf
left a comment
zetter-rpf
left a comment
There was a problem hiding this comment.
Nice one! thanks for spending the time with me to work it out.
fspeirs
deleted the
1186-editor-api-remix-lookup-should-deterministically-return-the-most-recently-created-record
branch
This commit changes
RemixesController#load_and_authorize_remixto deterministically always return the oldest remix, matching the logic that currently exists inLessonsController#user_remix.While, in principle, there should not be multiple records where
(remixed_from_id:, user_id:)are the same, in practice there are and there is no database constraint to prevent there being.The previous code would nondeterministically return one of possibly many records, causing downstream errors in Experience CS since each would have a different identifier, not matching the one stored in Experience CS.
Overarching epic:
https://github.com/RaspberryPiFoundation/experience-cs/issues/1826
Status
Points for consideration:
What's changed?
I factored out the remix selection logic from
LessonsController#user_remixinto a new concern and reused that logic there and inRemixesController#load_and_authorize_projectso now both endpoints should be consistent in which remix they select for which user and project.Steps to perform after deploying to production
There are no additional steps required and this behaviour change should only affect users who have multiple remix records - these users will now only be able to access their oldest remix instead of getting a random one back.