Conversation
MonkeyDo
left a comment
MonkeyDo
left a comment
There was a problem hiding this comment.
Nice going !
Thanks a lot, that's going to be very useful now that I have to implement a similar checkboc for copying Author Credits.
I might just copy your homework :p
When I first tried the feature, I got an error on submission: Submission Error: No Updated Field.
Having the checkbox as the only change in the form isn't considered a change to the Edition, which sort of makes sense, but I think we should allow this workflow.
| <Form.Check | ||
| className="margin-bottom-d8" | ||
| id="name" | ||
| label="Copy the edition name to edition-group" |
There was a problem hiding this comment.
Should be "Edition" and "the Edition Group".
I'm not fully convinced by the placement, but I can't think of a better way right now :/
Maybe below the disambiguation?
Or in the Edition Group section below?
There was a problem hiding this comment.
Maybe below the disambiguation?
This does sound like a reasonable place for checkbox
| if (!isNew && entityType === 'Edition' && body.copyToEG) { | ||
| const currentEditionGroup = currentEntity.editionGroup; | ||
| const entityDefaultAlias = body.aliases.find((alias) => alias.default); | ||
| if (currentEditionGroup.defaultAlias.name !== entityDefaultAlias.name) { |
There was a problem hiding this comment.
We probably also want to make sure the sort name and language are the same. The getNextAliasSet function takes care of that, and returns the existing alias if unchanged, which shouldn't result in any changes on the EG.
So you should be able to remove this if condition.
| .filter(entity => entity.get('dataId') !== null); | ||
|
|
||
| // copy name to the edition group | ||
| if (!isNew && entityType === 'Edition' && body.copyToEG) { |
There was a problem hiding this comment.
Let's make this a separate copyNameToEditionGroup function in an effort to keep handleCreateOrEditEntity more readable
| const currentEditionGroup = currentEntity.editionGroup; | ||
| const entityDefaultAlias = body.aliases.find((alias) => alias.default); | ||
| if (currentEditionGroup.defaultAlias.name !== entityDefaultAlias.name) { | ||
| const tempBody = {aliases: [{...currentEditionGroup.defaultAlias, default: true, name: entityDefaultAlias.name}]}; |
There was a problem hiding this comment.
Reading this aliases array, I think if the EG before modification has multiple aliases, this tempBody will always be a different array of aliases, since it onyl contains one item.
You'll have to use the whole currentEditionGroup.aliases and modify the default alias (I guess by filtering by id).
I don't think we have access to the aliases array on currentEditionGroup and so the const EGEntity = await fetchOrCreateMainEntity call should probably be done beforehand.
There was a problem hiding this comment.
About that last detail: I've suggested removing the currentEditionGroup.defaultAlias.name !== entityDefaultAlias.name check and letting getNextAliasSet do the comparison, but now I realize this adds some DB calls if we have to fetch the Edition Group first to get the aliases (and we don't want that).
I think we can get away with fetching all the aliases for the EG in makeEntityLoader: https://github.com/metabrainz/bookbrainz-site/blob/master/src/server/routes/entity/edition.js#L299
'editionGroup.defaultAlias' -> 'editionGroup.aliases' probably does the trick?
If none of this works then we'll want to keep an alias comparison condition, but make it similar to what updateAliasSet ORM function uses: https://github.com/metabrainz/bookbrainz-data-js/blob/master/src/func/alias.ts#L41-L48
There was a problem hiding this comment.
Reading this aliases array, I think if the EG before modification has multiple aliases, this tempBody will always be a different array of aliases, since it onyl contains one item.
Yes, I think that's what's happening here:
The Edition Group diff shows three removed aliases while copying the name from the Edition:
| return { | ||
| aliases, | ||
| annotation: data.annotationSection.content, | ||
| copyToEG: data.nameSection.copyToEG ?? false, |
There was a problem hiding this comment.
Let's rename this property to copyNameToEditionGroup for clarity; I know it's long, but I think it leaves less guesswork.
I'll build on your solution in a separate PR to add a copyAuthorCreditToEditionGroup so the distinction will be important.
| export const UPDATE_WARN_IF_EXISTS = 'UPDATE_WARN_IF_EXISTS'; | ||
| export const UPDATE_SEARCH_RESULTS = 'UPDATE_SEARCH_RESULTS'; | ||
|
|
||
| export const UPDATE_COPY_CHECKBOX = 'UPDATE_COPY_CHECKBOX'; |
There was a problem hiding this comment.
Same remark as my other comment, let's rename this UPDATE_COPY_NAME_CHECKBOX or UPDATE_COPY_NAME_TO_EDITION_GROUP to make way for UPDATE_COPY_AUTHOR_CREDIT_…
| debouncedUpdateNameField, | ||
| debouncedUpdateSortNameField, | ||
| searchName, | ||
| updateCheckbox, |
There was a problem hiding this comment.
And one last name change request :) updateCopyNameCheckbox or updateCopyNameToEditionGroup
Not sure about this, if only thing user wants is to change EG's default name wouldn't it be better to do it directly? |
2 participants
Problem
BB-663: Add option to copy Edition name to Edition Group
Solution
Added checkbox for copying edition name to edition-group
Areas of Impact
nameSection component, createOrEditEnity function