CiviHR's Model Factory

readme.md

CiviHR's Model Factory proposal

Overview

The current structure for models, instances, and apis offer a way to access and manipulate CiviHR's data while separating each responsibility into clear distinctive units.

Unfortunately, current implementations have a lack of cohesion, different services implement the same methods with slight variations, lack of inheritance, spec files repeat the same tests, and instance services are created without a purpose, just to follow the convention.

This document will address these issues and propose solutions for them.

---

Repetition is repeated too repeatedly

We have Model and api services which provide basic functionality that is shared between their specific implementations, but for some reason the all, find, and create methods are rewritten for each model or api implementation:

• Job Role Api defines the all and find methods
• Appraisal Api implements all, find, and create.
• Contact Job Role model only implements the all method.

This is a problem because:

• The same method is being repeated in different files. If a change in implementation is desired, then all the individual methods must be refactored. There's no single source of truth.
• Specs are testing the same thing over and over. (Contact model, job role, contact job role, etc.)
• It's not clear which models or apis have which methods available. Some methods even have different parameters: Appraisal Api, Contract Api, Job Role Api

To solve this, the all, find, and create methods should be moved to the parent model and api services. To allow the model to use specific apis and instances models could be created in the following manner:

Model.extend({
  api: jobContractApi,
  instance: JobContractInstance
});

And the parent model would define the all method as follows:

function all (filters, pagination, order, cache) {
  // ...

  return this.api.all(filters, pagination, order, cache)
    .then(function (response) {
      response.list = response.list.map(function (jobRole) {
        return this.instance.init(jobRole, true);
      }.bind(this));

      return response;
    }.bind(this));
}

Using generic services

In some cases, there is no need to implement a model, instance, or api if it's not going to have any custom functionality outside what their parent service provide. By creating these files "just in case" or "for later" we just increase the ammount of code that must be mantained.

If we pass a generic services it would simplify the creation of new models that have the bare minimum requirements for implementation:

Model.extend({
  api: api.extend({ entity: 'Appraisal' }), // we just need to pass the name of the entity since we don't need any customization
  instance: ModelInstance.extend({}) // we don't need a specific instance, lets just pass a generic one here
});

The all method for the api can be defined as follows:

function all (filters, pagination, sort, cache) {
  return this.getAll(this.entity, filters, pagination, sort, null, null, cache);
}

Unnecesary tests

When a service extends another, we should not test each one of the methods that are provided by the parent service, we should only test new methods or methods that have a different implementation.