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(contribution flow): add edition from work page and add work from edition page. Fix #BB-326 and #BB-328 #248

Merged
merged 3 commits into from Feb 4, 2019

Conversation

@akhilesh26
Copy link
Contributor

commented Jan 31, 2019

  1. Add a button called 'Add Edition' in work page to create edition from work page with a preloaded relationship.
  2. Add a button called 'Add Work' in edition page to create work from edition page with a preloaded relationship.

Problem

Implement feature explained in #BB-326 and #BB-328

Solution

  1. Add a button called 'Add Edition' in work page to create edition from work page with a preloaded relationship.
  2. Add a button called 'Add Work' in edition page to create work from edition page with a preloaded relationship.

Areas of Impact

Client-side
src/client/components/pages/entities/edition.js
src/client/components/pages/entities/work.js

Server-side
src/server/helpers/entityRouteUtils.js
src/server/routes/entity/edition.js
src/server/routes/entity/work.js

feat(contribution flow): add edition from work page and add work from…
… edition page.

1. Add a button called 'Add Edition' in work page to create edition from work page with a preloaded relationship.
2. Add a button called 'Add Work' in edition page to create work from edition page with a  preloaded relationship.
@coveralls

This comment has been minimized.

Copy link

commented Jan 31, 2019

Coverage Status

Coverage decreased (-0.1%) to 40.827% when pulling dee8476 on akhilesh26:add_edition_from_work into c4a48cb on bookbrainz:master.

@MonkeyDo
Copy link
Contributor

left a comment

That's a good start, clean like the previous PRs.
Thanks for your work :)

There's one functionality missing that you might not have seen in the ticket, that adds a bit of complications:
When creating an Edition from a Work page, preloading by default the defaultAlias set (name, sortname, language) from the Work into the new Edition, to avoid having to retype the same name (more chances to make a mistake!)

@@ -151,6 +160,14 @@ router.get(
addInitialRelationship(props, relationshipTypeId, initialRelationshipIndex, props.publication);
}

if (props.work) {
initialState.editionSection.work = props.work;

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Jan 31, 2019

Contributor

The .work property does not exist in editionSection, contrarily to how .publication and .publisher are set.
Have a look at

const editionSection = {
depth: edition.depth,
format: edition.editionFormat && edition.editionFormat.id,
height: edition.height,
languages: edition.languageSet ? edition.languageSet.languages.map(
({id, name}) => ({label: name, value: id})
) : [],
pages: edition.pages,
physicalVisible,
publication,
publicationVisible,
publisher,
releaseDate,
status: edition.editionStatus && edition.editionStatus.id,
weight: edition.weight,
width: edition.width
};

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Feb 2, 2019

Contributor

This initialState.editionSection.work = props.work; does not serve any purpose, as there is nowhere a .work property is used in the editionSection.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@MonkeyDo Thanks for the review. Changes did.

@@ -151,6 +160,14 @@ router.get(
addInitialRelationship(props, relationshipTypeId, initialRelationshipIndex, props.publication);
}

if (props.work) {
initialState.editionSection.work = props.work;

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Feb 2, 2019

Contributor

This initialState.editionSection.work = props.work; does not serve any purpose, as there is nowhere a .work property is used in the editionSection.

src/server/routes/entity/work.js Outdated Show resolved Hide resolved
@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

Very nearly there, thanks for your hard work!

The creation flow is a lot easier with these recent PRs.
It'll be a lot easier to explain to new users too, and the first step towards a unified creation form.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

Thankyou @MonkeyDo to support us very well and regular PR review. I created the changes for your comment. I am also happy to work with you. Your comments are always welcome for any kind of improvement.

@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Very nice, thank you !

I've modified the ticket BB-329 with two subtasks to homogenize the way we display Works and Editions.
I think it would be the perfect continuation to this PR, and it makes a lot of sense as a whole.

@MonkeyDo MonkeyDo merged commit 4986f05 into bookbrainz:master Feb 4, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 40.827%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Hi again @akhilesh26,

When trying this out, I encountered an issue that I hadn't foreseen for an Edition created from a Work: the deduplication check we run when the user types in the name input is not run.
If there is already an existing Edition with the same name as the Work, I am expecting the user to be shown a warning about duplicates.

I created PR #249 to fix the issue, and I was wondering if you could help me by trying it out to confirm everything is working fine.

Here is the url I've been using to test this:
http://localhost:9099/edition/create?work=b220e2f2-b467-45f8-ab8d-9142d3eb477e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.