Proposal for an improved structure of the API for version 1.1.x

[beeman]

I gave this some thought and came up with a new structure that seems to provide easier access to the things we want.

In the definition of the mixin each property is followed by the name of the method that is triggered on change:

"mixins": {
  "Changed": {
    "properties": {
      "status": "changedStatus",
      "productId": "changedProductId",
      "archived": "changedArchived"
    }
  }
}

When the mixin executes before save it will monitor the properties specified above and will put them all in their own object which gives us the following structure:

var changedProperties: {
  status: {
    ids: {
      '123': 'processing',
      '456': 'processing',
      '789': 'on-hold'
    },
    values: {
      'processing': ['123', '456'],
      'on-hold': ['789']
    }
  },
  productId: {
    ids: {
      '123': 'PRODUCT-1',
      '456': 'PRODUCT-1',
      '789': 'PRODUCT-2'
    },
    values: {
      'PRODUCT-1': ['123', '456'],
      'PRODUCT-2': ['789']
    }
  },
  cancelled: {
    ids: {
      '123': 1
      '456': 1,
      '789': 0
    },
    values: {
      1: ['123', '456'],
      2: ['789']
    }
  }
} 

When the mixin executes after save it will execute the callback function specified in the definition with the changedProperties for that property.

The changed parameter that is passed to the callback functions need to have a few helper methods to easily access the data:

getIds: function() {              // Return an array of ID's
  return Object.keys(this.ids);
},
getId: function(id) {             // Return the value for a specific ID
  return this.ids[id];
},
getValues: function() {           // Return an array of all the values
  return Object.keys(this.values);
},
getValue: function(value) {       // Return the ID's for a specific value
  return this.values[value];
}

The callbacks that are specified in the example above could be implemented like this:

/**

Reset daysInStatus field to today

var changed = {
  ids: {
    '123': 'processing',
    '456': 'processing',
    '789': 'on-hold'
  },
  values: {
    'processing': ['123', '456'],
    'on-hold': ['789']
  }
}
*/
Model.changedStatus = function(changed) {
  Model.updateAll({
    where: {
      id: {
        inq: changed.getIds()
      }
    }
  }, {
    daysInStatus = Helper.getToday()
  }).then(function(count){
    return count
  })
}


/**

Update product metadata when productId changes

var changed = {
  ids: {
    '123': 'PRODUCT-1',
    '456': 'PRODUCT-1',
    '789': 'PRODUCT-2'
  },
  values: {
    'PRODUCT-1': ['123', '456'],
    'PRODUCT-2': ['789']
  }
}
*/
Model.changedProductId = function(changed) {
  // Loop through each productId (the changed value)
  changed.getValues().map(function(productId){

    // Get the product info
    var product = Product.findById({id: productId});

    // Update all Items that have changed to this productId
    Model.updateAll({
      where: {
        id: {
          inq: changed.getValue(productId)
        }
      }
    }, {
      'sku': product.sku,
      'name': product.name,
      'group': product.group
    }).then(function(count){
      return count;
    })
  });
}

/**

Set status to cancelled if archive is true

var changed = {
  ids: {
    '123': 1
    '456': 1,
    '789': 0
  },
  values: {
    1: ['123', '456'],
    2: ['789']
  }
}
*/
Fulfillment.changedArchived = function(changed) {
  Model.updateAll({
    where: {
      id: {
        inq: changed.getIds()
      }
    }
  }, {
    status = 'cancelled';
  }).then(function(count){
    return count
  })
}

One of the advantages of this approach is that it is easy to handle changes on a per-property basis without having to add new logic for it. If you want to handle all properties using the same handler (as it is implemented in version 1.0.x) you can easily do that before.

In a future version it would be nice the possibility of adding the logic of this mixin programatically:

/**
 * var changed = {
 *   ids: {
 *     '123': 'processing',
 *     '456': 'processing',
 *     '789': 'on-hold'
 *   },
 *   values: {
 *     'processing': ['123', '456'],
 *     'on-hold': ['789']
 *   }
 * }
 */
Fulfillment.onChanged('status', function(changed) {
    // do first thing on changed status
});

Fulfillment.onChanged('status', function(changed) {
    // do another thing on changed status
});

And even watch a specific property value:

/**
 * var changed =  ['123', '456'];
 */
Fulfillment.onChangedValue('status', 'cancelled', function(changed) {
    // this is triggered when status changed to cancelled
});

[mrfelton]

So with this proposed approach, would Fulfillment.onChanged be called on/with a single model instance, with a list of changed model instances, or with a structure similar to what we are currently using that includes details of all changed instances?

[beeman]

The idea is that it is called with an object of which ID's have changed along with the value.

This way it shouldn't matter if you are dealing with a single or multiple instances. As both the ID's and values are available you can always loop through all the ID's separately, or update them all in 1 go.

[mrfelton]

Cool, that makes sense.

[beeman]

I have implemented this in the version-1.1.x branch and it all seems to work as designed.

One of the things that I'm currently looking at is how to properly test the ChangeSet that is passed to the callbacks.

it('should execute the callback after updating watched properties on multiple models', function(done) {
  var self = this;
  var spyStatusParams = { ids: {}, values: {} };
  spyStatusParams.ids[this.joe.id] = 'pending';
  spyStatusParams.ids[this.bilbo.id] = 'pending';
  spyStatusParams.values['pending'] = [this.joe.id.toString(), this.bilbo.id.toString()];

  this.Person.updateAll(null, {status: 'pending', name: 'pending'})
  .then(function(res) {
    expect(self.spyAge).not.to.have.been.called;
    expect(self.spyNickname).not.to.have.been.called;
    expect(self.spyStatus).to.have.been.called;
    expect(self.spyStatus).to.have.been.calledWith(spyStatusParams);
    done();
  })
  .catch(done);
});

This test fails with a message like this as it does not invoke the callback with a ChangeSet, but rather a plain object. I guess I need to change the testsuite above to make spyStatusParams a ChangeSet too, but it's unclear to me how to invoke that method/object. Any ideas are welcome.

AssertionError: expected changeStatus to have been called with arguments 
{
  ids: { 5586d573830590d60c26d689: "pending", 5586d573830590d60c26d68a: "pending" },
  values: { pending: ["5586d573830590d60c26d689", 5586d573830590d60c26d68a] }
}
changeStatus(
[ChangeSet] {
  ids: { 5586d573830590d60c26d689: "pending", 5586d573830590d60c26d68a: "pending" },
  values: { pending: ["5586d573830590d60c26d689", "5586d573830590d60c26d68a"] }
}) => [object Promise]