Conversation
Co-authored-by: Brent Ogunmakin <blackychan4234@gmail.com>
| const getAllWishlists = async () => { | ||
| return WishList.fetchAll({ withRelated: ['items'], require: true }); | ||
| }; | ||
|
|
||
| const getWishlistById = async (id) => { | ||
| return WishList.where('id', '=', id).fetch({ | ||
| withRelated: ['items'], | ||
| require: true, | ||
| }); | ||
| }; | ||
|
|
||
| const createWishlist = async ({ name, author, items }) => { | ||
| return new WishList({ name, author }) | ||
| .save(null, { | ||
| require: true, | ||
| method: 'insert', | ||
| }) | ||
| .then(({ id }) => { | ||
| return Promise.all( | ||
| items.map((item) => { | ||
| return new WishListItem({ name: item, wishlist_id: id }).save(null, { | ||
| require: true, | ||
| method: 'insert', | ||
| }); | ||
| }) | ||
| ); | ||
| }); | ||
| }; | ||
|
|
||
| const deleteWishlist = async (id) => { | ||
| return new WishList({ id }).destroy({ require: true }); | ||
| }; | ||
|
|
||
| const updateWishlist = async (id, { name, author }) => { | ||
| return new WishList({ id, name, author }).save(null, { | ||
| require: true, | ||
| method: 'update', | ||
| }); | ||
| }; | ||
|
|
||
| const updateItem = async (id, { name }) => { | ||
| return new WishListItem({ id }).save({ name }, { | ||
| require: true, | ||
| method: 'update', | ||
| patch: true, | ||
| }); | ||
| }; | ||
|
|
||
| const deleteItem = async (id) => { | ||
| return new WishListItem({ id }).destroy({ require: true }); | ||
| }; |
There was a problem hiding this comment.
It might be helpful to move these functionalities to a separate file and import them into the server for use in the routes (i.e. "separate the concerns" of Database Manipulation from Server Route Functionality into different files).
The actual content of the functions could remain essentially the same, we'd just be able to keep their definition out of the app.js file of the server - much like you've done with the models, for instance.
| res.status(200).send(wishlists); | ||
| }) | ||
| .catch((error) => { | ||
| res.status(500).send(); |
There was a problem hiding this comment.
In situations where we're sending a status code and nothing else, it may be preferable to use sendStatus, which will send the string message equivalent of the code as the content (i.e. res.sendStatus(500) is the same as res.status(500).send('Internal Server Error') ). This allows the client to make easy use of the string for generating an error popup to show the user, etc.
In a similar vein - if we chose NOT to sendStatus, we might consider sending something else back to the client in the case of an error - the error itself, such as:
.catch((error) => { res.status(500).send(error); });
or
.catch((error) => { res.status(500).send({error}); });
to send it in an object. This is not a NECESSARY change, of course, but can provide some extra information to the client (which, in this case, since we're writing it as well, we can make use of if we choose to create more explicit error handling).
There was a problem hiding this comment.
I do like the .sendStatus() solution. I generally try not to send errors verbatim in any response for security reasons (even though security practices are out of the scope of this project)
| expect(response.created).toBe(false); | ||
| }); | ||
|
|
||
| it('should respond with a 500 and a message if items are missing', async () => { |
There was a problem hiding this comment.
Example in reference to my comment about sendStatus in server/app.js - here is one of the places we could make use of a message in the response either via sendStatus or by sending the error back. Currently, our test is stated to be verifiying that the server 'should respond with a 500 and a message if items are missing', but we don't currently check for a message.
| describe('renders App', () => { | ||
| const component = shallow(<App />); | ||
|
|
||
| it('should have a page header', () => { |
There was a problem hiding this comment.
There are probably a few other things we could test about the App itself - we could check for our App's specific divs' presence by id or class, or for specific tags we would expect to be present in the case of the App loading normally, etc.
| component = shallow(<PageHeader />); | ||
| }); | ||
|
|
||
| it('Does header have an image', () => { |
There was a problem hiding this comment.
Syntax note - it's generally considered best practice to phrase "it" blocks with a phrase starting with "should", i.e. "should have a header image" and "should contain a search bar" rather than question-style syntax.
| @@ -0,0 +1,4 @@ | |||
| module.exports = (err, req, res, next) => { | |||
There was a problem hiding this comment.
This is another location (other than in the server/app.js) where we might try handling errors with a little more distinction between error types - even though this errorhandler from the docs is useful, we could probably provide the client side with more info. Since we designed the Client and Server in this exercise, it's up to us what the client expects back from the server, and what it does with that info - we could use this to make more explicit errors pop up on the client end, for instance, or use FS to compile a log of information about the error on the server-side to give us a running log of the reqs that cause errors and the sorts of errors they cause. This would help us track down possible problems by examining that data. Obviously, this could prove very useful if we had many users connecting to our app and using it every day and we started to get reports from our Customer Support team that something wasn't working for the customer vis-a-vis a certain action. You could even store that data in a DB, rather than an error log file...
| exports.WishList = WishList; | ||
| exports.WishListItem = WishListItem; | ||
| exports.Bookshelf = bookshelf; |
There was a problem hiding this comment.
Since we're using Node on the server side, if we capitalized Bookshelf when assigning the variable name on line 2 here, we could use some destructuring to export everything with one super-clean line:
module.exports = {WishList, WishListItem,Bookshelf};
Also, I really appreciate the separation of concerns here, keeping the code for DB interaction apart from the server/app.js makes for nice, clean code, as well as being modular by its nature (if we needed to replace our DB system with a different one, we could replace this file without modifying others, as long as the new one exports variables that have the same names and methods, etc.
There was a problem hiding this comment.
I like the destructuring idea
| const client = { | ||
| handleGetWishlists: () => | ||
| fetch('http://localhost:3001/wishlist', { | ||
| method: 'GET', |
There was a problem hiding this comment.
The fetch API defaults requests to GET unless otherwise specified, so this line can be omitted.
| const [wishlists, setWishlists] = useState([]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Excellent use of React Hooks here!
Though Experiment - useEffect doesn't behave well when passed an async function, but it IS possible to use async/await with useEffect... What would this useEffect need to look like if it made use of async/await?
| it('should load wishlists', () => { | ||
| expect(mockHandleGetWishlists).toBeCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
It might be worth considering testing failure states as well - how would we expect the HomePage to behave in the case of different errors occurring, what would it display or not display?
| expect(component.text()).toContain(wishlist.name); | ||
| expect(component.text()).toContain(wishlist.author); | ||
| expect(component.text()).toContain(wishlist.items[0].name); | ||
| expect(component.text()).toContain(wishlist.items[1].name); | ||
| expect(component.text()).toContain(wishlist.items[2].name); |
There was a problem hiding this comment.
Good attention to detail! It's good to be thorough and check all of these - after all, trusting that everything worked by checking ONE part of the Wishlist's ability to render the wishlist object it was passed as props might not mean it was doing EVERYTHING it was supposed to!
| const filteredItems = items | ||
| .map((item) => item.trim()) | ||
| .filter((item) => item); |
There was a problem hiding this comment.
Array.filter() is used to remove items that are set to a blank string from the array. That's fine, but since we're storing blank strings in the items array, we have to do this procedure of using .filter((item) => item) several more times throughout this document to get the desired result. One of the principles of DRY (Don't Repeat Yourself) programming is that if you have to write something out three times, it's worth encapsulating somehow - so, in the long run, it would probably be good to either: a) encapsulate that functionality in a reusable way, i.e. with a function:
let cleanArr = (arr)=> arr.filter(item=>item);
or b) decide on some other way of working with the items state value so that it is an array that doesn't contain blank strings at any point and thus avoids the issue that way.
| className={classes.input} | ||
| error={nameErrorText ? true : false} | ||
| helperText={nameErrorText} | ||
| id='nameInput' |
There was a problem hiding this comment.
Tip - It's often a good idea to have id and className conform to a non-camelCase naming convention, such as kebab-case or BEM, e.g.:
id='name-input'
Even though in the current situation, we're not using it in any of the CSS files, and we're letting MaterialUI handle a lot of the styling, in the long-term view it can be a good idea for a team to implement a standardized DOM element naming convention that differs from the javascript naming convention of camelCase.
| import { | ||
| Accordion, | ||
| AccordionSummary, | ||
| AccordionDetails, | ||
| Typography, | ||
| FormGroup, | ||
| FormControlLabel, | ||
| Checkbox, | ||
| IconButton, | ||
| } from '@material-ui/core/'; | ||
| import ExpandMoreIcon from '@material-ui/icons/ExpandMore'; | ||
| import DeleteIcon from '@material-ui/icons/Delete'; |
There was a problem hiding this comment.
Good idea to bring in a tech like Material UI here for a project like this one - it can save you a lot of time and energy in making things look the way you want them to!
James-Kelley-Galvanize
left a comment
James-Kelley-Galvanize
left a comment
There was a problem hiding this comment.
Overall Notes:
- Good use of React Hooks such as useState and useEffect.
- Mixed use of async/await and promises, consider standardizing as a team - since you already have Bluebird in the project, it may be worthwhile to make use of it in this case (even with the release of the ES6+ Promise API, Bluebird is marginally faster, and remains SLIGHTLY more efficient than even async/await).
- There are definitely tests for most of the things I'd expect to see tests for, but in general the unit tests aren't particularly fleshed out, i.e. they test only one or just part of the functionality of the unit.
- Likewise, tests could be a bit more "granular", in that they don't necessarily seem to follow the Red-Green-Refactor process of TDD, i.e. they mostly test the full (non-failure case) functionality of the unit in question at its final state, but the tests I'd expect to see preceding that test, which would reflect the TDD process of developing the code, are not necessarily there.
- Tests make good use of mocking and in some cases of multiple "expect" blocks per "it" block to test one functionality more thoroughly (i.e. in Wishlist.test.js).
- Good implementation of techs beyond Express/React/KNEX/PG, such as MaterialUI and Bluebird
- Cool points for implementing simple custom error handling middleware in server.
Thanks for the feedback! |
4 participants
No description provided.