Add header record using Fixy and DryRB Validation gems by zachingle · Pull Request #5 · Zepto-Dumpster/mt9

zachingle · 2022-05-20T02:25:32Z

This PR uses both the fixy and dry-validation gems to generate and validate the header record. dry-validation is used on the header record's initialisation to validate the data coming into the object. We then use fixy to format the data to ASB specs.

Specs for the MT9 file format can be found on page 42 and 62 of the ASB File Formats Technical Guide or the MT9 File Formats found in our GDrive

A macro is used for the account number so the rules can be shared with the detail record.

Seems like there is an issue for fixy subclass records not properly inheriting the line ending and record fields: OmarSkalli/fixy#25 but I don't think this is a big blocker. It'll just mean duplicating the set_line_ending line across the records.

gzzengwei · 2022-05-20T04:19:47Z

lib/mt9/header_record.rb

+    attr_reader :file_type, :account_number, :due_date, :client_short_name
+
+    field :file_type, 2, "1-2", :file_type
+    field :account_number, 15, "3-17", :account_number


should we split this to

bank number 2

branch number 4

account number 7

or at least

bank + branch number 6

account number 7

If we do want to split up the account number, we'll have to split out the suffix as well. However for ease of use I think we should keep the branch/branch number, unique number and suffix together, as that is how most New Zealanders use it, and I don't see much benefit in doing so (apart from keeping close relation to the fields in the ASB docs)

ah, I dont hv experience of dealing wih NZ bank transfer

coz in our split app's bank_account model, we have branch_code and account_number (used as BSB and A/C), so I would imagine we could do sth similar for NZ

if we decide to use one field for all related account number in the gem, then we need to figure out the mapping in our adapter

Yeah I think we'll figure out that mapping in the app. It should be pretty simple (just concatenating the two numbers together). We'll already be doing the reverse (splitting the branch_code and account_number from a larger account number) as we've decided on only showing the account number in the UI/API when in NZ

chris-teague

A few thoughts:

Fixy seems to suit what we're doing well
It's a small & relatively inactive gem, which concerns me a little
- but, it's codebase is small, easy enough to follow & something we could easily extend / support if needed
I'm not sure I like using Fixy formatters for validations - it's going to get clunky & have a lot of repetition once we add the DetailRecords too.
- To that end - I'm thinking that we should probably use DryRB Validations in the initializer (i.e. have a Contract for HeaderRecord, DetailRecord etc, if it isn't met, return the error object)

zachingle · 2022-05-24T00:45:53Z

A few thoughts:

Fixy seems to suit what we're doing well

It's a small & relatively inactive gem, which concerns me a little

but, it's codebase is small, easy enough to follow & something we could easily extend / support if needed

I'm not sure I like using Fixy formatters for validations - it's going to get clunky & have a lot of repetition once we add the DetailRecords too.

To that end - I'm thinking that we should probably use DryRB Validations in the initializer (i.e. have a Contract for HeaderRecord, DetailRecord etc, if it isn't met, return the error object)

PR Updated to reflect new approach. I like this a lot more compared to just using Fixy. I've raised a validation error for when the data is invalid so it fails fast. We still need to figure out how we can share the account number schema/rule between the header and detail record. dry-validaton does support inheritance of contracts but IDK if we can pull out single rules

corroded · 2022-05-24T07:28:05Z

.rubocop.yml

+  Enabled: false

-Layout/LineLength:
-  Max: 120


we should probs copy the current rubocop yml we have in zfp and split(main zepto) repos

corroded · 2022-05-24T07:28:26Z

.rubocop.yml

 AllCops:
  NewCops: enable
-  TargetRubyVersion: 2.6  
+  TargetRubyVersion: 2.7


any reason we're not in 3.0.3 land?

This was the version specified in the gemspec, and I think it's best if the gem can support as wide a range of Ruby versions as possible (albeit support for 2.7 will be dropped in 10 months)

corroded · 2022-05-24T07:32:22Z

lib/mt9/header_record.rb

+    field :filler109, 109, "52-160", :alphanumeric
+
+    field_value :filler5, SPACE * 5
+    field_value :filler109, SPACE * 109


lots of "magic numbers" here. I would try to name them here and make them constants. i.e. 109 could be SOMETHING_FILLER_COUNT / MAYBE_FILLER_PADDING or something so when we use them, it kinda sort of gives us context what it is (and that we should not be able to change it easily):

Suggested change

field_value :filler109, SPACE * 109

field_value :filler109, SPACE * MAYBE_FILLER_PADDING

chris-teague

Great work @zachingle! Just a couple of small suggestions / things to look into, but nothing that should block merging from my perspective - we can always raise a seperate PR if needed. 🔥

chris-teague · 2022-05-24T07:29:10Z

lib/mt9/validators/header_record_contract.rb

+
+      rule(:account_number).validate(:is_account_number?)
+
+      rule(:due_date) do


I'm thinking we should also have a check to see whether a valid due date has been supplied.

In that case, to simplify validation we could change due_date to receive a Date object instead of a string

chris-teague · 2022-05-24T07:31:27Z

lib/mt9/validators/header_record_contract.rb

+        required(:file_type).filled(:string, included_in?: MT9::Values::FILE_TYPES)
+        required(:account_number).filled(:string)
+        required(:due_date).filled(:string)
+        optional(:client_short_name).value(:string, size?: 0..20)


