Allow softDelete to support dates/timestamps

[ekryski]

We initially assumed that softDelete should just be a boolean deleted flag on a record. However, dates are a much better value because they can be used the same way but also provide time context. Therefore checking for a field like removedAt or archivedAt is much better.

This, however requires modifying the queries that are in the hook to account for date comparisons.

[eddyystop]

Yup, I should have known better than to use a flag.

We'd have to introduce a new param. The default would be to use the deleted flag, an option would be to use a date.

• Would a value of new Date().getTime() be acceptable to all DBs?
• What would an existance test for such a field be, acceptable for all DBs?

[jvatic]

As far as naming goes, I think deletedAt makes the most sense here. It should also be consistent with setCreatedAt/setUpdatedAt which use new Date().

[dejangeci]

Perhaps add a new hook setDeletedAt and depricate softDelete?

[eddyystop]

softDelete is problematic for the user service for several reasons. I think we need softDelete(options) but the 'why' of the options will not be easy for people to understand. Worse there is a conflict between softDelete and feathers-authentication-local.

[eddyystop]

From foxhound

const checkContext = require('feathers-hooks-common/lib/services/check-context');
const errors = require('@feathersjs/errors');

module.exports = function (field) {
  const deleteField = field || 'deletedAt';

  return function (hook) {
    const service = hook.service;
    hook.data = hook.data || {};
    hook.params.query = hook.params.query || {};
    checkContext(hook, 'before', null, 'softDelete');

    if (hook.params.query.$disableSoftDelete) {
      delete hook.params.query.$disableSoftDelete;
      return hook;
    }

    switch (hook.method) {
    case 'find':
      hook.params.query[deleteField] = null;
      return hook;
    case 'get':
      return throwIfItemDeleted('get', hook.id)
        .then(data => {
          hook.result = data;
          return hook;
        });
    case 'create':
      return hook;
    case 'update': // fall through
    case 'patch':
      if (hook.id !== null) {
        return throwIfItemDeleted('patch', hook.id)
          .then(() => hook);
      }
      hook.params.query[deleteField] = null;
      return hook;
    case 'remove':
      return Promise.resolve()
        .then(() => hook.id ? throwIfItemDeleted('remove', hook.id) : null)
        .then(() => {
          hook.data[deleteField] = new Date();
          hook.params.query[deleteField] = null;
          hook.params.query.$disableSoftDelete = true;

          return service.patch(hook.id, hook.data, hook.params)
            .then(result => {
              hook.result = result;
              return hook;
            });
        });
    }

    function throwIfItemDeleted (type, id) {
      const params = (type === 'get' || type === 'patch') <--- likely wrong.
        ? hook.params
        : {
          query: {},
          _populate: 'skip',
          user: hook.params.user,
          authenticated: hook.params.authenticated,
          provider: hook.params.provider,
        };

      params.query.$disableSoftDelete = true;

      return service.get(id, params)
        .then(data => {
          delete params.query.$disableSoftDelete;

          if (data[deleteField]) {
            throw new errors.NotFound('Item has been soft deleted.');
          }
          return data;
        })
        .catch(() => {
          throw new errors.NotFound('Item not found.');
        });
    }
  };
};

Hi, I updated the soft delete hook to work with buzzard with a date field instead a boolean, but I think I found an issue, I noticed that activating it on before all of a service, the patch call returns item not found, I think I fixed it, but I’m not sure, can you take a look at the code? I have also the test. Thanks.

const { expect } = require('chai');
const app = require('../../src/app');
const users = require('../../src/seeds/factories/users');

describe('Hook: softDelete', () =>
  it('deletedAt checks', () =>
    users.seed(3).then((res) => {
      res.map((item) => expect(item.deletedAt).to.be.null);

      describe('Hook: check softDelete on remove()', () =>
        it('deletedAt should not be null on remove()', () =>
          app.service('users').remove(res[0]._id).then((res) =>
            expect(res.deletedAt).to.not.be.null
          )));

      describe('Hook: check softDelete on find()', () =>
        it('deletedAt should be null on find()', () =>
          app.service('users').find({ paginate: false }).then((res) =>
            res.map((item) => expect(item.deletedAt).to.be.null)
          )));

    })));

Item not found is fixed by

54 `function throwIfItemDeleted (type, id) {`
55 `const params = (type === 'get' || type === 'patch')`

[foxhound87]

I've updated more this hooks,item not found error is fixed by doing:
if (method === 'patch') params.provider = undefined;

the new complete throwIfItemDeleted code:

    function throwIfItemDeleted (method, id) {
      const params = (method === 'get')
        ? hook.params
        : {
          query: {},
          _populate: 'skip',
          user: hook.params.user,
          authenticated: hook.params.authenticated,
          provider: hook.params.provider,
        };

      params.query.$disableSoftDelete = true;
      // fixes patch: "item not found" error
      if (method === 'patch') params.provider = undefined;

      return service.get(id, params)
        .then(data => {
          delete params.query.$disableSoftDelete;

          if (data[deleteField])
            throw new errors.NotFound('Item has been soft deleted.');

          return data;
        })
        .catch(() => {
          throw new errors.NotFound(msg);
        });
    }

I still do not understad why the params const is defined with that condition...

[eddyystop]

softDelete2 is a noteable improvement over softDelete. It set deletedAt as Date.now().

I'm closing this issue now.