Ensure consistent sorting of albums in list of shares

[nagmat84]

And another pick of cherries from #1414. This PR ensures that the list of available albums (i.e. those albums which aren't shared yet) in the list of shares is consistent and deterministic.

[codecov]

Codecov Report

Merging #1429 (0e3a393) into master (ab9d834) will decrease coverage by 0.81%.
The diff coverage is 100.00%.

[kamil4]

I agree that the list should be explicitly ordered by something but why is hardcoding the title the right thing to do? Shouldn't the list be ordered by what the user specified in the settings?

[nagmat84]

I agree that the list should be explicitly ordered by something but why is hardcoding the title the right thing to do? Shouldn't the list be ordered by what the user specified in the settings?

I had the exact same thought, but then I looked at the remaining code of this method and all other sorting is hard-coded, too. For example, the list of existing shares is also sorted according to the album title (and then by user name) in a hard-coded manner. I wanted to keep this code consistent.

Of course we could sort the list of albums without shares according to the user-defined sorting order, but then we should do this for the list of existing shares as well. This means more changes to the code than this PR.

I assume the choice was made, because the list of shares does not show any previews in form of thumbnails. if the user had selected an ordering with respect to updated_at the list would always appear to be sorted differently with no obvious reason.

[ildyria]

The code becomes:

$albums_query = DB::table('base_albums')
    ->leftJoin('albums', 'albums.id', '=', 'base_albums.id')
    ->select(['base_albums.id', 'title', 'parent_id'])
    ->orderBy('title', 'ASC');  // added by this PR.

[...]

$albums = $albums_query->get();
$this->linkAlbums($albums);
$albums->each(function ($album) {
$album->title = $this->breadcrumbPath($album);
});
$albums->each(function ($album) {
unset($album->parent_id);
unset($album->parent);
});

However I would believe this is not the behavior we want.
If we have the following structure:

--+ B2
  | 
  +-- A2
  |
  +-- B1
  |
  +-- D
--+ C1
  | 
  +-- A1

If I read the code properly, the resulting list will be

C1/A1
B2/A2
B2/B1
B2
C1
B2/D

While I would expect the following:

B2
B2/A2
B2/B2
B2/D
C1
C1/A1

I am not saying it is better than current behavior, but I don't think this is the direction we completely want to go in.

[nagmat84]

However I would believe this is not the behavior we want. If we have the following structure:

--+ B2
  +-- A2
  +-- B1
  +-- D
--+ C1
  +-- A1

If I read the code properly, the resulting list will be

C1/A1
B2/A2
B2/B1
B2
C1
B2/D

No, that is incorrect.

While I would expect the following:

B2
B2/A2
B2/B2
B2/D
C1
C1/A1

That is exactly what happens.

The code still builds a proper tree. It has done that before (even with indeterministic ordering) and it will still do so afterwards.

The only question is if we are fine with a hard-coded ordering wrt. the title as we already do in the same method for existing shares or if we want to use the globally defined default ordering here, but then we should also use it for existing shares.

[ildyria]

LGTM.