I'm wondering whether we want to make this required, even though this isn't strictly the case when looking at the docs? It'd be good to know whether it's a big no-no to send a file without a short name & add a little protection if so

Might be a @henripryde400 question to ask to the ASB guys

corroded · 2022-05-24T07:35:24Z

lib/mt9/validators/base_contract.rb

+  module Validators
+    class BaseContract < Dry::Validation::Contract
+      register_macro(:is_account_number?) do
+        key.failure("must be 15 or 16 numeric characters") unless /^(\d{15}|\d{16})$/.match(value)


Suggested change

key.failure("must be 15 or 16 numeric characters") unless /^(\d{15}|\d{16})$/.match(value)

key.failure("must be 15 or 16 numeric characters") unless /^(\d{15,16})$/.match(value)

nitpick, but shorter :) See: https://rubular.com/r/PAUYXnL7M2LDDv

corroded · 2022-05-24T07:36:07Z

lib/mt9/validators/header_record_contract.rb

+      rule(:account_number).validate(:is_account_number?)
+
+      rule(:due_date) do
+        key.failure("must be 6 or 8 numeric characters") unless /^(\d{6}|\d{8})$/.match(value)


Suggested change

key.failure("must be 6 or 8 numeric characters") unless /^(\d{6}|\d{8})$/.match(value)

key.failure("must be 6 or 8 numeric characters") unless /^(\d{6,8}$/.match(value)

This notation is used for a range (i.e 6 to 8 digits). Here we want either 6 or 8, but not 7 digits

corroded · 2022-05-24T07:37:04Z

lib/mt9/values.rb

+    RESERVED_BANK_CODES = %w[99].freeze
+
+    DIRECT_CREDIT = "12"
+    DIRECT_DEBIT = "20"


curious why they are strings and not integers?

Thought process here was that we don't perform any integer-like operations on these numbers and these are identifiers, just like the account number, and so are better represented as strings

corroded

Great work!

zachingle added 11 commits May 16, 2022 16:18

Add example MT9 file

d42619e

Add initial validator for header records

2cd34cc

Calm the cop

1bcde41

Remove comment

860fea8

Added sample direct debits file

013c5d5

Namespace validators under MT9 module

6d3a77d

Define individual constants for file types

3134987

Update TargetRubyVersion to 2.7

0301fc8

Move header record contract into seperate file

6ad1f19

Update ruby version in gemspec

f45fa5f

Add header record using Fixy gem

02f2207

zachingle requested review from chris-teague, gzzengwei and kieranjogrady May 20, 2022 02:25

gzzengwei reviewed May 20, 2022

View reviewed changes

zachingle added 6 commits May 20, 2022 15:35

Add specs

a2dfdf5

Calm the cop

f34f6e1

Add support for 16 digit account numbers. Move file to correct directory

c3e20e1

Move tests for validation/formatting to seperate spec

16ca73c

Move module and specs to correct file again

1d1271a

Change to ArgumentError

629c35f

zachingle requested review from corroded May 23, 2022 23:49

Merge branch 'header-record-validations' into zi/ZEP-1944-fixy-gem

8767d78

chris-teague reviewed May 24, 2022

View reviewed changes

Combine dry validation approach and clean up classes/specs

f79a8b8

zachingle changed the title ~~Add header record using Fixy gem~~ Add header record using Fixy and DryRB Validation gems May 24, 2022

Use full error messages

07a98a1

zachingle force-pushed the zi/ZEP-1944-fixy-gem branch from 0649bf3 to 07a98a1 Compare May 24, 2022 00:49

zachingle requested a review from gzzengwei May 24, 2022 03:31

zachingle marked this pull request as ready for review May 24, 2022 03:52

zachingle added 2 commits May 24, 2022 14:02

Allow 8 digit due dates

d983493

Use macro for account number

7fd5cd1

zachingle force-pushed the zi/ZEP-1944-fixy-gem branch from ff43a0f to 7fd5cd1 Compare May 24, 2022 05:36

corroded reviewed May 24, 2022

View reviewed changes

chris-teague approved these changes May 24, 2022

View reviewed changes

corroded reviewed May 24, 2022

View reviewed changes

corroded approved these changes May 24, 2022

View reviewed changes

SmileAndCompile merged commit ac725c6 into main May 24, 2022

zachingle mentioned this pull request May 27, 2022

ZEP-1944: PR Fixes #7

Merged

chrisixion deleted the zi/ZEP-1944-fixy-gem branch May 27, 2022 01:36

	field_value :filler109, SPACE * 109
	field_value :filler109, SPACE * MAYBE_FILLER_PADDING


		rule(:account_number).validate(:is_account_number?)

		rule(:due_date) do

	key.failure("must be 15 or 16 numeric characters") unless /^(\d{15}\|\d{16})$/.match(value)
	key.failure("must be 15 or 16 numeric characters") unless /^(\d{15,16})$/.match(value)

	key.failure("must be 6 or 8 numeric characters") unless /^(\d{6}\|\d{8})$/.match(value)
	key.failure("must be 6 or 8 numeric characters") unless /^(\d{6,8}$/.match(value)

Conversation

zachingle commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-teague left a comment

Choose a reason for hiding this comment

Uh oh!

zachingle commented May 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-teague left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corroded left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zachingle commented May 20, 2022 •

edited

Loading