Add update expenses route (#414)

server/constants/activities.js

@@ -7,6 +7,7 @@ module.exports = {
   GROUP_CREATED: 'group.created',
   GROUP_EXPENSE_CREATED: 'group.expense.created',
   GROUP_EXPENSE_DELETED: 'group.expense.deleted',
+  GROUP_EXPENSE_UPDATED: 'group.expense.updated',
   GROUP_EXPENSE_REJECTED: 'group.expense.rejected',
   GROUP_EXPENSE_APPROVED: 'group.expense.approved',
   GROUP_TRANSACTION_CREATED: 'group.transaction.created',

server/controllers/expenses.js

@@ -28,7 +28,8 @@ module.exports = (app) => {
     const group = req.group;
     const attributes = Object.assign({}, req.required.expense, {
       UserId: user.id,
-      GroupId: group.id
+      GroupId: group.id,
+      lastEditedById: user.id
     });
     models.Expense.create(attributes)
       .then(expense => models.Expense.findById(expense.id, { include: [ models.Group, models.User ]}))
@@ -43,14 +44,46 @@ module.exports = (app) => {
 
   const deleteExpense = (req, res, next) => {
     const expense = req.expense;
+    const user = req.remoteUser || req.user;
 
     assertExpenseStatus(expense, status.PENDING)
+      .then(() => expense.lastEditedById = user.id)
+      .then(() => expense.save())
       .then(() => expense.destroy())
       .tap(expense => createActivity(expense, activities.GROUP_EXPENSE_DELETED))
       .tap(() => res.send({success: true}))
       .catch(next);
   };
 
+  const update = (req, res, next) => {
+    const origExpense = req.expense;
+    const newExpense = req.required.expense;
+    const user = req.remoteUser || req.user;
+    const modifiableProps = [
+      'amount',
+      'attachment',
+      'category',
+      'comment',
+      'createdAt',
+      'currency',
+      'notes',
+      'payoutMethod',
+      'title',
+      'vat'
+    ];
+
+    assertExpenseStatus(origExpense, status.PENDING)
+      .tap(() => {
+        modifiableProps.forEach(prop => origExpense[prop] = newExpense[prop] || origExpense[prop]);
+        origExpense.updatedAt = new Date();
+        origExpense.lastEditedById = user.id;
+      })
+      .then(() => origExpense.save())
+      .tap(expense => createActivity(expense, activities.GROUP_EXPENSE_UPDATED))
+      .tap(expense => res.send(expense.info))
+      .catch(next);
+  };
+
   /**
    * Approve or reject an expense.
    */
@@ -165,6 +198,7 @@ module.exports = (app) => {
   return {
     create,
     deleteExpense,
+    update,
     setApprovalStatus,
     pay
   };

server/routes.js

@@ -153,6 +153,7 @@ module.exports = (app) => {
   app.post('/groups/:groupid/transactions', commonLegacySecurityMw, required('transaction'), mw.getOrCreateUser, groups.createTransaction); // Create a transaction for a group.
 
   app.get('/groups/:groupid/transactions/:transactionid', aZ.authorizeAccessToGroup({authIfPublic: true}), aZ.authorizeGroupAccessToTransaction({authIfPublic: true}), groups.getTransaction); // Get a transaction.
+  // TODO remove #postmigration, replaced by PUT /groups/:groupid/expenses/:expenseid
   app.put('/groups/:groupid/transactions/:transactionid', aZ.authorizeAccessToGroup(), aZ.authorizeGroupAccessToTransaction(), required('transaction'), groups.updateTransaction); // Update a transaction.
   // TODO remove #postmigration, replaced by DEL /groups/:groupid/expenses/:expenseid
   app.delete('/groups/:groupid/transactions/:transactionid', aZ.authorizeAccessToGroup({userRoles: [HOST], bypassUserRolesCheckIfAuthenticatedAsAppAndNotUser: true}), aZ.authorizeGroupAccessToTransaction(), groups.deleteTransaction); // Delete a transaction.
@@ -171,6 +172,7 @@ module.exports = (app) => {
   // TODO refactor with single route using authentication.js and authorization.js middleware
   app.post('/groups/:groupid/expenses', commonLegacySecurityMw, mw.authorizeIfGroupPublic, mw.authorizeAuthUserOrApp, mw.authorizeGroup, required('expense'), mw.getOrCreateUser, expenses.create); // Create an expense.
   app.post('/groups/:groupid/expenses', commonLegacySecurityMw, required('expense'), mw.getOrCreateUser, expenses.create); // Create an expense.
+  app.put('/groups/:groupid/expenses/:expenseid', aZ.authorizeAccessToGroup(), aZ.authorizeGroupAccessTo('expense'), required('expense'), expenses.update); // Update an expense.
   // TODO is option bypassUserRolesCheckIfAuthenticatedAsAppAndNotUser present in DEL /groups/:id/transactions/:id really needed?
   app.delete('/groups/:groupid/expenses/:expenseid', aZ.authorizeAccessToGroup({userRoles: [HOST]}), aZ.authorizeGroupAccessTo('expense'), expenses.deleteExpense); // Delete an expense.
   app.post('/groups/:groupid/expenses/:expenseid/approve', aZ.authorizeAccessToGroup(), aZ.authorizeGroupAccessTo('expense'), required('approved'), expenses.setApprovalStatus); // Approve an expense.

test/expenses.routes.test.js

@@ -50,6 +50,17 @@ describe('expenses.routes.test.js', () => {
 
       it('THEN returns 404', () => req.expect(404));
     });
+
+    describe('#update', () => {
+      beforeEach(() => {
+        req = request(app)
+          .put(`/groups/${group.id}/expenses/123`)
+          .set('Authorization', `Bearer ${user.jwt(application)}`);
+      });
+
+      it('THEN returns 404', () => req.expect(404));
+    });
+
   });
 
   describe('#create', () => {
@@ -181,6 +192,55 @@ describe('expenses.routes.test.js', () => {
             });
           });
 
+          describe('#update', () => {
+            describe('WHEN not authenticated', () =>
+              it('THEN returns 401', () => request(app)
+                .put(`/groups/${group.id}/expenses/${actualExpense.id}`)
+                .expect(401)));
+
+            describe('WHEN expense does not belong to group', () => {
+              var otherExpense;
+
+              beforeEach(() => createOtherExpense().tap(e => otherExpense = e));
+
+              it('THEN returns 403', () => request(app)
+                .put(`/groups/${group.id}/expenses/${otherExpense.id}`)
+                .set('Authorization', `Bearer ${user.jwt(application)}`)
+                .expect(403));
+            });
+
+            describe('WHEN not providing expense', () => {
+              var updateReq;
+
+              beforeEach(() => {
+                updateReq = request(app)
+                  .put(`/groups/${group.id}/expenses/${actualExpense.id}`)
+                  .set('Authorization', `Bearer ${user.jwt(application)}`);
+              });
+
+              it('THEN returns 400', () => missingRequired(updateReq, 'expense'));
+            });
+
+            describe('success', () => {
+              var response;
+
+              beforeEach(() => request(app)
+                .put(`/groups/${group.id}/expenses/${actualExpense.id}`)
+                .set('Authorization', `Bearer ${user.jwt(application)}`)
+                .send({expense: {title: 'new title'}})
+                .expect(200)
+                .then(res => response = res.body));
+
+              it('THEN returns modified expense', () => {
+                expect(response.title).to.be.equal('new title');
+                expect(response.category).to.be.equal('Engineering');
+              });
+
+              it('THEN a group.expense.updated activity is created', () =>
+                expectExpenseActivity('group.expense.updated', actualExpense.id));
+            });
+          });
+
           describe('#approve', () => {
             var approveReq;
 

test/lib/expectHelpers.js

@@ -1,5 +1,5 @@
 module.exports = {
-  
+
   badRequest(req, message) {
     req.expect(400, {
       error: {
@@ -9,18 +9,17 @@ module.exports = {
       }
     })
   },
-  
+
   missingRequired(req, field) {
-    req.expect(400, {
+    return req.expect(400, {
       error: {
         code: 400,
         type: 'missing_required',
         message: 'Missing required fields',
         fields: {
-          payoutMethod: `Required field ${field} missing`
+          [field]: `Required field ${field} missing`
         }
       }
     })
   }
-
 };