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

refactor: move asset stacks to their own entity #6353

Merged
merged 6 commits into from
Jan 27, 2024

Conversation

zackpollard
Copy link
Contributor

@zackpollard zackpollard commented Jan 13, 2024

Refactor to move asset stacks to their own entity. This is the first part of this work. Bundled into this is also some initial work for auto-stacking burst assets.

To Do:

  • Add SQL migrations for existing stacks to move to new table format

Further Work (Different PR(s)):

  • Add FK constraint so primaryAssetId must exist within the same stack
  • Check test cases and add more for asset stacking
  • Finish burst auto-stacking work
  • Refactor stacking to have a asset-stack service and repository, rework API calls from mobile and web to use new APIs for stack operations
  • Migrate live photos to use these stacks (Add stack type to stack entity)

Copy link

cloudflare-pages bot commented Jan 13, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 19dc1d8
Status: ✅  Deploy successful!
Preview URL: https://8b815637.immich.pages.dev
Branch Preview URL: https://feat-auto-stack-burst-photos.immich.pages.dev

View logs

@zackpollard zackpollard force-pushed the feat/auto-stack-burst-photos branch 3 times, most recently from 25fb0ac to 6ff6b9c Compare January 13, 2024 11:56
@zackpollard zackpollard force-pushed the feat/auto-stack-burst-photos branch 3 times, most recently from 2f5a320 to 0937af9 Compare January 14, 2024 02:08
server/src/domain/asset/asset.service.ts Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/response-dto/asset-response.dto.ts Outdated Show resolved Hide resolved
@zackpollard zackpollard force-pushed the feat/auto-stack-burst-photos branch 3 times, most recently from e7a9de8 to eace547 Compare January 15, 2024 14:10
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.

Looks pretty good, although I am having a hard time wrapping my head about the actual stacking/unstacking logic.

server/src/domain/asset/asset.service.spec.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.spec.ts Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/infra/entities/asset.entity.ts Outdated Show resolved Hide resolved
server/src/infra/entities/index.ts Outdated Show resolved Hide resolved
server/src/infra/repositories/asset-stack.repository.ts Outdated Show resolved Hide resolved
server/src/infra/repositories/asset-stack.repository.ts Outdated Show resolved Hide resolved
server/src/domain/repositories/asset-stack.repository.ts Outdated Show resolved Hide resolved
@zackpollard zackpollard force-pushed the feat/auto-stack-burst-photos branch 4 times, most recently from 295f34b to 643a9d5 Compare January 16, 2024 00:13
@jrasm91 jrasm91 force-pushed the feat/auto-stack-burst-photos branch 2 times, most recently from cf8294b to 6df4921 Compare January 27, 2024 05:19
@jrasm91 jrasm91 marked this pull request as ready for review January 27, 2024 05:29
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.

It looks like an entire asset-stack CRUD api exists inside of the bulk asset update endpoint now 😆, besides that I think this looks great. I was able to test creating stacks with the new code, as well as migrating from main to this branch and back again, all without losing existing stacks.

@jrasm91 jrasm91 force-pushed the feat/auto-stack-burst-photos branch from 6df4921 to 19dc1d8 Compare January 27, 2024 05:39
@zackpollard zackpollard merged commit 25cad79 into main Jan 27, 2024
24 checks passed
@zackpollard zackpollard deleted the feat/auto-stack-burst-photos branch January 27, 2024 18:52
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.

None yet

3 participants