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): create a person with optional values #7706

Merged
merged 4 commits into from Mar 7, 2024
Merged

Conversation

jrasm91
Copy link
Contributor

@jrasm91 jrasm91 commented Mar 7, 2024

  • speed up person e2e (remove beforeEach)
  • add option to set birthDate, isHidden, and name when creating a person
  • sort person controller methods
  • rename person service methods
  • refactor person service update method to have less branching

Copy link

cloudflare-pages bot commented Mar 7, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: f3edf69
Status: ✅  Deploy successful!
Preview URL: https://cf940112.immich.pages.dev
Branch Preview URL: https://refactor-person-api.immich.pages.dev

View logs

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

});

it('should not accept invalid birth dates', async () => {
for (const { birthDate, response } of invalidBirthday) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should move the for each out of the it for a more helpful output if the test fails?

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 will do it in a follow up. There is only one test that runs either way right now.

Comment on lines +226 to 228
if (assetId) {
await this.jobRepository.queue({ name: JobName.GENERATE_PERSON_THUMBNAIL, data: { id } });
}
Copy link
Member

Choose a reason for hiding this comment

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

Can't we merge this if with the if starting in l. 214?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the faceId needs to be saved before the job can be queued. I don't want there to be a race condition if the job runs before the update happens.

@jrasm91 jrasm91 merged commit 661409b into main Mar 7, 2024
25 checks passed
@jrasm91 jrasm91 deleted the refactor/person-api branch March 7, 2024 20:34
aviv926 pushed a commit to aviv926/immich that referenced this pull request Mar 7, 2024
* feat: create person dto

* chore: open api

* fix: e2e

* fix: web usage
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

2 participants