DO NOT MERGE - just for code review purposes :)

__tests__/spellbooks.test.js

@@ -10,6 +10,8 @@ const mockUser = {
   password: '12345',
 };
 
+// since you're using this function across multiple files
+// you could think about moving it out into a utils file
 const registerAndLogin = async (userProps = {}) => {
   const password = userProps.password ?? mockUser.password;
 
@@ -26,13 +28,14 @@ const registerAndLogin = async (userProps = {}) => {
   return [agent, user];
 };
 
-describe.skip('spellbooks routes', () => {
+describe('spellbooks routes', () => {
   beforeEach(() => {
     return setup(pool);
   });
   afterAll(() => {
     pool.end();
   });
+  // make sure to describe your route -- which route should return all known spells for a user?
   it('should return all known spells for a user', async () => {
     const spell = {
       id: 4,
@@ -44,6 +47,9 @@ describe.skip('spellbooks routes', () => {
       charClass: 'Wizard',
       charLvl: 7,
     };
+    // you might want to consider adjusting your create user route
+    // to allow this additional information to be sent up on create
+    // that would save this extra post after user creation
     const [agent] = await registerAndLogin();
     const user = await agent.patch('/api/v1/users/6').send(userInfo);
     expect(user.body.charClass).toEqual('Wizard');
@@ -58,6 +64,7 @@ describe.skip('spellbooks routes', () => {
         "userId": "6",
       }
     `);
+    // do you need the second test here? isn't this just testing the same thing?
     const anotherLearnedSpell = await agent
       .post('/api/v1/spells/1/learn')
       .send(spell2);
@@ -73,6 +80,7 @@ describe.skip('spellbooks routes', () => {
     const res = await agent.get('/api/v1/spellbook');
     expect(res.body.length).toEqual(2);
   });
+  // which route?
   it('should let users delete a known spell', async () => {
     const spell = {
       id: 4,
@@ -86,8 +94,10 @@ describe.skip('spellbooks routes', () => {
     expect(user.body.charClass).toEqual('Wizard');
     expect(user.body.casterLvl).toEqual(4);
 
-    const learnedSpell = await agent.post('/api/v1/spells/4/learn').send(spell);
-    expect(learnedSpell.body).toMatchInlineSnapshot(`
+    // do you need to send up the spell here?
+    // i think the route just uses the id from the parameters not the request body?
+    const { body } = await agent.post('/api/v1/spells/4/learn').send(spell);
+    expect(body).toMatchInlineSnapshot(`
       Object {
         "id": "8",
         "prepared": false,
@@ -100,6 +110,7 @@ describe.skip('spellbooks routes', () => {
 
     await agent.get('/api/v1/spellbook/4').expect(404);
   });
+
   it('should let users update the preparation of a spell', async () => {
     const spell = {
       id: 4,
@@ -114,6 +125,10 @@ describe.skip('spellbooks routes', () => {
     expect(user.body.casterLvl).toEqual(4);
 
     const learnedSpell = await agent.post('/api/v1/spells/4/learn').send(spell);
+    // you don't need to test this again bc you tested the post works above
+    // you could also just create the spellbook entry directly in the database
+    // using the model to avoid the API call
+    // i.e. Spellbook.insertKnownSpell({userId: user.id, spellId: 4, prepared: false})
     expect(learnedSpell.body).toMatchInlineSnapshot(`
       Object {
         "id": "8",
@@ -130,6 +145,7 @@ describe.skip('spellbooks routes', () => {
       .send(updatedInfo);
     expect(updatedSpell.body.prepared).toEqual(true);
   });
+
   it('should return all prepared spells for a user', async () => {
     const spell = {
       id: 4,

__tests__/spells.test.js

@@ -26,14 +26,18 @@ const registerAndLogin = async (userProps = {}) => {
   return [agent, user];
 };
 
-describe.skip('spell routes', () => {
+describe('spell routes', () => {
   beforeEach(() => {
     return setup(pool);
   });
   afterAll(() => {
     pool.end();
   });
   //TODO figure out why this is suddenly failing when the route works fine in thunderclient
+
+  // because you're not sending up the user info when you login, the
+  // actual user saved on the cookie does not have the most up to date user info
+
   it('should return available spells for a user by charClass and casterLvl', async () => {
     const userInfo = {
       charClass: 'Wizard',

__tests__/users.test.js

@@ -32,7 +32,7 @@ const registerAndLogin = async (userProps = {}) => {
   return [agent, user];
 };
 
-describe.skip('user routes', () => {
+describe('user routes', () => {
   beforeEach(() => {
     return setup(pool);
   });
@@ -47,6 +47,7 @@ describe.skip('user routes', () => {
   });
 
   it('signs in an existing user with an email', async () => {
+    // can you just make this user directly using the UserService to save the API call?
     await request(app).post('/api/v1/users').send(mockExistingUser);
     const res = await request(app)
       .post('/api/v1/users/sessions')
@@ -69,6 +70,22 @@ describe.skip('user routes', () => {
     const res = await agent.get('/api/v1/users/6');
 
     expect(res.body.username).toEqual('Test');
+    // i like to use inlinesnapshot when i have a large object to test against
+    expect(res.body).toMatchInlineSnapshot(`
+      Object {
+        "cantripsKnown": null,
+        "casterLvl": null,
+        "charClass": null,
+        "charLvl": null,
+        "charMod": null,
+        "charName": null,
+        "email": "test@example.com",
+        "id": "6",
+        "profBonus": null,
+        "spellsKnown": null,
+        "username": "Test",
+      }
+    `);
   });
 
   it('should update a user', async () => {
@@ -84,6 +101,7 @@ describe.skip('user routes', () => {
     expect(res.body.charClass).toEqual('Bard');
     expect(res.body.charLvl).toEqual(5);
 
+    // any reason you're testing this twice?
     const newUpdate = {
       charName: 'Dom',
       charClass: 'Sorcerer',
@@ -97,6 +115,7 @@ describe.skip('user routes', () => {
   });
 
   it('should update a users spell slots', async () => {
+    // how is this test different from the test above?
     const userInfo = {
       charName: 'Dandelion',
       charClass: 'Bard',
@@ -105,11 +124,14 @@ describe.skip('user routes', () => {
     const [agent] = await registerAndLogin();
     const user = await agent.patch('/api/v1/users/1').send(userInfo);
     expect(user.status).toBe(200);
-
-    // const res = agent.get();
   });
 
+  // is there a negative test for the update?
+  // meaning, do you need to be the logged in user to update a user?
+  // should you have a test to check that you can't update users that aren't the logged in user?
+
   it('/protected should return a 401 if not authenticated', async () => {
+    // i'm guessing you don't actually need the /protected route - you may just want to delete it if you don't need it
     const res = await request(app).get('/api/v1/users/protected');
     expect(res.status).toEqual(401);
   });
@@ -127,25 +149,29 @@ describe.skip('user routes', () => {
   });
 
   it('/users should return 200 if user is admin', async () => {
+    // i would avoid using your real email in this test
+    // don't expose your email if you don't need to and also this
+    // makes it a more general test that uses your env var
+
     const agent = request.agent(app);
 
     // create a new user
     await agent.post('/api/v1/users').send({
-      email: 'geoffrey.sauer89@gmail.com',
+      email: process.env.EMAIL,
       password: '1234',
     });
     // sign in the user
     await agent
       .post('/api/v1/users/sessions')
-      .send({ email: 'geoffrey.sauer89@gmail.com', password: '1234' });
+      .send({ email: process.env.EMAIL, password: '1234' });
 
     const res = await agent.get('/api/v1/users/');
     expect(res.status).toEqual(200);
   });
 
   it('/users should return a 200 if user is admin', async () => {
     const [agent] = await registerAndLogin({
-      email: 'geoffrey.sauer89@gmail.com',
+      email: process.env.EMAIL,
     });
     const res = await agent.get('/api/v1/users/');
     expect(res.status).toEqual(200);

lib/controllers/spellbooks.js

@@ -18,6 +18,7 @@ module.exports = Router()
   .get('/prepared', async (req, res, next) => {
     try {
       const spells = await Spellbook.getPreparedSpells(req.user.id);
+      // do you want this to 404 if there are no spells?
       if (!spells) {
         next();
       }
@@ -41,6 +42,7 @@ module.exports = Router()
     try {
       const spells = await Spellbook.getKnownSpells(req.user.id);
       if (!spells) {
+        // same question - should this 404 if there are no spells?
         next();
       }
       res.json(spells);

lib/middleware/authorize.js

@@ -1,6 +1,9 @@
 module.exports = async (req, res, next) => {
   try {
-    if (!req.user) throw new Error('You do not have access to view this page');
+    // you may want to make this more specific -- since req.user is an object,
+    // simply having an empty object would pass this check
+    if (!req.user?.email)
+      throw new Error('You do not have access to view this page');
 
     next();
   } catch (err) {

lib/models/Spellbook.js

@@ -80,6 +80,10 @@ module.exports = class Spellbook {
   }
 
   static async getPreparedSpells(id) {
+    // do you want this to return all the user info as well?
+    // if not, you can skip the extra join on users and just filter by
+    // spellbooks.user_id (you may want the user info for the front end but just checking)
+
     const { rows } = await pool.query(
       `SELECT spells.id, spells.index, spells.level, spells.school
       FROM spells