Skip to content

Make load_and_authorize_remix deterministic. #696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fspeirs wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −1
Open

Make load_and_authorize_remix deterministic. #696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/controllers/api/projects/remixes_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ def project
end

def load_and_authorize_remix
@project = Project.find_by!(remixed_from_id: project.id, user_id: current_user&.id)
@project = Project.where(remixed_from_id: project.id, user_id: current_user&.id)
.order(created_at: :desc, updated_at: :desc)
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordering here (created_at: :desc, updated_at: :desc) is inconsistent with a similar query in app/controllers/api/lessons_controller.rb:98 which orders remixes by created_at: :asc and only uses created_at for ordering. Consider whether both places should use the same ordering logic, or if there's a specific reason for the difference. If the goal is to consistently return the "most recent" remix, then LessonsController#user_remix should also be updated to use descending order.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@fspeirs fspeirs Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably both should use the most-recently-created logic. Would appreciate a reviewer's view on this.

Copy link
Contributor

@zetter-rpf zetter-rpf Feb 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😵‍💫 This is a bit of a muddle, I was hoping that there wasn't that many affected but I saw you said in the epic it's in the 10s of thousands.

I agree on making everywhere that loads projects consistent before we clear the duplicates and put the unique index in.

Probably both should use the most-recently-created logic.

The thing I'd want to minimize is hiding/losing projects that people have worked on. Picking the most recently created sounds like it might be a good approach, but would it be possible to confirm that? Maybe running a query to see if many duplicate projects with an older created_at have a more recent updated_at?

There are things that hang off of projects ('components' and 'feedback' via 'school_projects') so it might be good to check if they would become hidden.

Copy link
Contributor Author

@fspeirs fspeirs Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. I'll do some more analysis on this before we merge it. I have some SQL in my notes that I can adapt to show some of this, I think. I'm about out of brainpower for today but I'll keep you in the loop.

Copy link
Contributor Author

@fspeirs fspeirs Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zetter-rpf - I have added an SQL query to the related issue for this PR that shows how to identify which students have remixes of which projects.

Fortunately the problem is not as big as first stated - I had forgotten to exclude _fivetran_deleted rows from the result. It ends up being only 318 student/project pairs, most of which only have 2 duplicates.

I don't think feedback records exist for Scratch projects, since that's a feature of editor-ui as I understand it and not available in our Scratch Editor?

.first!
Comment on lines +47 to +49
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query filters on both remixed_from_id and user_id, but there's only a single-column index on remixed_from_id (see db/schema.rb:241). Consider adding a composite index on (remixed_from_id, user_id) to improve query performance, especially since this query is now also ordering by created_at and updated_at. This would make the WHERE clause more efficient and potentially help with the ordering if the index also includes those columns.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@fspeirs fspeirs Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a follow-up task for this in my epic.

authorize! :show, @project
end

Expand Down
35 changes: 35 additions & 0 deletions spec/requests/projects/remix_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,24 @@

expect(response).to have_http_status(:not_found)
end

context 'when multiple remixes exist for the same user and project' do
before do
create(:project, remixed_from_id: original_project.id, user_id: authenticated_user.id,
created_at: 2.days.ago, updated_at: 2.days.ago)
end

let!(:newer_remix) do
create(:project, remixed_from_id: original_project.id, user_id: authenticated_user.id,
created_at: 1.hour.from_now, updated_at: 1.hour.from_now)
end

it 'returns the most recently created remix' do
get("/api/projects/#{original_project.identifier}/remix", headers:)

expect(response.parsed_body['identifier']).to eq(newer_remix.identifier)
end
end
end

describe('#show_identifier') do
Expand Down Expand Up @@ -97,6 +115,23 @@
get("/api/projects/#{original_project.identifier}/remix/identifier", headers:)
expect(response).to have_http_status(:not_found)
end

context 'when multiple remixes exist for the same user and project' do
before do
create(:project, remixed_from_id: original_project.id, user_id: authenticated_user.id,
created_at: 2.days.ago, updated_at: 2.days.ago)
end

let!(:newer_remix) do
create(:project, remixed_from_id: original_project.id, user_id: authenticated_user.id,
created_at: 1.hour.from_now, updated_at: 1.hour.from_now)
end

it 'returns the identifier of the most recently created remix' do
get("/api/projects/#{original_project.identifier}/remix/identifier", headers:)
expect(response.parsed_body['identifier']).to eq(newer_remix.identifier)
end
end
end

describe '#create' do
Expand Down