Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix two bugs when providing options to new File() and new Batch() #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crotger
Copy link

@crotger crotger commented Oct 11, 2019

Hey there, we're using this for a banking partner that has an alphanumeric (instead of numeric) immediateOrigin value and company ID value. I went to override these values in the schema by using the options.header and options.control values in File and Batch but discovered a couple of bugs there.

Essentially my code looked like this:

const FILE_OPTS = {
  file: {
    header: {
      immediateOrigin: { type: "alphanumeric" },
      // ... other header settings
    }
  },
  batch: {
    header: {
      //... header settings
    },
    control: {
      //... control settings
    }
  }
}

let file = new File(FILE_OPTS.file);
let batch = new Batch(FILE_OPTS.batch);

First, that since the code was using _.merge, the default settings overwrote the ones I passed in, so the type of immediate origin ended up staying as "numeric".
Second, _.merge mutates its input, so my FILE_OPTS constants were changed to the nACH2 defaults.
Third, the code in batch/index.js was using the "header" setting for the control options.

I fixed that in our fork by using _.defaultsDeep instead of _.merge, providing a new empty object to avoid mutating whatever the user has passed in to the constructor, and corrected the typo in the batch options setting to use options.control.

@@ -12,8 +12,8 @@ function Batch(options) {
this._entries = [];

// Allow the batch header/control defaults to be overriden if provided
this.header = options.header ? _.merge(options.header, require('./header'), _.defaults) : _.cloneDeep(require('./header'));
this.control = options.control ? _.merge(options.header, require('./control'), _.defaults) : _.cloneDeep(require('./control'));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line was checking for options.control but loading options.header if it was set.

@@ -10,8 +10,8 @@ function File(options) {
this._batches = [];

// Allow the batch header/control defaults to be overriden if provided
this.header = options.header ? _.merge(options.header, require('./header')(), _.defaults) : require('./header')();
this.control = options.control ? _.merge(options.header, require('./control'), _.defaults) : _.cloneDeep(require('./control'));
this.header = options.header ? _.defaultsDeep({}, options.header, require('./header')()) : require('./header')();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing an empty object as the first argument to defaultsDeep ensures it creates a new object for these settings instead of mutating options.header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant