-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(mobile): repositories for album service #12701
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a great start to me. Basically no real code changes, just separated out the database code. With some tests and future iterations it should make future improvements a lot easier.
import 'package:immich_mobile/entities/user.entity.dart'; | ||
|
||
abstract interface class IAssetRepository { | ||
Future<List<Asset>> getByAlbumWithOwnerUnequal(Album album, User user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is a bit unwieldy, maybe just getAlbums({ notOwnedBy}). Also why do you need to get albums not owned by the user? Is that essentially shared with user? Would albumRepo.getSharedWithMe work as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should actually return the Asset
s belonging to a specific album but not owned by user x. I've modified the signature; it's now easier to read
In the server we try to avoid having services depend on other services. Common code used by two is extracted out to a utility file or class, and then testing only has to inject repository mocks. Do you think we can eventually get to that here as well? |
I think we can move in that direction with mobile as well. It will certainly take a few more PRs but it's doable (maybe we need to rename the sync service to a repository :-P) |
@fyfrey feel free to merge these refactor PRs after I give an approval. |
This PR adds repositories for the album service to access the local database.
Removes all direct dependencies on Isar within the album service.
Could only add single unit test so far (because it first needs repositories for OpenAPI interaction to test any other methods of the album service)