Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(server, web): include pictures of shared albums on map #7439

Merged
merged 11 commits into from
May 13, 2024
Merged

feat(server, web): include pictures of shared albums on map #7439

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
93cca06
feat(server, web): include pictures of shared albums on map
andreasgerstmayr Feb 26, 2024
f30c44c
run prettier
andreasgerstmayr Feb 27, 2024
9e37377
Merge remote-tracking branch 'upstream/main' into include-shared-albu…
andreasgerstmayr Feb 27, 2024
fa66cea
re-create api clients
andreasgerstmayr Feb 27, 2024
40da6ee
implement suggestions from code review
andreasgerstmayr May 9, 2024
ab39adf
Merge remote-tracking branch 'upstream/main' into include-shared-albu…
andreasgerstmayr May 9, 2024
a1f3bff
shared from partner -> shared from partners
andreasgerstmayr May 9, 2024
06640ba
rename to 'include shared partner assets'
andreasgerstmayr May 10, 2024
15169de
Merge branch 'refs/heads/main' into fork/include-shared-albums-in-map
zackpollard May 13, 2024
8c2a4e9
chore: fix tsc error in server and prettier in web
zackpollard May 13, 2024
803d977
fix: include assets shared via owner albums
jrasm91 May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions mobile/openapi/doc/AssetApi.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 10 additions & 3 deletions mobile/openapi/lib/api/asset_api.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion mobile/openapi/test/asset_api_test.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions open-api/immich-openapi-specs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,14 @@
"schema": {
"type": "boolean"
}
},
{
"name": "withSharedAlbums",
"required": false,
"in": "query",
"schema": {
"type": "boolean"
}
}
],
"responses": {
Expand Down
23 changes: 18 additions & 5 deletions open-api/typescript-sdk/axios-client/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7932,10 +7932,11 @@ export const AssetApiAxiosParamCreator = function (configuration?: Configuration
* @param {boolean} [isArchived]
* @param {boolean} [isFavorite]
* @param {boolean} [withPartners]
* @param {boolean} [withSharedAlbums]
* @param {*} [options] Override http request option.
* @throws {RequiredError}
*/
getMapMarkers: async (fileCreatedAfter?: string, fileCreatedBefore?: string, isArchived?: boolean, isFavorite?: boolean, withPartners?: boolean, options: RawAxiosRequestConfig = {}): Promise<RequestArgs> => {
getMapMarkers: async (fileCreatedAfter?: string, fileCreatedBefore?: string, isArchived?: boolean, isFavorite?: boolean, withPartners?: boolean, withSharedAlbums?: boolean, options: RawAxiosRequestConfig = {}): Promise<RequestArgs> => {
const localVarPath = `/asset/map-marker`;
// use dummy base URL string because the URL constructor only accepts absolute URLs.
const localVarUrlObj = new URL(localVarPath, DUMMY_BASE_URL);
Expand Down Expand Up @@ -7981,6 +7982,10 @@ export const AssetApiAxiosParamCreator = function (configuration?: Configuration
localVarQueryParameter['withPartners'] = withPartners;
}

if (withSharedAlbums !== undefined) {
localVarQueryParameter['withSharedAlbums'] = withSharedAlbums;
}



setSearchParams(localVarUrlObj, localVarQueryParameter);
Expand Down Expand Up @@ -9071,11 +9076,12 @@ export const AssetApiFp = function(configuration?: Configuration) {
* @param {boolean} [isArchived]
* @param {boolean} [isFavorite]
* @param {boolean} [withPartners]
* @param {boolean} [withSharedAlbums]
* @param {*} [options] Override http request option.
* @throws {RequiredError}
*/
async getMapMarkers(fileCreatedAfter?: string, fileCreatedBefore?: string, isArchived?: boolean, isFavorite?: boolean, withPartners?: boolean, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise<Array<MapMarkerResponseDto>>> {
const localVarAxiosArgs = await localVarAxiosParamCreator.getMapMarkers(fileCreatedAfter, fileCreatedBefore, isArchived, isFavorite, withPartners, options);
async getMapMarkers(fileCreatedAfter?: string, fileCreatedBefore?: string, isArchived?: boolean, isFavorite?: boolean, withPartners?: boolean, withSharedAlbums?: boolean, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise<Array<MapMarkerResponseDto>>> {
const localVarAxiosArgs = await localVarAxiosParamCreator.getMapMarkers(fileCreatedAfter, fileCreatedBefore, isArchived, isFavorite, withPartners, withSharedAlbums, options);
const index = configuration?.serverIndex ?? 0;
const operationBasePath = operationServerMap['AssetApi.getMapMarkers']?.[index]?.url;
return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, operationBasePath || basePath);
Expand Down Expand Up @@ -9407,7 +9413,7 @@ export const AssetApiFactory = function (configuration?: Configuration, basePath
* @throws {RequiredError}
*/
getMapMarkers(requestParameters: AssetApiGetMapMarkersRequest = {}, options?: RawAxiosRequestConfig): AxiosPromise<Array<MapMarkerResponseDto>> {
return localVarFp.getMapMarkers(requestParameters.fileCreatedAfter, requestParameters.fileCreatedBefore, requestParameters.isArchived, requestParameters.isFavorite, requestParameters.withPartners, options).then((request) => request(axios, basePath));
return localVarFp.getMapMarkers(requestParameters.fileCreatedAfter, requestParameters.fileCreatedBefore, requestParameters.isArchived, requestParameters.isFavorite, requestParameters.withPartners, requestParameters.withSharedAlbums, options).then((request) => request(axios, basePath));
},
/**
*
Expand Down Expand Up @@ -9748,6 +9754,13 @@ export interface AssetApiGetMapMarkersRequest {
* @memberof AssetApiGetMapMarkers
*/
readonly withPartners?: boolean

/**
*
* @type {boolean}
* @memberof AssetApiGetMapMarkers
*/
readonly withSharedAlbums?: boolean
}

/**
Expand Down Expand Up @@ -10604,7 +10617,7 @@ export class AssetApi extends BaseAPI {
* @memberof AssetApi
*/
public getMapMarkers(requestParameters: AssetApiGetMapMarkersRequest = {}, options?: RawAxiosRequestConfig) {
return AssetApiFp(this.configuration).getMapMarkers(requestParameters.fileCreatedAfter, requestParameters.fileCreatedBefore, requestParameters.isArchived, requestParameters.isFavorite, requestParameters.withPartners, options).then((request) => request(this.axios, this.basePath));
return AssetApiFp(this.configuration).getMapMarkers(requestParameters.fileCreatedAfter, requestParameters.fileCreatedBefore, requestParameters.isArchived, requestParameters.isFavorite, requestParameters.withPartners, requestParameters.withSharedAlbums, options).then((request) => request(this.axios, this.basePath));
}

/**
Expand Down
6 changes: 4 additions & 2 deletions open-api/typescript-sdk/fetch-client.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions server/src/domain/asset/asset.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
authStub,
faceStub,
newAccessRepositoryMock,
newAlbumRepositoryMock,
newAssetRepositoryMock,
newAssetStackRepositoryMock,
newCommunicationRepositoryMock,
Expand All @@ -21,6 +22,7 @@ import { JobName } from '../job';
import {
AssetStats,
ClientEvent,
IAlbumRepository,
IAssetRepository,
IAssetStackRepository,
ICommunicationRepository,
Expand Down Expand Up @@ -164,6 +166,7 @@ describe(AssetService.name, () => {
let configMock: jest.Mocked<ISystemConfigRepository>;
let partnerMock: jest.Mocked<IPartnerRepository>;
let assetStackMock: jest.Mocked<IAssetStackRepository>;
let albumMock: jest.Mocked<IAlbumRepository>;

it('should work', () => {
expect(sut).toBeDefined();
Expand All @@ -179,6 +182,7 @@ describe(AssetService.name, () => {
configMock = newSystemConfigRepositoryMock();
partnerMock = newPartnerRepositoryMock();
assetStackMock = newAssetStackRepositoryMock();
albumMock = newAlbumRepositoryMock();

sut = new AssetService(
accessMock,
Expand All @@ -190,6 +194,7 @@ describe(AssetService.name, () => {
communicationMock,
partnerMock,
assetStackMock,
albumMock,
);

when(assetMock.getById)
Expand Down
9 changes: 8 additions & 1 deletion server/src/domain/asset/asset.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { IAssetDeletionJob, ISidecarWriteJob, JOBS_ASSET_PAGINATION_SIZE, JobNam
import {
ClientEvent,
IAccessRepository,
IAlbumRepository,
IAssetRepository,
IAssetStackRepository,
ICommunicationRepository,
Expand Down Expand Up @@ -85,6 +86,7 @@ export class AssetService {
@Inject(ICommunicationRepository) private communicationRepository: ICommunicationRepository,
@Inject(IPartnerRepository) private partnerRepository: IPartnerRepository,
@Inject(IAssetStackRepository) private assetStackRepository: IAssetStackRepository,
@Inject(IAlbumRepository) private albumRepository: IAlbumRepository,
) {
this.access = AccessCore.create(accessRepository);
this.configCore = SystemConfigCore.create(configRepository);
Expand Down Expand Up @@ -159,14 +161,19 @@ export class AssetService {

async getMapMarkers(auth: AuthDto, options: MapMarkerDto): Promise<MapMarkerResponseDto[]> {
const userIds: string[] = [auth.user.id];
const albumIds: string[] = [];
if (options.withPartners) {
const partners = await this.partnerRepository.getAll(auth.user.id);
const partnersIds = partners
.filter((partner) => partner.sharedBy && partner.sharedWith && partner.sharedById != auth.user.id)
.map((partner) => partner.sharedById);
userIds.push(...partnersIds);
}
return this.assetRepository.getMapMarkers(userIds, options);
if (options.withSharedAlbums) {
const sharedAlbums = await this.albumRepository.getShared(auth.user.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't include the asset relation does it?

Copy link
Contributor Author

@andreasgerstmayr andreasgerstmayr May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, where should I include the asset relation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so Jason answered me on Discord about this:
https://discord.com/channels/979116623879368755/994044917355663450/1238973376349864108

Jason said: "Just want to make sure this is not loading the album AND all the assets for it"

I said: "If I understand correctly, you are asking if it is loading only the shared albums when figuring out what to show on the map, versus loding the shared albums and all the assets. And the problem with that is loading all the assets in the shared album takes more resources."

Jason responded: "Yup"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this function does not query all assets, only the albums:

immich/server/src/repositories/album.repository.ts

Lines 154 to 166 in f667c95

@GenerateSql({ params: [DummyValue.UUID] })
async getShared(ownerId: string): Promise<AlbumEntity[]> {
const albums = await this.repository.find({
relations: { albumUsers: { user: true }, sharedLinks: true, owner: true },
where: [
{ albumUsers: { userId: ownerId } },
{ sharedLinks: { userId: ownerId } },
{ ownerId, albumUsers: { user: Not(IsNull()) } },
],
order: { createdAt: 'DESC' },
});
return albums.map((album) => withoutDeletedUsers(album));

albumIds.push(...sharedAlbums.map((album) => album.id));
}
return this.assetRepository.getMapMarkers(userIds, albumIds, options);
}

async getMemoryLane(auth: AuthDto, dto: MemoryLaneDto): Promise<MemoryLaneResponseDto[]> {
Expand Down
6 changes: 6 additions & 0 deletions server/src/domain/asset/dto/map-marker.dto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,10 @@ export class MapMarkerDto {
@IsBoolean()
@Transform(toBoolean)
withPartners?: boolean;

@ApiProperty()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

The class got moved to server/src/dtos/search.dto.ts and doesn't have this annotation anymore.

@Optional()
@IsBoolean()
@Transform(toBoolean)
withSharedAlbums?: boolean;
}
2 changes: 1 addition & 1 deletion server/src/domain/repositories/asset.repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export interface IAssetRepository {
softDeleteAll(ids: string[]): Promise<void>;
restoreAll(ids: string[]): Promise<void>;
findLivePhotoMatch(options: LivePhotoSearchOptions): Promise<AssetEntity | null>;
getMapMarkers(ownerIds: string[], options?: MapMarkerSearchOptions): Promise<MapMarker[]>;
getMapMarkers(ownerIds: string[], albumIds: string[], options?: MapMarkerSearchOptions): Promise<MapMarker[]>;
getStatistics(ownerId: string, options: AssetStatsOptions): Promise<AssetStats>;
getTimeBuckets(options: TimeBucketOptions): Promise<TimeBucketItem[]>;
getTimeBucket(timeBucket: string, options: TimeBucketOptions): Promise<AssetEntity[]>;
Expand Down
38 changes: 27 additions & 11 deletions server/src/infra/repositories/asset.repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ export class AssetRepository implements IAssetRepository {
});
}

async getMapMarkers(ownerIds: string[], options: MapMarkerSearchOptions = {}): Promise<MapMarker[]> {
async getMapMarkers(ownerIds: string[], albumIds: string[], options: MapMarkerSearchOptions = {}): Promise<MapMarker[]> {
const { isArchived, isFavorite, fileCreatedAfter, fileCreatedBefore } = options;

const assets = await this.repository.find({
Expand All @@ -511,19 +511,35 @@ export class AssetRepository implements IAssetRepository {
longitude: true,
},
},
where: {
ownerId: In([...ownerIds]),
isVisible: true,
isArchived,
exifInfo: {
latitude: Not(IsNull()),
longitude: Not(IsNull()),
where: [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to write this OR expression?
WHERE (ownerID IN ... OR albumId IN ...) AND isVisible = ... AND isArchived = ... AND ...

{
ownerId: In([...ownerIds]),
isVisible: true,
isArchived,
exifInfo: {
latitude: Not(IsNull()),
longitude: Not(IsNull()),
},
isFavorite,
fileCreatedAt: OptionalBetween(fileCreatedAfter, fileCreatedBefore),
},
isFavorite,
fileCreatedAt: OptionalBetween(fileCreatedAfter, fileCreatedBefore),
},
{
albums: {
id: In([...albumIds])
},
isVisible: true,
isArchived,
exifInfo: {
latitude: Not(IsNull()),
longitude: Not(IsNull()),
},
isFavorite,
fileCreatedAt: OptionalBetween(fileCreatedAfter, fileCreatedBefore),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is repeated twice. Can you move it up to a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done.

}
],
relations: {
exifInfo: true,
albums: true,
},
order: {
fileCreatedAt: 'DESC',
Expand Down
1 change: 1 addition & 0 deletions web/src/lib/components/map-page/map-settings-modal.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
<SettingSwitch title="Only favorites" bind:checked={settings.onlyFavorites} />
<SettingSwitch title="Include archived" bind:checked={settings.includeArchived} />
<SettingSwitch title="Include shared with me" bind:checked={settings.withPartners} />
<SettingSwitch title="Include shared albums" bind:checked={settings.withSharedAlbums} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remember this other option so it is more obvious what the difference is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed Include shared with me to Include shared from partners. Another option would be Include assets from partners.

What do you think, which option sounds better? Or do you have other suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not Jason but I would suggest Include partner assets or Include shared partner assets. Also, this rename is important for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to Include shared partner assets now, thanks for the suggestion @NicholasFlamy

{#if customDateRange}
<div in:fly={{ y: 10, duration: 200 }} class="flex flex-col gap-4">
<div class="flex items-center justify-between gap-8">
Expand Down
2 changes: 2 additions & 0 deletions web/src/lib/stores/preferences.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export interface MapSettings {
includeArchived: boolean;
onlyFavorites: boolean;
withPartners: boolean;
withSharedAlbums: boolean;
relativeDate: string;
dateAfter: string;
dateBefore: string;
Expand All @@ -57,6 +58,7 @@ export const mapSettings = persisted<MapSettings>('map-settings', {
includeArchived: false,
onlyFavorites: false,
withPartners: false,
withSharedAlbums: false,
relativeDate: '',
dateAfter: '',
dateBefore: '',
Expand Down
3 changes: 2 additions & 1 deletion web/src/routes/(user)/map/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
}
abortController = new AbortController();

const { includeArchived, onlyFavorites, withPartners } = $mapSettings;
const { includeArchived, onlyFavorites, withPartners, withSharedAlbums } = $mapSettings;
const { fileCreatedAfter, fileCreatedBefore } = getFileCreatedDates();

return await getMapMarkers(
Expand All @@ -56,6 +56,7 @@
fileCreatedAfter: fileCreatedAfter || undefined,
fileCreatedBefore,
withPartners: withPartners || undefined,
withSharedAlbums: withSharedAlbums || undefined,
},
{
signal: abortController.signal,
Expand Down
Loading