Merge pull request #6615 from jaswilli/delete-204

Use HTTP status 204 on deletes

core/client/app/adapters/base.js

@@ -41,20 +41,6 @@ export default RESTAdapter.extend(DataAdapterMixin, {
         return url;
     },
 
-    // Override deleteRecord to disregard the response body on 2xx responses.
-    // This is currently needed because the API is returning status 200 along
-    // with the JSON object for the deleted entity and Ember expects an empty
-    // response body for successful DELETEs.
-    // Non-2xx (failure) responses will still work correctly as Ember will turn
-    // them into rejected promises.
-    deleteRecord() {
-        let response = this._super(...arguments);
-
-        return response.then(() => {
-            return null;
-        });
-    },
-
     handleResponse(status) {
         if (status === 401) {
             if (this.get('session.isAuthenticated')) {

core/client/app/mirage/config.js

@@ -125,6 +125,12 @@ export default function () {
         return response;
     });
 
+    this.del('/posts/:id/', function (db, request) {
+        db.posts.remove(request.params.id);
+
+        return new Mirage.Response(204, {}, {});
+    });
+
     /* Roles ---------------------------------------------------------------- */
 
     this.get('/roles/', function (db, request) {
@@ -267,7 +273,11 @@ export default function () {
         };
     });
 
-    this.del('/tags/:id/', 'tag');
+    this.del('/tags/:id/', function (db, request) {
+        db.tags.remove(request.params.id);
+
+        return new Mirage.Response(204, {}, {});
+    });
 
     /* Users ---------------------------------------------------------------- */
 
@@ -304,7 +314,11 @@ export default function () {
         };
     });
 
-    this.del('/users/:id/', 'user');
+    this.del('/users/:id/', function (db, request) {
+        db.users.remove(request.params.id);
+
+        return new Mirage.Response(204, {}, {});
+    });
 
     this.get('/users/:id', function (db, request) {
         return {

core/server/api/index.js

@@ -56,20 +56,22 @@ cacheInvalidationHeader = function cacheInvalidationHeader(req, result) {
     var parsedUrl = req._parsedUrl.pathname.replace(/^\/|\/$/g, '').split('/'),
         method = req.method,
         endpoint = parsedUrl[0],
-        cacheInvalidate,
         jsonResult = result.toJSON ? result.toJSON() : result,
+        INVALIDATE_ALL = '/*',
         post,
         hasStatusChanged,
-        wasDeleted,
         wasPublishedUpdated;
 
-    if (method === 'POST' || method === 'PUT' || method === 'DELETE') {
+    if (['POST', 'PUT', 'DELETE'].indexOf(method) > -1) {
-        if (endpoint === 'settings' || endpoint === 'users' || endpoint === 'db' || endpoint === 'tags') {
+        if (['settings', 'users', 'db', 'tags'].indexOf(endpoint) > -1) {
-            cacheInvalidate = '/*';
+            return INVALIDATE_ALL;
         } else if (endpoint === 'posts') {
+            if (method === 'DELETE') {
+                return INVALIDATE_ALL;
+            }
+
             post = jsonResult.posts[0];
             hasStatusChanged = post.statusChanged;
-            wasDeleted = method === 'DELETE';
             // Invalidate cache when post was updated but not when post is draft
             wasPublishedUpdated = method === 'PUT' && post.status === 'published';
 
@@ -77,15 +79,13 @@ cacheInvalidationHeader = function cacheInvalidationHeader(req, result) {
             delete post.statusChanged;
 
             // Don't set x-cache-invalidate header for drafts
-            if (hasStatusChanged || wasDeleted || wasPublishedUpdated) {
+            if (hasStatusChanged || wasPublishedUpdated) {
-                cacheInvalidate = '/*';
+                return INVALIDATE_ALL;
             } else {
-                cacheInvalidate = '/' + config.routeKeywords.preview + '/' + post.uuid + '/';
+                return '/' + config.routeKeywords.preview + '/' + post.uuid + '/';
             }
         }
     }
-
-    return cacheInvalidate;
 };
 
 /**
@@ -209,6 +209,10 @@ http = function http(apiMethod) {
             // Add X-Cache-Invalidate, Location, and Content-Disposition headers
             return addHeaders(apiMethod, req, res, (response || {}));
         }).then(function then(response) {
+            if (req.method === 'DELETE') {
+                return res.status(204).end();
+            }
+
             // Send a properly formatting HTTP response containing the data with correct headers
             res.json(response || {});
         }).catch(function onAPIError(error) {

core/server/api/notifications.js

@@ -115,7 +115,7 @@ notifications = {
      * Remove a specific notification
      *
      * @param {{id (required), context}} options
-     * @returns {Promise(Notifications)}
+     * @returns {Promise}
      */
     destroy: function destroy(options) {
         var tasks;
@@ -153,8 +153,6 @@ notifications = {
                 return element.id === parseInt(options.id, 10);
             });
             notificationCounter = notificationCounter - 1;
-
-            return notification;
         }
 
         tasks = [
@@ -163,9 +161,7 @@ notifications = {
             destroyNotification
         ];
 
-        return pipeline(tasks, options).then(function formatResponse(result) {
+        return pipeline(tasks, options);
-            return {notifications: [result]};
-        });
     },
 
     /**

core/server/api/posts.js

@@ -206,7 +206,7 @@ posts = {
      *
      * @public
      * @param {{id (required), context,...}} options
-     * @return {Promise(Post)} Deleted Post
+     * @return {Promise}
      */
     destroy: function destroy(options) {
         var tasks;
@@ -214,15 +214,14 @@ posts = {
         /**
          * @function deletePost
          * @param  {Object} options
-         * @return {Object} JSON representation of the deleted post
          */
         function deletePost(options) {
             var Post = dataProvider.Post,
                 data = _.defaults({status: 'all'}, options),
-                fetchOpts = _.defaults({require: true}, options);
+                fetchOpts = _.defaults({require: true, columns: 'id'}, options);
 
             return Post.findOne(data, fetchOpts).then(function (post) {
-                return post.destroy(options).return({posts: [post.toJSON()]});
+                return post.destroy(options).return(null);
             }).catch(Post.NotFoundError, function () {
                 throw new errors.NotFoundError(i18n.t('errors.api.posts.postNotFound'));
             });
@@ -237,9 +236,7 @@ posts = {
         ];
 
         // Pipeline calls each task passing the result of one to be the arguments for the next
-        return pipeline(tasks, options).tap(function formatResponse(response) {
+        return pipeline(tasks, options);
-            response.posts[0].statusChanged = true;
-        });
     }
 };
 

core/server/api/tags.js

@@ -164,23 +164,18 @@ tags = {
      *
      * @public
      * @param {{id, context}} options
-     * @return {Promise<Tag>} Deleted Tag
+     * @return {Promise}
      */
     destroy: function destroy(options) {
         var tasks;
 
         /**
-         * ### Model Query
+         * ### Delete Tag
          * Make the call to the Model layer
          * @param {Object} options
-         * @returns {Object} options
          */
-        function doQuery(options) {
+        function deleteTag(options) {
-            return tags.read(options).then(function (result) {
+            return dataProvider.Tag.destroy(options).return(null);
-                return dataProvider.Tag.destroy(options).then(function () {
-                    return result;
-                });
-            });
         }
 
         // Push all of our tasks into a `tasks` array in the correct order
@@ -188,7 +183,7 @@ tags = {
             utils.validate(docName, {opts: utils.idDefaultOptions}),
             utils.handlePermissions(docName, 'destroy'),
             utils.convertOptions(allowedIncludes),
-            doQuery
+            deleteTag
         ];
 
         // Pipeline calls each task passing the result of one to be the arguments for the next

core/server/api/users.js

@@ -361,7 +361,7 @@ users = {
     /**
      * ## Destroy
      * @param {{id, context}} options
-     * @returns {Promise<User>}
+     * @returns {Promise}
      */
     destroy: function destroy(options) {
         var tasks;
@@ -382,33 +382,22 @@ users = {
         }
 
         /**
-         * ### Model Query
+         * ### Delete User
          * Make the call to the Model layer
          * @param {Object} options
-         * @returns {Object} options
          */
-        function doQuery(options) {
+        function deleteUser(options) {
-            return users.read(options).then(function (result) {
+            return dataProvider.Base.transaction(function (t) {
-                return dataProvider.Base.transaction(function (t) {
+                options.transacting = t;
-                    options.transacting = t;
 
-                    Promise.all([
+                return Promise.all([
-                        dataProvider.Accesstoken.destroyByUser(options),
+                    dataProvider.Accesstoken.destroyByUser(options),
-                        dataProvider.Refreshtoken.destroyByUser(options),
+                    dataProvider.Refreshtoken.destroyByUser(options),
-                        dataProvider.Post.destroyByAuthor(options)
+                    dataProvider.Post.destroyByAuthor(options)
-                    ]).then(function () {
+                ]).then(function () {
-                        return dataProvider.User.destroy(options);
+                    return dataProvider.User.destroy(options);
-                    }).then(function () {
+                }).return(null);
-                        t.commit();
+            }).catch(function (error) {
-                    }).catch(function (error) {
-                        t.rollback(error);
-                    });
-                }).then(function () {
-                    return result;
-                }, function (error) {
-                    return Promise.reject(new errors.InternalServerError(error));
-                });
-            }, function (error) {
                 return errors.formatAndRejectAPIError(error);
             });
         }
@@ -418,7 +407,7 @@ users = {
             utils.validate(docName, {opts: utils.idDefaultOptions}),
             handlePermissions,
             utils.convertOptions(allowedIncludes),
-            doQuery
+            deleteUser
         ];
 
         // Pipeline calls each task passing the result of one to be the arguments for the next

core/server/models/post.js

@@ -375,7 +375,7 @@ Post = ghostBookshelf.Model.extend({
             // whitelists for the `options` hash argument on methods, by method name.
             // these are the only options that can be passed to Bookshelf / Knex.
             validOptions = {
-                findOne: ['importing', 'withRelated', 'require'],
+                findOne: ['columns', 'importing', 'withRelated', 'require'],
                 findPage: ['page', 'limit', 'columns', 'filter', 'order', 'status', 'staticPages'],
                 findAll: ['columns'],
                 add: ['importing']

core/test/functional/routes/api/notifications_spec.js

@@ -94,20 +94,13 @@ describe('Notifications API', function () {
                     // begin delete test
                     request.del(location)
                         .set('Authorization', 'Bearer ' + accesstoken)
-                        .expect('Content-Type', /json/)
+                        .expect(204)
-                        .expect(200)
                         .end(function (err, res) {
                             if (err) {
                                 return done(err);
                             }
 
-                            // a delete returns a JSON object containing the notification
+                            res.body.should.be.empty();
-                            // we just deleted.
-                            var deleteResponse = res.body;
-                            should.exist(deleteResponse.notifications);
-                            deleteResponse.notifications[0].type.should.equal(newNotification.type);
-                            deleteResponse.notifications[0].message.should.equal(newNotification.message);
-                            deleteResponse.notifications[0].status.should.equal(newNotification.status);
 
                             done();
                         });

core/test/functional/routes/api/posts_spec.js

@@ -844,20 +844,16 @@ describe('Post API', function () {
             var deletePostId = 1;
             request.del(testUtils.API.getApiQuery('posts/' + deletePostId + '/'))
                 .set('Authorization', 'Bearer ' + accesstoken)
-                .expect('Content-Type', /json/)
                 .expect('Cache-Control', testUtils.cacheRules.private)
-                .expect(200)
+                .expect(204)
                 .end(function (err, res) {
                     if (err) {
                         return done(err);
                     }
 
-                    var jsonResponse = res.body;
+                    res.body.should.be.empty();
-                    should.exist(jsonResponse);
-                    should.exist(jsonResponse.posts);
                     res.headers['x-cache-invalidate'].should.eql('/*');
-                    testUtils.API.checkResponse(jsonResponse.posts[0], 'post');
+
-                    jsonResponse.posts[0].id.should.eql(deletePostId);
                     done();
                 });
         });
@@ -907,18 +903,15 @@ describe('Post API', function () {
 
                     request.del(testUtils.API.getApiQuery('posts/' + draftPost.posts[0].id + '/'))
                         .set('Authorization', 'Bearer ' + accesstoken)
-                        .expect('Content-Type', /json/)
                         .expect('Cache-Control', testUtils.cacheRules.private)
-                        .expect(200)
+                        .expect(204)
                         .end(function (err, res) {
                             if (err) {
                                 return done(err);
                             }
 
-                            var jsonResponse = res.body;
+                            res.body.should.be.empty();
-                            should.exist(jsonResponse);
+
-                            should.exist(jsonResponse.posts);
-                            testUtils.API.checkResponse(jsonResponse.posts[0], 'post');
                             done();
                         });
                 });

core/test/integration/api/api_notifications_spec.js

@@ -123,9 +123,7 @@ describe('Notifications API', function () {
             NotificationsAPI.destroy(
                 _.extend({}, testUtils.context.internal, {id: notification.id})
             ).then(function (result) {
-                should.exist(result);
+                should.not.exist(result);
-                should.exist(result.notifications);
-                result.notifications[0].id.should.equal(notification.id);
 
                 done();
             }).catch(done);
@@ -144,9 +142,7 @@ describe('Notifications API', function () {
             NotificationsAPI.destroy(
                 _.extend({}, testUtils.context.owner, {id: notification.id})
             ).then(function (result) {
-                should.exist(result);
+                should.not.exist(result);
-                should.exist(result.notifications);
-                result.notifications[0].id.should.equal(notification.id);
 
                 done();
             }).catch(done);

core/test/integration/api/api_posts_spec.js

@@ -552,4 +552,36 @@ describe('Post API', function () {
             });
         });
     });
+
+    describe('Destroy', function () {
+        it('can delete a post', function (done) {
+            var options = {context: {user: 1}, id: 1};
+
+            PostAPI.read(options).then(function (results) {
+                should.exist(results.posts[0]);
+
+                return PostAPI.destroy(options);
+            }).then(function (results) {
+                should.not.exist(results);
+
+                return PostAPI.read(options);
+            }).then(function () {
+                done(new Error('Post still exists when it should have been deleted'));
+            }).catch(function () {
+                done();
+            });
+        });
+
+        it('returns an error when attempting to delete a non-existent post', function (done) {
+            var options = {context: {user: 1}, id: 123456788};
+
+            PostAPI.destroy(options).then(function () {
+                done(new Error('No error was thrown'));
+            }).catch(function (error) {
+                error.errorType.should.eql('NotFoundError');
+
+                done();
+            });
+        });
+    });
 });

core/test/integration/api/api_tags_spec.js

@@ -126,9 +126,8 @@ describe('Tags API', function () {
         it('can destroy Tag', function (done) {
             TagAPI.destroy(_.extend({}, testUtils.context.admin, {id: firstTag}))
                 .then(function (results) {
-                    should.exist(results);
+                    should.not.exist(results);
-                    should.exist(results.tags);
+
-                    results.tags.length.should.be.above(0);
                     done();
                 }).catch(done);
         });

core/test/integration/api/api_users_spec.js

@@ -693,15 +693,6 @@ describe('Users API', function () {
     });
 
     describe('Destroy', function () {
-        function checkDestroyResponse(response) {
-            should.exist(response);
-            should.exist(response.users);
-            should.not.exist(response.meta);
-            response.users.should.have.length(1);
-            testUtils.API.checkResponse(response.users[0], 'user');
-            response.users[0].created_at.should.be.an.instanceof(Date);
-        }
-
         describe('Owner', function () {
             it('CANNOT destroy self', function (done) {
                 UserAPI.destroy(_.extend({}, context.owner, {id: userIdFor.owner}))
@@ -714,16 +705,16 @@ describe('Users API', function () {
                 // Admin
                 UserAPI.destroy(_.extend({}, context.owner, {id: userIdFor.admin}))
                     .then(function (response) {
-                        checkDestroyResponse(response);
+                        should.not.exist(response);
                         // Editor
                         return UserAPI.destroy(_.extend({}, context.owner, {id: userIdFor.editor}));
                     }).then(function (response) {
-                        checkDestroyResponse(response);
+                        should.not.exist(response);
 
                         // Author
                         return UserAPI.destroy(_.extend({}, context.owner, {id: userIdFor.author}));
                     }).then(function (response) {
-                        checkDestroyResponse(response);
+                        should.not.exist(response);
 
                         done();
                     }).catch(done);
@@ -742,17 +733,17 @@ describe('Users API', function () {
                 // Admin
                 UserAPI.destroy(_.extend({}, context.admin, {id: userIdFor.admin2}))
                     .then(function (response) {
-                        checkDestroyResponse(response);
+                        should.not.exist(response);
 
                         // Editor
                         return UserAPI.destroy(_.extend({}, context.admin, {id: userIdFor.editor2}));
                     }).then(function (response) {
-                        checkDestroyResponse(response);
+                        should.not.exist(response);
 
                         // Author
                         return UserAPI.destroy(_.extend({}, context.admin, {id: userIdFor.author2}));
                     }).then(function (response) {
-                        checkDestroyResponse(response);
+                        should.not.exist(response);
 
                         done();
                     }).catch(done);
@@ -784,7 +775,7 @@ describe('Users API', function () {
             it('Can destroy self', function (done) {
                 UserAPI.destroy(_.extend({}, context.editor, {id: userIdFor.editor}))
                     .then(function (response) {
-                        checkDestroyResponse(response);
+                        should.not.exist(response);
                         done();
                     }).catch(done);
             });
@@ -792,7 +783,7 @@ describe('Users API', function () {
             it('Can destroy author', function (done) {
                 UserAPI.destroy(_.extend({}, context.editor, {id: userIdFor.author}))
                     .then(function (response) {
-                        checkDestroyResponse(response);
+                        should.not.exist(response);
                         done();
                     }).catch(done);
             });