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

Fixes issue #29 and allows optional fields to be omitted during object creation #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ianberryman
Copy link

Address issue #29 by adding default value to discretionary data field and all "required: false" fields. Default value is space-filled to field width.

Address issue glenselle#29 by adding default value to discretionary data field and all "required: false" fields, except those with "blank: true".  Default value is space-filled to field width.
type: 'alphanumeric',
value: utils.SPACE.repeat(8)
}
};

Choose a reason for hiding this comment

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

I suppose removing the newline at the end of the file was not intentional. Could you put it back in?

lib/utils.js Outdated
module.exports.getNextMultipleDiff = getNextMultipleDiff;
module.exports.SPACE = SPACE;

Choose a reason for hiding this comment

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

While we're at it, it would be nice to add the file-ending newline here too.

@waldyrious
Copy link

@berryman17 thanks for this, I hit the same issue myself.

Could you please add a test to test/entry.js to cover this change? Something like this, for example:

it('should create an entry even if optional fields are missing', function(){
  var entry = new Entry({
    receivingDFI: '081000210',
    DFIAccount: '12345678901234567',
    amount: '3521',
    transactionCode: '22',
    individualName: 'Glen Selle',
  });
  entry.generateString(function(string) {
    console.log(string);
  });
});

This fails on current master, but it should pass with this PR.

@ianberryman
Copy link
Author

Thanks for the review @waldyrious I've committed some changes to address your requests.

@waldyrious
Copy link

@joaopaulofonseca can you take a look when you get the chance? This would be useful for us, as we discussed previously (to avoid having to pass empty strings in these fields).

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.

2 participants