CollectionView page #468

prabalsingh24 · 2020-06-29T04:45:35Z

Problem

CollectionView Page

prabalsingh24 · 2020-06-29T04:48:30Z

@MonkeyDo What do you think of thiss? I've created only EditionCollection page right now. If you think this is alright, I will create other EntityTables similar to this as well

coveralls · 2020-06-29T04:50:02Z

Coverage decreased (-0.8%) to 58.182% when pulling 62e9f7c on prabalsingh24:CollectionPage into 63e6a53 on bookbrainz:UserCollection.

prabalsingh24 · 2020-06-29T06:05:23Z

I made collectionView page for Work also. Reused the existing Work-Table

MonkeyDo

Great job reusing the existing entity tables, that's 100% the way to go.
And now that you're adding checkboxes, it will allow us in the future to add batch actions easily !

MonkeyDo · 2020-06-30T11:21:06Z

src/client/components/pages/collection.js

+				.then((res) => {
+					window.location.href = `/collection/${this.props.collection.id}`;
+				}, (error) => {
+					this.setState({error: 'Internal Error'});


Same as in the other PR, it would be great if this was a more user-friendly error message

MonkeyDo · 2020-06-30T11:22:24Z

src/client/components/pages/collection.js

+			<Alert bsStyle="danger">{this.state.error}</Alert> : null;
+		const EntityTable = getEntityTable(this.props.collection.entityType);
+		const propsForTable = {
+			[this.entityKey]: this.props.entities,


That's a smart way to solve the issue of each entity table requiring a specific named key. Nice!

MonkeyDo · 2020-06-30T11:24:35Z

src/client/components/pages/collection.js

+						<h1><strong>{this.props.collection.name}</strong></h1>
+						<h4>{this.props.collection.description}</h4>


I think you can get the formatting closer to the existing entity pages, and also use a big entity icon. (I was originally thinking the icon of the entity type)

MonkeyDo · 2020-06-30T11:27:06Z

src/client/components/pages/collection.js

+						<hr/>
+						{
+							this.props.isOwner ?
+								<ButtonGroup className="pull-right margin-bottom-1">


I wonder if it would make more sense to show these buttons below the entity table, along with the batch remove button.

If that doesn't work out well, I'd suggest having the buttons left aligned rather than right-aligned

MonkeyDo · 2020-06-30T11:27:58Z

src/client/components/pages/collection.js

+							className="pull-left margin-top-1"
+							onClick={this.handleRemoveEntities}
+						>
+							<FontAwesomeIcon icon="times"/>


Suggested change

<FontAwesomeIcon icon="times"/>

<FontAwesomeIcon icon="times-circle"/>

I think here the times-circle icon will be ideal

src/client/stylesheets/style.less

MonkeyDo · 2020-06-30T11:42:09Z

src/server/routes/collection.js

+	const {orm} = req.app.locals;
+	const relations = getEntityRelations(collection.entityType);
+
+	const entitiesPromise = collection.items.map(item => orm.func.entity.getEntity(orm, collection.entityType, item.bbid, relations));


In the future we might want to think about optimizing this to fetch all the entities in a single batch, but that's for another day.

I'll add this to that doc

MonkeyDo · 2020-06-30T11:43:36Z

src/server/routes/collection.js

@@ -80,9 +140,23 @@ router.get('/:collectionId/edit', auth.isAuthenticated, auth.isCollectionOwner,

 router.post('/:collectionId/edit/handler', auth.isAuthenticatedForHandler, auth.isCollectionOwner, collectionCreateOrEditHandler);

-router.get('/:collectionId', (req, res) => {
+router.post('/:collectionId/remove', auth.isAuthenticated, auth.isCollectionOwnerOrCollaborator, async (req, res) => {
+	const {bbids} = req.body;


We probably want to send a detailed error if there are no bbids (missing in body or empty array)

MonkeyDo · 2020-06-30T11:45:00Z

src/server/routes/collection.js

+		const promiseArray = bbids.map((bbid) =>
+			new UserCollectionItem()
+				.where('bbid', bbid)
+				.where('collection_id', collection.id)
+				.destroy()
+		);
+		await Promise.all(promiseArray);


I wonder if you could do this in one batch instead of multiple calls?
Perhaps using Bookshelf's collections ?

src/server/routes/collection.js

prabalsingh24 · 2020-07-03T15:09:50Z

@MonkeyDo Have a look at this. Also should there be an identifier in Publisher, Work, Author Table? if yes then which?
There is ISBN identifier row in EditionTable..

MonkeyDo · 2020-07-03T17:23:13Z

Also should there be an identifier in Publisher, Work, Author Table? if yes then which?
There is ISBN identifier row in EditionTable..

ISBNs for Editions is a distinguishing feature, I don't think we want to show any particular identifier for the other entities.
However, for Publishers, let's show Date Founded and Date Disolved
Not much more to show for Edition Groups.
For Authors we can also show additional info like date and place of birth, date and place of death (empty if ended=false).

We'll also want pagination for the collection items, looking at reosarevok's collection of 7189 artists…
If you want that can be done in a separate PR, in case it requires to refactor a bunch of other components, to limit the scope.

MonkeyDo · 2020-07-03T17:27:29Z

Also I think disabling the "Remove selected work" button rather than hiding it would be better, I find changes in layout are not appreciated by users if they can be avoided.

Apart from that it's looking good! It feels like a full workflow for the first time, it's really nice to see it all come together :)

prabalsingh24 · 2020-07-04T10:01:38Z

src/server/helpers/collections.js

prabalsingh24 · 2020-07-04T10:09:15Z

@MonkeyDo Have a look at this

prabalsingh24 · 2020-07-04T10:10:38Z

I also noticed couple of bugs. One in UserCollectionForm and other in Editor Collection Page. I will solve them in different PR.

MonkeyDo

Pagination works great, instant improvement !
Placement of the collection edit buttons looks good to me.

With that in place, do you think we want to display the total number of items somewhere in the collection attributes, or do we rely only on the pagination count? I suppose the same column in the collections page would make sense. I'll add that to the google doc.

Also how do you feel about a "clear selection" button?

A line I couldn't comment on directly but that you can change in this PR is line 273 of src/server/routes/collection.js:
res.status(500).send(); -> next(err);

src/client/components/pages/entities/editionGroup-table.js

MonkeyDo · 2020-07-06T16:52:02Z

src/server/routes/collection.js

+		// get next enabled for pagination
+		const {newResultsArray, nextEnabled} = utils.getNextEnabledAndResultsArray(bbids, size);
+		// load entities from bbids
+		const entitiesPromise = newResultsArray.map(bbid => orm.func.entity.getEntity(orm, collection.entityType, bbid, relations));


We need to optimize this at some point to fetch all the entities in one go.

src/server/routes/collection.js

MonkeyDo · 2020-07-06T17:46:17Z

Thinking a bout it again, perhaps there should be a count or entitiesCount method on the ORM model to count the UserCollectionItems?
https://bookshelfjs.org/api.html#Model-instance-count

MonkeyDo

OK, this is in a good state now, looking nice !
I've got one final item I just noticed. Apart from that I think it's ready to merge !

src/client/components/pages/entities/publisher-table.js

prabalsingh24 · 2020-07-08T13:04:03Z

is this alright? or should this be done in css class in style.less ?

MonkeyDo

Awesome, good to go !

prabalsingh24 requested a review from MonkeyDo June 29, 2020 04:45

MonkeyDo requested changes Jun 30, 2020

View reviewed changes

prabalsingh24 added 6 commits July 3, 2020 09:54

feat(collection-page): add Edition-collectionPage

2b412ea

chore: create POST /collection/:id/remove route

687a3bd

chore: refactor: refactor work-table

366d3ec

chore: add WorkTable in collectionView

b239001

chore: load work from workCollection

fc5806b

chore: show proper plural word in button- Remove 'Editions', 'Works'

b55c56d

prabalsingh24 force-pushed the CollectionPage branch from 089c4dd to b55c56d Compare July 3, 2020 04:29

prabalsingh24 added 8 commits July 3, 2020 10:00

fix conflicts

fd7ec01

chore: improve collectionPage header

97c77e6

chore: modify collectionView header

608c55d

chore: add author-table

92a1550

chore: add publisher-table

30e3e53

chore: add edition-group table

feadacd

chore: add DeleteCollectionModal in CollectionView Page

5351680

chore: update lastModifed after adding/removing entities from collection

126f0cc

prabalsingh24 requested a review from MonkeyDo July 3, 2020 15:06

chore: add languages in workTable

add12c7

prabalsingh24 added 3 commits July 4, 2020 10:38

chore: modify button visibility

926f3bb

chore: add pagination in collectionTables

75e67db

chore: add more columns in publisher, author table

2b54e6f

prabalsingh24 commented Jul 4, 2020

View reviewed changes

src/server/helpers/collections.js Outdated Show resolved Hide resolved

chore: minor changes

5bbfb86

MonkeyDo reviewed Jul 6, 2020

View reviewed changes

prabalsingh24 added 2 commits July 8, 2020 10:45

chore: minor changes; remove commented code

07469c0

chore: use one query for updating lastModified

634e71f

prabalsingh24 requested a review from MonkeyDo July 8, 2020 05:25

MonkeyDo requested changes Jul 8, 2020

View reviewed changes

src/client/components/pages/entities/publisher-table.js Outdated Show resolved Hide resolved

chore: minor change in table <th>

f39b8a8

prabalsingh24 requested a review from MonkeyDo July 8, 2020 13:05

chore: minor fix

62e9f7c

MonkeyDo approved these changes Jul 8, 2020

View reviewed changes

MonkeyDo merged commit 06ece0c into metabrainz:UserCollection Jul 8, 2020

prabalsingh24 deleted the CollectionPage branch July 11, 2020 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CollectionView page #468

CollectionView page #468

prabalsingh24 commented Jun 29, 2020

prabalsingh24 commented Jun 29, 2020 •

edited

coveralls commented Jun 29, 2020 •

edited

prabalsingh24 commented Jun 29, 2020

MonkeyDo left a comment

MonkeyDo Jun 30, 2020

MonkeyDo Jun 30, 2020

MonkeyDo Jun 30, 2020

MonkeyDo Jun 30, 2020

MonkeyDo Jun 30, 2020

MonkeyDo Jun 30, 2020

prabalsingh24 Jul 3, 2020

MonkeyDo Jun 30, 2020

MonkeyDo Jun 30, 2020

prabalsingh24 commented Jul 3, 2020 •

edited

MonkeyDo commented Jul 3, 2020 •

edited

MonkeyDo commented Jul 3, 2020 •

edited

prabalsingh24 commented Jul 4, 2020

prabalsingh24 commented Jul 4, 2020

prabalsingh24 commented Jul 4, 2020

MonkeyDo left a comment

MonkeyDo Jul 6, 2020

MonkeyDo commented Jul 6, 2020 •

edited

MonkeyDo left a comment

prabalsingh24 commented Jul 8, 2020

MonkeyDo left a comment

		<h1><strong>{this.props.collection.name}</strong></h1>
		<h4>{this.props.collection.description}</h4>

	<FontAwesomeIcon icon="times"/>
	<FontAwesomeIcon icon="times-circle"/>

CollectionView page #468

CollectionView page #468

Conversation

prabalsingh24 commented Jun 29, 2020

Problem

prabalsingh24 commented Jun 29, 2020 • edited

coveralls commented Jun 29, 2020 • edited

prabalsingh24 commented Jun 29, 2020

MonkeyDo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prabalsingh24 commented Jul 3, 2020 • edited

MonkeyDo commented Jul 3, 2020 • edited

MonkeyDo commented Jul 3, 2020 • edited

prabalsingh24 commented Jul 4, 2020

prabalsingh24 commented Jul 4, 2020

prabalsingh24 commented Jul 4, 2020

MonkeyDo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonkeyDo commented Jul 6, 2020 • edited

MonkeyDo left a comment

Choose a reason for hiding this comment

prabalsingh24 commented Jul 8, 2020

MonkeyDo left a comment

Choose a reason for hiding this comment

prabalsingh24 commented Jun 29, 2020 •

edited

coveralls commented Jun 29, 2020 •

edited

prabalsingh24 commented Jul 3, 2020 •

edited

MonkeyDo commented Jul 3, 2020 •

edited

MonkeyDo commented Jul 3, 2020 •

edited

MonkeyDo commented Jul 6, 2020 •

edited