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.

Sign up for GitHub

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

Jump to bottom

Feat(EG): Copy Edition name to Edition Group #842

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feat(EG): Copy Edition name to Edition Group #842

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66b1cfd
feat(EG): copy edition name to edition group
tr1ten May 3, 2022
595f1ac
minor code improvements
tr1ten Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/client/entity-editor/name-section/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const UPDATE_NAME_FIELD = 'UPDATE_NAME_FIELD';
export const UPDATE_SORT_NAME_FIELD = 'UPDATE_SORT_NAME_FIELD';
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';
Copy link
Contributor

Choose a reason for hiding this comment

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

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_…

export type Action = {
type: string,
payload?: unknown,
Expand Down Expand Up @@ -203,3 +203,17 @@ export function searchName(
});
};
}

/**
* Produces an action indicating that the checkbox
* should be updated with the provided value.
*
* @param {boolean} isCheck - The new value to be used for the checkbox.
* @returns {Action} The resulting UPDATE_COPY_CHECKBOX action.
*/
export function updateCheckbox(isCheck:boolean): Action {
return {
payload: isCheck,
type: UPDATE_COPY_CHECKBOX
};
}
19 changes: 18 additions & 1 deletion src/client/entity-editor/name-section/name-section.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/

import {Alert, Col, ListGroup, Row} from 'react-bootstrap';
import {Alert, Col, Form, ListGroup, Row} from 'react-bootstrap';
import {
checkIfNameExists,
debouncedUpdateDisambiguationField,
debouncedUpdateNameField,
debouncedUpdateSortNameField,
searchName,
updateCheckbox,
Copy link
Contributor

Choose a reason for hiding this comment

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

And one last name change request :) updateCopyNameCheckbox or updateCopyNameToEditionGroup

updateLanguageField
} from './actions';
import {isAliasEmpty, isRequiredDisambiguationEmpty} from '../helpers';
Expand Down Expand Up @@ -63,6 +64,7 @@ import {getEntityDisambiguation} from '../../helpers/entity';
* @param {string} props.nameValue - The name currently set for this entity.
* @param {string} props.sortNameValue - The sort name currently set for this
* entity.
* @param {Function} props.onCheckboxToggle - A function to be called when toggle copy name to EG checkbox
* @param {Function} props.onLanguageChange - A function to be called when a
* different language type is selected.
* @param {Function} props.onNameChange - A function to be called when the name
Expand Down Expand Up @@ -129,13 +131,15 @@ class NameSection extends React.Component {

render() {
const {
action,
disambiguationDefaultValue,
entityType,
exactMatches,
languageOptions,
languageValue,
nameValue,
sortNameValue,
onCheckboxToggle,
onLanguageChange,
onSortNameChange,
onDisambiguationChange,
Expand Down Expand Up @@ -172,6 +176,17 @@ class NameSection extends React.Component {
onChange={this.handleNameChange}
/>
</Col>
{action === 'edit' && entityType === 'edition' &&
<Col className="md-2" lg={{offset: 3, span: 6}}>
<Form.Check
className="margin-bottom-d8"
id="name"
label="Copy the edition name to edition-group"
Copy link
Contributor

@MonkeyDo MonkeyDo Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?
image

Or in the Edition Group section below?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe below the disambiguation?

This does sound like a reasonable place for checkbox

type="checkbox"
onChange={onCheckboxToggle}
/>
</Col>
}
<Col lg={{offset: 3, span: 6}}>
{isRequiredDisambiguationEmpty(
warnIfExists,
Expand Down Expand Up @@ -271,6 +286,7 @@ NameSection.propTypes = {
languageOptions: PropTypes.array.isRequired,
languageValue: PropTypes.number,
nameValue: PropTypes.string.isRequired,
onCheckboxToggle: PropTypes.func.isRequired,
onDisambiguationChange: PropTypes.func.isRequired,
onLanguageChange: PropTypes.func.isRequired,
onNameChange: PropTypes.func.isRequired,
Expand Down Expand Up @@ -314,6 +330,7 @@ function mapStateToProps(rootState) {
function mapDispatchToProps(dispatch, {entity, entityType}) {
const entityBBID = entity && entity.bbid;
return {
onCheckboxToggle: (event) => dispatch(updateCheckbox(event.target.checked)),
onDisambiguationChange: (event) =>
dispatch(debouncedUpdateDisambiguationField(event.target.value)),
onLanguageChange: (value) =>
Expand Down
7 changes: 5 additions & 2 deletions src/client/entity-editor/name-section/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@
*/

import {
UPDATE_DISAMBIGUATION_FIELD, UPDATE_LANGUAGE_FIELD, UPDATE_NAME_FIELD,
UPDATE_SEARCH_RESULTS, UPDATE_SORT_NAME_FIELD, UPDATE_WARN_IF_EXISTS
UPDATE_COPY_CHECKBOX, UPDATE_DISAMBIGUATION_FIELD, UPDATE_LANGUAGE_FIELD,
UPDATE_NAME_FIELD, UPDATE_SEARCH_RESULTS, UPDATE_SORT_NAME_FIELD, UPDATE_WARN_IF_EXISTS
} from './actions';
import Immutable from 'immutable';


function reducer(
state = Immutable.Map({
copyToEG: false,
disambiguation: '',
exactMatches: [],
language: null,
Expand All @@ -48,6 +49,8 @@ function reducer(
return state.set('searchResults', payload);
case UPDATE_WARN_IF_EXISTS:
return state.set('exactMatches', payload);
case UPDATE_COPY_CHECKBOX:
return state.set('copyToEG', payload);
// no default
}
return state;
Expand Down
1 change: 1 addition & 0 deletions src/server/routes/entity/edition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ function transformNewForm(data) {
return {
aliases,
annotation: data.annotationSection.content,
copyToEG: data.nameSection.copyToEG ?? false,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

depth: data.editionSection.depth &&
parseInt(data.editionSection.depth, 10),
disambiguation: data.nameSection.disambiguation,
Expand Down
16 changes: 16 additions & 0 deletions src/server/routes/entity/entity.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,7 @@ export function handleCreateOrEditEntity(
aliasSet: {id: number} | null | undefined,
annotation: {id: number} | null | undefined,
bbid: string,
editionGroup:any | undefined,
disambiguation: {id: number} | null | undefined,
identifierSet: {id: number} | null | undefined,
type: EntityTypeString
Expand Down Expand Up @@ -1085,6 +1086,21 @@ export function handleCreateOrEditEntity(
let allEntities = [...otherEntities, mainEntity]
.filter(entity => entity.get('dataId') !== null);

// copy name to the edition group
if (!isNew && entityType === 'Edition' && body.copyToEG) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const tempBody = {aliases: [{...currentEditionGroup.defaultAlias, default: true, name: entityDefaultAlias.name}]};
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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:
image

const aliasSet = await getNextAliasSet(orm, transacting, currentEditionGroup, tempBody);
const EGEntity = await fetchOrCreateMainEntity(
orm, transacting, false, currentEditionGroup.bbid, 'EditionGroup'
);
EGEntity.set('aliasSetId', aliasSet.id);
EGEntity.shouldInsert = false;
allEntities.push(EGEntity);
}
}
if (isMergeOperation) {
allEntities = await processMergeOperation(orm, transacting, req.session,
mainEntity, allEntities, relationshipSets);
Expand Down