Refresh Entity Import Infrastructure #1096
Conversation
…ults to display imports
…ecentImports results
…splay SPA recentImports
…om import-entities
…e and relationships optional
…oading entitypage
- Resolve trivial merge conflicts, among them moving from .js to .ts(x) - Migrate additional entries of the deleted `scripts/build-client-js.sh` and `watch-client-js.sh` to `webpack.client.js - Migrate search results page changes, disable "Add to Collection" checkboxes for imports - Drop red heading which explains imports at the bottom of search page for now, it has evolved since and would probably become convoluted - Move "Review recent imports" menu entry into the docs dropdown, the menubar has become very crowded since 2018 Tasks which still have to be done: - Account for the renamed entity types "creator" and "publication" - Use `AuthorAttributes` instead of `CreatorAttributes` - Use `EditionGroupAttributes` instead of `PublicationAttributes` - Signature of `EntityImage` has changed, `backupIcon` is now an object - Signature of `EntityRelationships` has changed and expects more props - Check how much extra massaging is necessary for imports with the recent changes to search indexing!
Only breaking changes about which the linter complained, no polishing. Using https://react-bootstrap-v4.netlify.app/migrating/ as a reference.
Replace "bsStyle" attribute with the "variant" attribute. Using https://react-bootstrap-v4.netlify.app/migrating/ as a reference.
Make indentation within the file consistent. (The whole sql folder is still inconsistent.)
I am not 100% sure which columns of the view are actually required, so I closely followed the other entity types and even added `ordering_type_id` because the other types also have their `type_id`.
There was a problem hiding this comment.
Looking good, working great so far !
Pretty clean merge for such an outdated PR, if you ask me!
There are as expected some issues that will need to be resolved, but obviously we want to keep changes to this huge PR minimal, so feel free to apply the minimal change and keep more involved ones for future PRs.
| recentImports={this.state.recentImports} | ||
| /> | ||
| <p> {`Displaying ${limit} of ${totalResults} results`} </p> | ||
| <Pagination |
There was a problem hiding this comment.
The pagination component here needs some more modifications in order to be useful, although it can be kept for a future PR as it is not crucial for the moment and we can navigate using url params.
For reference, https://react-bootstrap-v4.netlify.app/components/pagination/
The parent pagination component takes much fewer props, but we have to declare the pagination buttons underneath manually:
<Pagination>
<Pagination.First />
<Pagination.Prev />
<Pagination.Item>{1}</Pagination.Item>
<Pagination.Ellipsis />
[...]
<Pagination.Item>{paginationProps.totalPages}</Pagination.Item>
<Pagination.Next />
<Pagination.Last />
</Pagination>
There was a problem hiding this comment.
Indeed, I even had something about the pagination in my notes about the 2018 GSoC project, so that would be it.
I had a quick look and decided not to bother with this for now, I would rather see the approve/discard/edit actions working first. So let's do this later in a separate PR.
src/client/containers/layout.js
Outdated
| @@ -126,6 +126,10 @@ class Layout extends React.Component { | |||
| <FontAwesomeIcon fixedWidth icon={faBarcode}/> | |||
| {' Identifier Types '} | |||
| </NavDropdown.Item> | |||
| <NavDropdown.Item href="/imports/recent"> | |||
There was a problem hiding this comment.
I think maybe this would work better if you made the "Revisions" menu item a dropdown with two options "Latest revisions" and as you have it "Review recent imports".
What do you think?
There was a problem hiding this comment.
Sounds like a good idea, the docs menu was just the simplest option to use while I was still resolving the merge conflicts.
I've implemented your idea, this is what I have so far:
Some additional thoughts:
- I think the new dropdown should be moved to the left of the Docs menu to avoid having regular menu items between the dropdowns.
- All other multi-word menu items are currently using title case, so we could use title case for the new ones as well. On the other hand, MusicBrainz recently standardized all menu items to use sentence case... If we decide to follow that, the necessary changes should be done in a separate PR.
- "(Latest) Revisions" links to a page which is titled "Recent Activity", this terminology is also used on the landing page. It might make sense to try being consistent with the terms recent/latest and activity/revisions. I think a menu item "Revisions > Recent activity" might be a good solution (unless we implement my next idea).
- The "Statistics" menu item does not feel important enough to me to have its own menu item, maybe we could integrate that into the new dropdown as well? The top editor stats would fit under the "Revisions" menu heading, but the total entity stats would not. Maybe we could call the dropdown "Activity" to account for this, or does that sound too generic?
There was a problem hiding this comment.
I like your suggestions, Statistics in the new dropdown item makes sense, and I'm all for standardizing naming and capitalization. I'm not sure we need to move the new dropdown, but we can discuss the pros and cons.
However, I think this should be the first item to be put in the "post-GSOC list" so as not to derail you from more critical aspects. Let's not bother about it for now.
| * @returns {function} - Returns a function which give pagination props given | ||
| * currentPage and totalResults | ||
| */ | ||
| export default function getPaginationPropsGenerator( |
There was a problem hiding this comment.
As I described above, the props for the react-bootstrap Pagination component have changed significantly, and you'll have to see which of these props are required and which are not needed anymore, if any.
Probably best in a separate PR though, considering how much is already going on in this one...
| @@ -526,6 +534,49 @@ export async function generateIndex(orm) { | |||
| })); | |||
| await _processEntityListForBulk(processedCollections); | |||
|
|
|||
| const {AuthorImport, EditionImport, EditionGroupImport, PublisherImport, WorkImport} = orm; | |||
There was a problem hiding this comment.
As I mentioned in video chat, I have been working on the search infrastructure (#1093) and some of the changes here will need to be reevaluated.
However as other comments we should keep this for a separate PR, not least because the PR is not yet merged...
So we'll leave this as-is for the time being.
There was a problem hiding this comment.
Agreed, ignoring the search indexing bits in this PR is what I had in mind as well.
There was a problem hiding this comment.
Yep, totally makes sense. I also just merged my search indexing improvements PR, so we have a better base to work from now.
| '/:importId/edit', | ||
| middleware.loadIdentifierTypes, middleware.loadGenders, | ||
| middleware.loadLanguages, middleware.loadAuthorTypes, | ||
| importRoutes.editImportEntity |
There was a problem hiding this comment.
For some entity types, we need to load more information and add some of it to the page props.
In this case for example, loadGenders loads the available genders from the DB into res.locals.genders, and needs to be extracted from there and passed into page props like we do for the regular entities edit pages:
bookbrainz-site/src/server/routes/entity/author.ts
Lines 340 to 344 in 7c23ed6
Currently the editImportEntity function does not allow that, but maybe it can be modified to allow attaching certain extras from res.locals to props, something like:
if(res.locals.genders){
optionalProps.genderOptions = res.locals.genders
}
There was a problem hiding this comment.
I will look into this next, so far I haven't even tried the approve/edit action buttons, only the discard button which leads to a confirmation page.
Problem
The original PR #201 from 2018 is horribly out of date and can't be properly tested with the current infrastructure.
Solution
Since there were not too many possibly conflicting changes, I decided to try merging of the current master branch into the old feature branch from 2018.
This is the first new commit d055486 that includes a couple more advanced merge conflicts resolutions, they are documented in the commit message.
The following commits up to and including 0cf2a66 were necessary to get babel to transpile everything and let the server start up again.
Once the server was running, I solved a few display issues with the new imported entity display pages (for example
/imports/author/42).Finally I had to add series import tables to my database in order to prevent the recent imports page from crashing the server. Originally I planned to do these schema changes as a separate step, but the code forced me to do that now since it iterates over all entity types, looking for their import tables, including series which didn't exist in 2018.
Areas of Impact
Too many probably, we should create a separate branch based on
mastersince we don't want to directly merge this as it is still not ready for production use.Next Steps
down.sqlfile (onceup.sqlhas been reviewed)