Conversation
sezzim
force-pushed
the
performer-merge
branch
2 times, most recently
from
13bd14b to
b760086
Compare
sezzim
force-pushed
the
performer-merge
branch
from
b760086 to
fd99af6
Compare
|
I think this could potentially be a good addition. I know it is currently supported via plugins but since scenes can do it then might be good for performers. Haven't really looked at the code and there are conflicts to take care of. |
WithoutPants
left a comment
WithoutPants
left a comment
There was a problem hiding this comment.
For expediency, I've addressed the identified issues in the backend code, and written some unit tests - this will be pushed shortly.
The biggest blocker to merging this is the lack of support for merging custom fields. For this reason, I'll defer this for 0.30. I'll sort out custom field support myself during 0.31 if it's not already addressed.
|
Just an additional testing note: merging from the performers page redirected to the scene list filtering on the destination performer, but showing the performer id rather than the name. |
This should be fixed. |
| stash_ids { | ||
| endpoint | ||
| stash_id | ||
| updated_at | ||
| } |
There was a problem hiding this comment.
Yep, this is a merge error.
There was a problem hiding this comment.
Nevermind, this is necessary for the change to tag merge
| tag_filter: filter.makeFilter(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
For consistency, and because I don't think the old logic was correct (it didn't evict the "aliases" field), I changed this to evict the tag rather than update the cache manually.
The "refetch the performer" strategy didn't seem to work, and didn't seem necessary once I added eviction, so I've remove it from all three merge types.
WithoutPants
left a comment
WithoutPants
left a comment
There was a problem hiding this comment.
More static review comments. I'll fix these while testing.
graphql/schema/schema.graphql
Outdated
| performerDestroy(input: PerformerDestroyInput!): Boolean! | ||
| performersDestroy(ids: [ID!]!): Boolean! | ||
| bulkPerformerUpdate(input: BulkPerformerUpdateInput!): [Performer!] | ||
| performerMerge(input: PerformerMergeInput!): Performer |
There was a problem hiding this comment.
This should be Performer!
| for _, src := range sources { | ||
| if err := src.LoadRelationships(ctx, qb); err != nil { | ||
| return fmt.Errorf("loading performer relationships from %d: %w", src.ID, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are we loading relationships here? We're not actually doing anything with the sources.
| stash_ids { | ||
| endpoint | ||
| stash_id | ||
| updated_at | ||
| } |
There was a problem hiding this comment.
Yep, this is a merge error.
Sync extension components with upstream changes since v0.29.3: - PerformerList: Add merge dialog and menu item - TagList: Add merge dialog and menu item - StudioList: Add edit dialog, edit button, and "e" keyboard shortcut - Scene.tsx: Add merge action in operations dropdown These features were added in upstream PRs stashapp#5910 and stashapp#6238 but were missing from our extension implementations. Also update extensions README.md to document these features.
3 participants
Fixes #1351. (The issue is about both performers and scenes, but scene merging was implemented some time ago.)
Also includes (as separate commits) some changes to make the UI of the different merge types consistent, although there's still a difference for scenes, where there's a convenient drop-down, so adding it there seemed preferable to adding a dedicated button.
No tests, and while it worked for me, I don't think that's complete enough coverage of all the fields to call it good. I could probably work on some tests if it generally looks good.