feat(server, web): unassign faces #9374
Conversation
martabal
force-pushed
the
feat/unassign-faces
branch
from
7474201
to
7e2faf8
Compare
Deploying immich with
|
| Latest commit: |
0f6e665
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://99410572.immich.pages.dev |
| Branch Preview URL: | https://feat-unassign-faces.immich.pages.dev |
martabal
force-pushed
the
feat/unassign-faces
branch
16 times, most recently
from
7e7454a
to
0c3a058
Compare
martabal
force-pushed
the
feat/unassign-faces
branch
from
0c3a058
to
7e9dcaa
Compare
|
|
||
| await expect(sut.unassignFace(authStub.admin, faceStub.face1.id)).resolves.toStrictEqual( | ||
| mapFaces(faceStub.unassignedFace, authStub.admin), | ||
| ); |
There was a problem hiding this comment.
I think it would make sense to also expect some of the repo methods to have been called. Specifically that requirePermission gets called for PERSON_WRITE. Also, I would like to see a test without a personId, so that you don't run into that if but instead run into the createNewFeaturePhoto method. Basically try to cover the relevant branches.
|
|
||
| await expect( | ||
| sut.unassignFaces(authStub.admin, { data: [{ assetId: faceStub.face1.id, personId: 'person-1' }] }), | ||
| ).resolves.toStrictEqual([{ id: 'assetFaceId1', success: true }]); |
There was a problem hiding this comment.
Here as well, some branches remain untested.
| const changeFeaturePhoto: string[] = []; | ||
| const results: BulkIdResponseDto[] = []; |
There was a problem hiding this comment.
My OCD really likes that these two lines have the exact same length lmao
There was a problem hiding this comment.
They just line up so perfectly!
There was a problem hiding this comment.
What's the problem with it ?
There was a problem hiding this comment.
Oh nothing, I was just happy about that lol
There was a problem hiding this comment.
That was a positive comment :D
There was a problem hiding this comment.
Oh damn, I just saw it. Satisfying 😄
There was a problem hiding this comment.
The html in here is pretty complex (deeply nested, many branches, etc). I'm not really sure how to properly fix this? Maybe it's just a general issue of the page structure? Maybe sub-components could help as well?
There was a problem hiding this comment.
I'm not sure either, maybe I chose the wrong way to do this
web/src/lib/components/faces-page/unassigned-faces-side-panel.svelte
Outdated
Show resolved
Hide resolved
| people?: PersonWithFacesResponseDto[]; | ||
| people?: PeopleWithFacesResponseDto; |
There was a problem hiding this comment.
This is a breaking change! I'm not really sure how to handle this case, as it isn't covered by the PropertyLifecycle idea. The proper approach would probably be to introduce a new variable? Interested for @jrasm91 opinion on this though.
There was a problem hiding this comment.
Yep that's a breaking change. It's fine to have another variable, but I prefer to have everything in people.
Another simple fix would be to send all faces: currently we only filter faces associated with a person. But I prefer to represent faces/people like that:
"people": {
"visiblePeople": [
{
"birthDate": null,
"id": "b70e6b20-2dce-4184-aa20-dac1c9717a16",
"isHidden": false,
"name": "Test Person",
"thumbnailPath": "thumbnail.jpg",
"faces": [
{
"id": "ef8c6a56-2891-424b-a521-11c673e9bfd3",
"imageHeight": 1440,
"imageWidth": 1920,
"boundingBoxX1": 1494,
"boundingBoxX2": 1705,
"boundingBoxY1": 430,
"boundingBoxY2": 733
}
]
}
],
"numberOfFaces": 2
}
Co-authored-by: Daniel Dietzler <[email protected]>
…svelte Co-authored-by: Daniel Dietzler <[email protected]>
…svelte Co-authored-by: Daniel Dietzler <[email protected]>
martabal
force-pushed
the
feat/unassign-faces
branch
3 times, most recently
from
5c42546
to
ff865e3
Compare
martabal
force-pushed
the
feat/unassign-faces
branch
from
ff865e3
to
8f683f8
Compare
martabal
force-pushed
the
feat/unassign-faces
branch
from
8f683f8
to
0f6e665
Compare
Continuing #5511, using the UI proposed here