web (job submission): enumerate and order batches correctly#6452
web (job submission): enumerate and order batches correctly#6452
Conversation
Also code cleanup
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the batch enumeration and ordering system in the job submission web interface. The key changes include replacing database-level ordering with in-memory sorting, implementing consistent ordering across different batch states, and reorganizing function names for better clarity.
- Removes database ORDER BY clauses and implements client-side sorting for proper batch ordering
- Introduces specialized handling for in-progress batches that don't have completion times
- Renames functions and actions for improved consistency and clarity throughout the interface
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| html/user/submit.php | Major refactoring of batch sorting logic, function renaming, and code cleanup |
| html/inc/bootstrap.inc | Updates navigation link to use new action name |
| $batches[] = $batch; | ||
| } | ||
| function show_aborted($all_batches, $order, $limit, $user, $app) { | ||
| $batches = batches_in_state($all_batches, BATCH_STATE_ABORTED); |
There was a problem hiding this comment.
The show_aborted function receives an $order parameter but doesn't use it for sorting, unlike show_in_progress and show_complete functions. Consider adding sort_batches($batches, $order); after line 271 for consistency.
| $batches = batches_in_state($all_batches, BATCH_STATE_ABORTED); | |
| $batches = batches_in_state($all_batches, BATCH_STATE_ABORTED); | |
| sort_batches($batches, $order); |
| $f = function($a, $b) { | ||
| return (int)($b->completion_time - $a->completion_time); | ||
| }; | ||
| break; |
There was a problem hiding this comment.
The variable $f is used without being initialized when the $order parameter doesn't match any of the expected cases ('sub_asc', 'sub_desc', 'comp_asc', 'comp_desc'). This will cause a fatal error. Add a default case or validate the $order parameter before the switch statement.
| break; | |
| break; | |
| default: | |
| $f = function($a, $b) { | |
| return (int)($a->create_time - $b->create_time); // Default to 'sub_asc' | |
| }; | |
| break; |
Also code cleanup