-
-
Notifications
You must be signed in to change notification settings - Fork 285
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
Add datasets importing functionality #201
base: master
Are you sure you want to change the base?
Conversation
…ults to display imports
…ecentImports results
…splay SPA recentImports
…om import-entities
…e and relationships optional
…rops for import-entity
…form form to entity
…nsform import to form
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, quite a few small comments and changes though.
@@ -68,6 +69,11 @@ class SearchPage extends React.Component { | |||
<div id="searchPage"> | |||
<SearchField onSearch={this.handleSearch}/> | |||
<SearchResults results={this.state.results}/> | |||
<h4 style={{color: 'maroon'}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the style to the stylesheet (src/client/stylesheets/style.less) and then use a class here rather than an inline style. Or consider using one of bootstrap's text helper classes: https://getbootstrap.com/docs/3.3/css/#helper-classes-colors
} | ||
result.id = result.bbid || result.importId; | ||
const tag = result.importId ? ( | ||
<span style={{color: 'red', fontWeight: 'bold'}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use a react-bootstrap label here, perhaps? https://react-bootstrap.github.io/components/label/
<a href={link}> | ||
<a | ||
href={link} | ||
style={result.importId ? {color: 'red'} : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this to src/client/stylesheets/style.less, then use the classnames package (https://github.com/JedWatson/classnames) to programmatically set the class name to something here to indicate an import (maybe "import-link").
src/server/routes/imports/recent.js
Outdated
|
||
return orm.bookshelf.transaction(async (transacting) => { | ||
// First fetch total imports | ||
const totalResults = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be quite a slow query - https://wiki.postgresql.org/wiki/Slow_Counting . Avoid fetching the total number of results if possible - I don't think it'll be a problem if you use an offset that's more than the number of rows in the table - you'll just get 0 results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's necessary to know total number of valid results to diplayed to determine begin and end page correctly. If we don't know the range, we cannot use pagination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need the range to use pagination? You should be able to define pages like so:
Page 1: 0 - X
Page 2: X+1 - 2X
Page 3: 2X+1 - 3X
And so on. If your user gives page 10,000 and there are only 3,000 pages, then there should just be no results on the page.
If you need to know whether the next two pages exist, then the server can query 3X
results from the current offset, and then if more then 2X
results are returned, you have at least 2 more valid pages after the current one.
For now, I'm happy to accept this, but we'll need to do further testing to see whether it's too slow to use in practise.
} | ||
|
||
componentDidMount() { | ||
this.handleClick(this.state.currentPage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause the state to be updated, which is bad inside componentDidMount - https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-did-mount-set-state.md
Why not just send the initial set of results from the server, rather than requesting them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's an anti-pattern to use setState in componentDidMount. Infact, React official document calls for using ajax calls inside the componentDidMount and then updating the state. See here => https://reactjs.org/docs/faq-ajax.html
Making ajax calls for data to be displayed makes the displaying the content consistent and keeps logic for getting page layout and content separate.
const {headers} = req; | ||
const token = headers['x-auth-token']; | ||
|
||
if (token !== req.session.token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The token as implemented doesn't seem to be useful. I think we should remove it until we work out a better system and identify the exact problem that needs to be solved.
const {orm} = req.app.locals; | ||
const editorId = req.session.passport.user.id; | ||
const {importEntity} = res.locals; | ||
orm.bookshelf.transaction(async transacting => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use parentheses around the parameter to an arrow function
const {orm} = res.app.locals; | ||
const editorId = req.session.passport.user.id; | ||
const {importEntity} = res.locals; | ||
const entity = await orm.bookshelf.transaction(transacting => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with arrow function parentheses here - eslint should have caught this.
const entityData = transformForm[type](formData); | ||
|
||
const entity = await orm.bookshelf.transaction(async (transacting) => { | ||
await orm.func.imports.deleteImport( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to swap the order of these, and pass the new entity BBID to the deleteImport
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to pass the new entity BBID to the deleteImport function? Also how does it matter we keep deletion before creation or creation before deletion, as it all happens in the same transaction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deleteImport function needs to know the new entity BBID to update the link_import table doesn't it?
https://github.com/bookbrainz/bookbrainz-data-js/blob/master/src/func/imports/delete-import.js
import ImportEditionRouter from './edition'; | ||
import ImportPublicationRouter from './publication'; | ||
import ImportPublisherRouter from './publisher'; | ||
// import ImportCreatorRouter from './creator'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these meant to be commented out?
…oading entitypage
Ready for second review, I have some doubts which I have mentioned as comments after reviews. As soon as they're sorted, we're good to go :) |
Adds imports