moov-io / ach

ACH implements a reader, writer, and validator for Automated Clearing House (ACH) files. The HTTP server is available in a Docker image and the Go package is available.
https://moov-io.github.io/ach/
Apache License 2.0
455 stars 154 forks source link

Strategy for Adding Support for More Batch Types #105

Closed jjafuller closed 7 years ago

jjafuller commented 7 years ago

I would like to help out adding more batch types, but based on the way the library is currently written I am not sure how you were planning on adding support for batch types other than PPD.

Can you point me in the right direction?

wadearnold commented 7 years ago

I don't have a hashed out strategy but do have some ideas.

I was going to make a Batch interface w/ parse, validate, write, build functions exposed along with any EntryDetail and Addend that was specific to the batch type. Right now these are explicit for PPD files and need to change to (SEC)Header, (SEC)EntryDetail, (SEC)Addendum, (SEC)Control for each of the matching SEC (Standard entry class code) of the different batch types.

The file reader would then need to look at the SEC code of the Batch Header and then use a switch based on the implemented SEC codes. Unsupported SEC codes would still default to an unsupported error.

When building a new batch file the NewBatch function will need to take a SEC code.

wadearnold commented 7 years ago

@jjafuller Starting to try and abstract the Batch SEC type into an interface and make the Batch names, Entry Details, and validation explicit to the SEC code. https://github.com/moov-io/ach/compare/master...wadearnold:batch-sec-interface-refactor

jjafuller commented 7 years ago

It sounds like you're on the right track. One thing to consider is that many of the SEC batch record formats are the same, so the trick is achieving the correct functionality without duplicating logic more than necessary.

Do you have a priority on which SEC codes you want to add? I know that ones I work with the most frequently are PPD, CCD, WEB, and TEL. However, I do work with ENRs, and IAT fairly regularly. IATs are always an issue since they play by their own rules.

wadearnold commented 7 years ago

What I am thinking is that:

  1. Make the Reader, Writer, and all test use the batcher interface. Only BatchPPD concrete type implements the batcher interface at this time. This refactor should get the code back to its current working state for PPD files.

  2. Extract the common Batch, Entry Detail, and Addendum functionality into concrete types that are not used by themselves but composed into the SEC code type. I assume this to be highly iterative per type to figure out what is repeated so that we have DRY code. type batchRCK struct { Batch // code specific to RCK implementation }

  3. Continue implementing the different SEC codes while refactoring out common code to the composable struct for each SEC type.

I know this is pretty high level but what are your thoughts @jjafuller ?

wadearnold commented 7 years ago

@jjafuller Im stuck and wondering if you can help. I can't figure out how to make the Batcher.SetHeader & Batcher.SetControl work without the interface definition requiring the concrete type BatchPPD. Any ideas on how to refactor this so that all BatchSEC types can have a consistent method signature moving forward?

Current Batcher https://github.com/wadearnold/ach/blob/batch-sec-interface-refactor/batcher.go

Current Branch https://github.com/wadearnold/ach/tree/batch-sec-interface-refactor

jjafuller commented 7 years ago

Ah yes, the classic case for generics. You have a few different options for achieving the desired functionality, I think the typical thing to do would to use use an empty interface{} and do type assertions in downstream processing. Take a look at point 4 in this article: https://appliedgo.net/generics/

wadearnold commented 7 years ago

I am at the point that I need to implement another Batch(SEC) type in order to refactor the common parts into batch/batchEntry/Addenda type that can be composed

wadearnold commented 7 years ago

Refactored error handling #105 to make it easier to debug parsing. This should drastically improve the ability to add additional SEC types

wadearnold commented 7 years ago

Whoops pull request was #113 this is 105

jjafuller commented 7 years ago

I'm excited to give this a shot as soon as I get some time to work with it.

wadearnold commented 7 years ago

@jjafuller my next steps are to:

  1. Increase code coverage on Reader
  2. Write a BatchWeb and refactor BatchPPD into a composable batch struct for duplicate code
  3. Try another SEC code.
wadearnold commented 7 years ago

Pull request #114 increases code coverage on Reader.

A fixed width file was not getting all the validation of the file and now is the same as parsing a 94 character line.

killianbrackey commented 7 years ago

@wadearnold I minimally (repetetively) started to implement WEB and CCD in our fork, but am slowly migrating back to the master branch here. I will speak with some bank reps to see what I can share in terms of test files for both of those formats to facilitate support for both SEC codes and their respective test cases. Let me know if there is anything else you want me to take a look at or that you might need.

wadearnold commented 7 years ago

@killianbrackey If you can obfuscate the file but keep the context that would be ideal. I don't need real names and routing numbers but the rest should be ok. There are several NACHA "rules" that I keep finding large vendors not following. Real world examples of additional SEC files to process would be helpful.

I started messing around this week with adding WEB support. I have not finished and probably won't before you are done. Couple items to look at. Adding a Batch type that would be used to compose BatchWEB types and other BatchSEC types. The logic that I would then add into the specific batch types would be the validation that is specific to that type and the NewBatchWEB to make it easier to build. You can compare this branch and see were I was going with it. https://github.com/wadearnold/ach/tree/WEB-SEC-support

Thanks again for your assistance!

wadearnold commented 7 years ago

This latest commit #129 is a pretty major refactor but now supports the ability to easily add additional SEC batch payments types into the library. Please review the readme which now has information on how to add a new batch type.

The library has the stub code for batchWEB and will now read and write a WEB entry but the validation for batchWEB is not implemented. That is my next step. Once that is completed we should be very close to being done with this issue!