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.

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

feat(server, web): smart merge #5796

Merged
merged 11 commits into from
Jan 18, 2024
Merged

feat(server, web): smart merge #5796

merged 11 commits into from
Jan 18, 2024

Conversation

martabal
Copy link
Member

@martabal martabal commented Dec 17, 2023

Changes made in this PR

Currently, when you merge person A with person B, you lose the information you set about person A. With this PR, when you merge person A with person B, the server updates the person's information. person B with person A's information if person B's information is undefined. This only works when you only merge one person with another
person.

fixes #5848

Copy link

cloudflare-pages bot commented Dec 17, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: bea83ae
Status: ✅  Deploy successful!
Preview URL: https://b50def8c.immich.pages.dev
Branch Preview URL: https://feat-smart-merge.immich.pages.dev

View logs

@martabal martabal marked this pull request as ready for review December 17, 2023 18:11
@alextran1502
Copy link
Contributor

From a user perspective, that smart merge toggle isn't clear of what it would do. To improve it, we could make this into a default mechanism.

In the case users are confused about what information about the merger is kept, we can create an info card to show the end result i.e name, birthday, ...etc

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot :)

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Are you going to remove the restriction that this only applies to the first merge person?

server/test/fixtures/person.stub.ts Outdated Show resolved Hide resolved
@jrasm91
Copy link
Contributor

jrasm91 commented Jan 9, 2024

I agree we should just do this by default and no need to make it something the user can turn on and off.

@jrasm91 jrasm91 enabled auto-merge (squash) January 18, 2024 01:50
@jrasm91 jrasm91 merged commit f0b328f into main Jan 18, 2024
21 checks passed
@jrasm91 jrasm91 deleted the feat/smart-merge branch January 18, 2024 01:52
@jrasm91
Copy link
Contributor

jrasm91 commented Jan 18, 2024

I fixed it so that the merged person's data is selectively copied over. The way it was written before could allow data to be overridden in some scenarios, which seemed undesirable.

@mertalev mertalev mentioned this pull request Jan 19, 2024
3 tasks
@martabal martabal restored the feat/smart-merge branch January 19, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Number of assets for person incorrect on merge
4 participants