Closed NicholasMy closed 5 months ago
app/views/assessments/index.html.erb (1)
`198-198`: Ensure backend ordering is correctly implemented.Verification successful
--- The backend ordering logic for attachments appears to be correctly implemented. The `.ordered` scope is actively used in various controller files for handling attachments, which aligns with the expected behavior following the removal of `.ordered` from the view layer. This confirms that the ordering is managed at the controller or model level, ensuring that the data is correctly ordered before reaching the view.Scripts Executed
The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Verify that the backend ordering logic for attachments is correctly implemented. # This script checks for the presence of the `.ordered` scope in the `CourseAttachment` model. rg --type ruby --glob '*_controller.rb' '.ordered' ``` Length of output: 899
Thanks for catching this - it was an oversight on my part
Just a thought: course attachments are ordered on the course page due to @course_attachments.from_category(cat).ordered
.
For consistency, I think either
assessments_controller.rb#index
render @attachments.ordered
Option (1) is probably better, in which case the same .ordered
scope should be applied when defining @course_attachments
. This presumes that applying a where
filter (via from_category
) preserves order, although I can't find anything information online regarding this.
What do you think?
I originally experimented with option 2 while I was tracing the source of the lack of ordering, but I settled on option 1 (for assessment attachments) because I can't think of a scenario where we'd want the unordered attachments. I think doing the same for course attachments makes sense.
where
appears to preserve the order, but I also can't find any concrete confirmation online. I tested making these modifications. If I keep the incorrect order (by not calling ordered
anywhere) and apply the where
, it keeps the incorrect order. The SQL query doesn't include any ordering:
SELECT `attachments`.* FROM `attachments` WHERE `attachments`.`course_id` = 28 AND `attachments`.`assessment_id` IS NULL AND `attachments`.`category_name` = 'General3'
If I correctly order them before applying the where
, it keeps the correct order, and the resulting SQL still explicitly orders it, so I don't think this is a coincidence.
SELECT `attachments`.* FROM `attachments` WHERE `attachments`.`course_id` = 28 AND `attachments`.`assessment_id` IS NULL AND `attachments`.`category_name` = 'General3' ORDER BY release_at ASC, name ASC
I think doing the same for course attachments makes sense.
That sounds good to me, could you make the change to order course attachments on the backend too?
If I correctly order them before applying the where, it keeps the correct order, and the resulting SQL still explicitly orders it, so I don't think this is a coincidence.
In that case, I think it's safe to remove the ordering for course attachments on the frontend. Thanks for investigating this!
No problem, how does this look?
Description
This update fixes the sorting order of assessment attachments.
Motivation and Context
When rendering attachments on the assessment page, they're ordered how the database returns them instead of by the desired "release_at ASC, name ASC" ordering constant. Course attachments are correct and it already calls
.ordered
in the same way I did here. If an instructor wants to re-order attachments, they currently need to delete them and re-upload them. This fix would allow them to only update the release date to re-order them. I also believe this was the intended behavior, but forgetting to call.ordered
was an oversight.How Has This Been Tested?
I created two course attachments, A and B, with B's release date after A's. I verified B appears after A in the list. I updated B's release date to be before A's. I verified B appears before A in the list.
Types of changes
Potentially breaking change: Some instructors may depend on the "manual" method of ordering attachments as described above. That was ultimately a bug, and it never should have behaved that way, but maybe this should wait until the semester is over.
Checklist:
overcommit --install && overcommit --sign
to use pre-commit hook for lintingSummary by CodeRabbit