fix(mobile): 14983 Images upload to shared album with common name

[Tyris]

This PR should address #14983.

The issue is caused because the client just asks for the first album with the matching name - which can result in the shared folder being returned.

The methods in question should never be uploading to an album owned by someone else, so setting shared to false in these cases should work (assuming I understand "shared" correctly - ie: that the item is shared with me).

Above assumption was wrong - have used ownerId instead.

Testing performed:

• Old code (reproduce issue)
  • Create a new user with an album called "Test" and share that to my original user
  • Use the original user to upload an album called "Test" using the "Sync albums" option
  • Verify that files end up in the wrong album ✅
• Using fixed code (verify fix)
  • Using the new user, create an album called "Test2" and share that to my original user
  • Use the original user to upload an album called "Test2" using the "Sync albums" option
  • Verify that a new album was created and images are not synced to the wrong album ✅
• Using fixed code (verify edge case)
  • Using the original user, create an album called "Test3"
  • Use the original user to upload an album called "Test3" using the "Sync albums" option
  • Verify that the images go to the correct existing album ✅

Other notes:

• Messing around with creating/deleting folders and adding to them can cause pretty weird issues because the "album exists" check is done locally; so if the albums aren't synced it will result in creation of a new album with that same name. This was apparent when creating new albums remotely and then immediately doing a sync for an album with the same name on the client - a new album would be created causing a duplicate (yuk!)

  In general, the sync system seems to be a little unreliable across these types of cases... Not sure the best long-term solution - a unified storage engine that handles remote and local through a single interface (think Firebase RTDB) would probably be the cleanest... I tried forcing an remote album refresh before adding assets, but this was non-trivial and expensive.

• Related to my investigation of the above, the refreshRemoteAlbums() call has some strange logic that "works" but isn't really what was intended when written.

  Using getAll(shared: true) gets all shared albums the user can access (regardless of owner, despite the previous comment stating "owned if isShared is false)).

  Using getAll(shared: null) gets all albums (incuding shared = true and shared = false).

  I presume the intent here was to get albums that were shared to me (but not owned by me), and not shared (ie: mine), but the logic is way off since (because shared is set to null, instead of false; but also because shared doesn't actually do this!). It also just then combines them - so makes more sense to just get them in a single call.

  Have simplified this code to do what it should do (without the confusing bits) - would appreciate feedback on this commit in particular: 979ce9

[alextran1502]

Hello, can you write a few words in the PR's description on how you tested this?

[Tyris]

Hello, can you write a few words in the PR's description on how you tested this?

Have updated now that I've had a chance to test. My initial assumption that "shared" means "not mine" was wrong - but I'm not the first person to make this mistake D:

Would appreciate some feedback on my other notes above :)

[alextran1502]

Thank you. I will take a look at this in the coming days

[alextran1502]

@Tyris

In general, the sync system seems to be a little unreliable across these types of cases... Not sure the best long-term solution - a unified storage engine that handles remote and local through a single interface (think Firebase RTDB) would probably be the cleanest... I tried forcing an remote album refresh before adding assets, but this was non-trivial and expensive.

We are refactoring the database ORM; then, we will refactor to a new sync mechanism where the sync differences are calculated on the server. When that happens, we will have more mechanisms to identify when the sync is complete to allow the fetching of albums

[alextran1502]

With this change, you are no longer getting the albums that are shared with you

[Tyris]

I'll go back and test again - but I'm pretty sure that's not the case. When I tested, the two albums that I didn't own (and therefor were returned for the shared query) were also returned in the shared: null query (And also if I removed shared from the query entirely).

shared just means whether an album is shared, not that an album is shared with you. Of course, if an album is shared with you then shared will be true, but shared being true could also mean it's your album that is shared with someone else.

Not having any filters on the query should return all albums you have access to (regardless of the shared flag and regardless of the owner flag) - short of some truly bizarre logic on the backend; I can't see how else this would work (adding a filter to a where clause should never return results that aren't also returned when that filter is omitted).

Of course, you wrote it, so I absolutely would believe I'm wrong, but I just can't see it here :)

[alextran1502]

I created a second user to share some albums with the admin user, then sign in into the admin user and I won't see those albums in the album page. I believe I had these two calls here to fully fetched all albums.

[Tyris]

I can see why this is happening now. My owned albums that I've shared do appear when shared: true, but omitted shared does not work as expected (it does what you suggested: only returns albums you own).

This is obvious looking at https://github.com/immich-app/immich/blob/main/server/src/services/album.service.ts#L47
Notably, from the mobile app, setting shared: false is the same as shared: null (or omitted entirely), which maps to albumRepository.getOwned(). This is misleading at best... but for now I'll revert my change and add some comments to explain proper usage of the inputs 👍

[Tyris]

@Tyris

In general, the sync system seems to be a little unreliable across these types of cases... Not sure the best long-term solution - a unified storage engine that handles remote and local through a single interface (think Firebase RTDB) would probably be the cleanest... I tried forcing an remote album refresh before adding assets, but this was non-trivial and expensive.

We are refactoring the database ORM; then, we will refactor to a new sync mechanism where the sync differences are calculated on the server. When that happens, we will have more mechanisms to identify when the sync is complete to allow the fetching of albums

Is there any doco/discussion on when/how this is happening? It's definitely an area I'm curious to look into closer; but I doubt I can add much value until I understand more of the code-base and wouldn't want to do too much in my own branch if there are other (more reasoned) efforts going on.