feat(server, web): include pictures of shared albums on map by andreasgerstmayr · Pull Request #7439 · immich-app/immich

andreasgerstmayr · 2024-02-26T22:29:46Z

This PR adds a new setting "Include shared albums" to the map settings, to includes pictures of albums shared with the current user on the map.

Related: #3176

andreasgerstmayr · 2024-02-26T22:32:29Z

-        exifInfo: {
-          latitude: Not(IsNull()),
-          longitude: Not(IsNull()),
+      where: [


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

…ms-in-map

jrasm91

This looks really good. Thanks!

jrasm91 · 2024-03-29T03:30:31Z

    }
-    return this.assetRepository.getMapMarkers(userIds, options);
+    if (options.withSharedAlbums) {
+      const sharedAlbums = await this.albumRepository.getShared(auth.user.id);


This doesn't include the asset relation does it?

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

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"

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));

jrasm91 · 2024-03-29T03:30:50Z

  @Transform(toBoolean)
  withPartners?: boolean;
+
+  @ApiProperty()


This should not be needed

👍

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

jrasm91 · 2024-03-29T03:32:34Z

+          isVisible: true,
+          isArchived,
+          exifInfo: {
+            latitude: Not(IsNull()),
+            longitude: Not(IsNull()),
+          },
+          isFavorite,
+          fileCreatedAt: OptionalBetween(fileCreatedAfter, fileCreatedBefore),


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

Good idea, done.

jrasm91 · 2024-03-29T03:34:32Z

      <SettingSwitch title="Include shared with me" bind:checked={settings.withPartners} />
+      <SettingSwitch title="Include shared albums" bind:checked={settings.withSharedAlbums} />


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

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?

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

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

danieldietzler · 2024-05-02T09:47:24Z

@andreasgerstmayr You've requested a review from me but you haven't yet addressed @jrasm91 comments and there are also lots of merge conflicts right now...?

andreasgerstmayr · 2024-05-02T11:40:19Z

@andreasgerstmayr You've requested a review from me but you haven't yet addressed @jrasm91 comments and there are also lots of merge conflicts right now...?

hi @danieldietzler, sorry about that, must have been some automated process (it's showing up as 2:35 in the night), maybe because you're listed as a codeowner. No idea why it's happening now and not when I created the PR though.

Sorry for the late response, I was on holiday for a couple of weeks and then didn't get time so far.
Thanks for the review, I'll rebase and implement the changes soon.

danieldietzler · 2024-05-02T12:08:16Z

Ah ok got it. No rush then :)

…ms-in-map

andreasgerstmayr · 2024-05-09T21:36:41Z

I fixed the merge conflicts and implemented (most of) @jrasm91 suggestions now (except the one I didn't understand, see above).
Sorry for the delay on my side!

zackpollard

LGTM, will let @jrasm91 take a look over it before we merge though

zackpollard · 2024-05-13T10:59:38Z

I fixed a tsc error caused by changes in main since the branch was created as well as a prettier error in web. The PR should now be good to merge

jrasm91

I updated this to include assets that are in shared albums where the user is the owner.

andreasgerstmayr · 2024-05-14T20:40:28Z

I updated this to include assets that are in shared albums where the user is the owner.

Good catch! I didn't think of that.

Thank you for merging! :)

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

93cca06

andreasgerstmayr commented Feb 26, 2024

View reviewed changes

michelheusschen added 🖥️web 🗄️server labels Feb 27, 2024

andreasgerstmayr added 3 commits February 27, 2024 23:40

run prettier

f30c44c

Merge remote-tracking branch 'upstream/main' into include-shared-albu…

9e37377

…ms-in-map

re-create api clients

fa66cea

jrasm91 approved these changes Mar 29, 2024

View reviewed changes

jrasm91 self-assigned this Apr 29, 2024

andreasgerstmayr requested a review from danieldietzler as a code owner May 2, 2024 00:35

andreasgerstmayr added 3 commits May 9, 2024 22:43

implement suggestions from code review

40da6ee

Merge remote-tracking branch 'upstream/main' into include-shared-albu…

ab39adf

…ms-in-map

shared from partner -> shared from partners

a1f3bff

rename to 'include shared partner assets'

06640ba

zackpollard approved these changes May 13, 2024

View reviewed changes

zackpollard added 2 commits May 13, 2024 11:47

Merge branch 'refs/heads/main' into fork/include-shared-albums-in-map

15169de

chore: fix tsc error in server and prettier in web

8c2a4e9

zackpollard requested a review from jrasm91 May 13, 2024 10:59

fix: include assets shared via owner albums

803d977

jrasm91 approved these changes May 13, 2024

View reviewed changes

jrasm91 merged commit 48927f5 into immich-app:main May 13, 2024

	@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));

		<SettingSwitch title="Include shared with me" bind:checked={settings.withPartners} />
		<SettingSwitch title="Include shared albums" bind:checked={settings.withSharedAlbums} />

Conversation

andreasgerstmayr commented Feb 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrasm91 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreasgerstmayr May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieldietzler commented May 2, 2024

Uh oh!

andreasgerstmayr commented May 2, 2024

Uh oh!

danieldietzler commented May 2, 2024

Uh oh!

andreasgerstmayr commented May 9, 2024

Uh oh!

zackpollard left a comment

Choose a reason for hiding this comment

Uh oh!

zackpollard commented May 13, 2024

Uh oh!

jrasm91 left a comment

Choose a reason for hiding this comment

Uh oh!

andreasgerstmayr commented May 14, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andreasgerstmayr May 9, 2024 •

edited

Loading