Skip to content

fix(server): clear activity when asset is removed from album #19019

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
Merged
merged 7 commits into from
Jul 10, 2025
Merged

fix(server): clear activity when asset is removed from album #19019

merged 7 commits into from
Jul 10, 2025

Conversation

dahool
Copy link
Collaborator

@dahool dahool commented Jun 8, 2025 •
edited
Loading

This is an alternative fix for #17475 as discussed in PR #18927

Schema checks will fail since I cannot update activity.table.ts since this change is not supported yet

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

@dahool dahool marked this pull request as ready for review June 9, 2025 13:24
@dahool dahool requested a review from danieldietzler as a code owner June 9, 2025 13:24
@dahool dahool requested review from alextran1502 and mertalev and removed request for alextran1502 June 9, 2025 13:24
@dahool dahool mentioned this pull request Jun 17, 2025
Closed
8 tasks
@dahool dahool mentioned this pull request Jun 18, 2025
Merged
@dahool dahool requested a review from danieldietzler June 18, 2025 19:16
danieldietzler
danieldietzler approved these changes Jun 19, 2025
View reviewed changes
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.

Thank you!

@dahool
Copy link
Collaborator Author

dahool commented Jun 19, 2025

I wonder, should I update the migration to be at the bottom? from my testing, the migration order seems to be important, and since there have been others added after I created the PR, I'm guessing it will fail to run on an existing system

@jrasm91
Copy link
Member

jrasm91 commented Jun 19, 2025 •
edited
Loading

Yes, just change the name to be a bigger number

jrasm91
jrasm91 reviewed Jun 20, 2025
View reviewed changes
Copy link
Member

@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.

Nice!

@danieldietzler
Copy link
Member

Hey @dahool could you fix the merge conflict? Thanks! :)

@dahool
Copy link
Collaborator Author

dahool commented Jul 10, 2025

Hey @dahool could you fix the merge conflict? Thanks! :)

done ;)

@danieldietzler
Copy link
Member

Thanks!

@danieldietzler danieldietzler enabled auto-merge (squash) July 10, 2025 15:22
@danieldietzler danieldietzler disabled auto-merge July 10, 2025 15:22
@danieldietzler
Copy link
Member

danieldietzler commented Jul 10, 2025 •
edited
Loading

Actually I think you need to generate a new SQL migration. We've recently added some more metadata around that

@dahool
Copy link
Collaborator Author

dahool commented Jul 10, 2025

Actually I think you need to generate a new SQL migration. We've recently added some more metadata around that

generating the migration again it just return the same script

@danieldietzler
Copy link
Member

Oh sorry yeah I didn't look at the migration closely enough. Everything that's in there isn't affected.

@danieldietzler danieldietzler merged commit 55fe67d into immich-app:main Jul 10, 2025
48 checks passed
@shenlong-tanwen shenlong-tanwen mentioned this pull request Jul 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrasm91 jrasm91 jrasm91 approved these changes

@danieldietzler danieldietzler danieldietzler approved these changes

@mertalev mertalev mertalev approved these changes

Assignees
No one assigned
Labels
changelog:bugfix 🗄️server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dahool @jrasm91 @danieldietzler @mertalev