Fix volume misplaced

[frankiejol]

List volumes by order

[Copilot]

Pull request overview

This pull request fixes an issue where volumes were being listed in the wrong order by changing the SQL query to order by n_order instead of id. The n_order column is the intended field for maintaining volume ordering, while id is just the primary key which may not reflect the correct sequence.

Changes:

• Modified volume listing query in lib/Ravada/Front/Domain.pm to order by n_order column instead of id
• Added a test in t/vm/40_volumes.t to verify volumes are returned in correct order even when IDs are out of sequence

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
lib/Ravada/Front/Domain.pm	Changed ORDER BY clause from id to n_order to fix volume ordering
t/vm/40_volumes.t	Added test functions _swap_volumes_id and test_order to validate volume ordering fix

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The UPDATE statement is missing the table name. This SQL query will fail with a syntax error. The correct syntax should be "UPDATE volumes SET id=? WHERE id=?".

[Copilot]

The test attempts to directly manipulate the volume ID in the database to simulate volumes being out of order. However, directly changing the primary key ID may violate database constraints and doesn't accurately test the ordering fix. The fix addresses ordering by n_order, so the test should manipulate the n_order column instead to properly validate that volumes are returned in the correct order regardless of their ID values.

Suggested change

	"UPDATE volumes set id=? WHERE id=?"
	);
	$sth_change->execute($volumes[-1]->{id}+1 , $volumes[1]->{id});
	$sth->execute($domain->id);
	"UPDATE volumes set n_order=? WHERE id=?"
	);
	$sth_change->execute($volumes[-1]->{n_order}+1 , $volumes[1]->{id});
	$sth->execute($domain->id);
	@volumes = ();
	while (my $row = $sth->fetchrow_hashref) {
	push @volumes,($row);
	}

[Copilot]

After executing the UPDATE statement that changes a volume ID, the prepared statement is re-executed but the results are never fetched or used. This line appears to serve no purpose and should either be removed or its results should be validated as part of the test assertions.

Suggested change

$sth->execute($domain->id);

[Copilot]

The test assumes there are at least 4 volumes (indices 0-3) by iterating from 0 to 3 and accessing array elements. However, there's no verification that the domain actually has 4 volumes. If the domain has fewer volumes, this will cause the test to access undefined array elements, leading to failures or undefined behavior. Add a check to verify that @volumes has at least 4 elements before proceeding with the test assertions.

[Copilot]

The function name _swap_volumes_id is misleading because the function doesn't actually swap volumes. It attempts to change one volume's ID and then validates the ordering. A more accurate name would be _test_volume_ordering or _validate_volume_order_after_id_change to better reflect what the function does.

[Copilot]

The test_order function creates a domain but never cleans it up by calling $domain->remove(user_admin). This will leave test domains in the system after the test runs, potentially causing issues with subsequent test runs or consuming resources. Add proper cleanup to remove the domain after testing.

Suggested change

	_swap_volumes_id($domain);
	_swap_volumes_id($domain);
	$domain->remove(user_admin);