Cast AccountNumber string to integer #158

samuel02 · 2018-06-04T18:48:19Z

According to the documentation at https://developer.fortnox.se/documentation/resources/customers/
the field SalesAccount on the Customer is a 4 digit string.

d-Pixie · 2018-06-08T07:50:28Z

lib/fortnox/api/models/customer.rb


        # SalesAccount	Sales account of the customer, 4 digits
-        attribute :sales_account, Types::AccountNumber
+        attribute :sales_account, Types::Sized::String[4]


This is not ok as is as it would allow the account NUMBER to be set to "abcd" etc. If the API actually barfs if we send it a number here we should do some casting instead.
Is this something you have experienced problems with or was it an attempt to prevent problems? Also note that we do NOT reflect the Fortnox API spec directly in our API due to theirs being full of errors, conflicts and inconsistencies. Instead we try to normalise the API as much as possible, returning actual Ruby Date objects for date and time, always representing numeric types as numbers, using enums in place of strings for countries etc.

samuel02 · 2018-06-14T18:21:32Z

@d-Pixie @ehannes I updated this PR to instead cast it from a string.

ehannes

Nice! Please have a look my comments. Feel ask if something is unclear or if I have misunderstood something.

ehannes · 2018-06-14T19:45:15Z

spec/fortnox/api/types/account_number_spec.rb

    it { is_expected.to be_nil }
  end

-  context 'when AccountNumber created with empty string' do


@samuel02 Why dropping this case? Is it not needed anymore?

It got complicated :) The problem is that the constructor Dry::Types::Coercions::Form.to_int will return nil in the case of an empty string. And since the type is declared as optional that's ok and it won't raise anything. What do you think? Should I write a custom constructor that handles that case as well?

Well, it being complicated is hardly a reason to not test for it. In fact it is a very good reason TO test for it! 😄

Sure :) My thinking was just that it doesn't maybe need to be that strict. The consequence of the new code would be that an empty string would end up as a nil value which was already permitted. So in practice it probably doesn't matter that much. But sure :) I pushed another implementation. Unfortunately it's not enough with the constructor you provided since Integer doesn't implement #empty?, if we'd have ActiveSupport we could have used Object#blank? but I didn't want to pull that in. So I ended up writing my own. WDYT?

ehannes · 2018-06-14T19:46:59Z

spec/fortnox/api/types/account_number_spec.rb

-
  context 'when AccountNumber created with valid number' do
-    include_examples 'equals input', 1234
+    subject { klass['1234'] }


1234 should still be a valid number, right? We want to test both string and integer input. So we should probably add a when AccountNumber created with valid string case here instead of changing this integer to string.

ehannes · 2018-06-14T19:50:02Z

spec/fortnox/api/types/account_number_spec.rb

  end

  context 'when AccountNumber created with a too large number' do
-    include_examples 'raises ConstraintError', 10_000


Same here. We should probably keep these as integers. We only need to test the casting once.

ehannes · 2018-06-14T19:50:12Z

spec/fortnox/api/types/account_number_spec.rb

  end

  context 'when AccountNumber created with a negative number' do
-    include_examples 'raises ConstraintError', -1


Same here. We should probably keep these as integers. We only need to test the casting once.

d-Pixie · 2018-06-15T07:40:13Z

spec/fortnox/api/types/account_number_spec.rb

    it { is_expected.to be_nil }
  end

-  context 'when AccountNumber created with empty string' do


Well, it being complicated is hardly a reason to not test for it. In fact it is a very good reason TO test for it! 😄

d-Pixie · 2018-06-15T07:55:02Z

lib/fortnox/api/types.rb

      AccountNumber = Strict::Int
                      .constrained(gteq: 0, lteq: 9999)
                      .optional
+                      .constructor(Dry::Types::Coercions::Form.method(:to_int))


I like the thought behind this. You can probably achieve the same result by changing Strict::Int to Coercible::Int at the top of the declaration. I'm assuming this doesn't work for the empty string case though.

To correctly deal with nil and empty string you can do something like this:

AccountNumber = Strict::Int .constrained(gteq: 0, lteq: 9999) .optional .constructor { |v| Integer(v) unless v.nil? or v.empty? }

ehannes

Now we're close :)

ehannes · 2018-06-18T07:06:18Z

lib/fortnox/api/types.rb

                      .constrained(gteq: 0, lteq: 9999)
                      .optional
+                      .constructor do |input|
+                        unless input.nil? || input.to_s.empty?


Are these cases tested?

ehannes · 2018-06-18T07:07:27Z

lib/fortnox/api/types.rb

      AccountNumber = Strict::Int
                      .constrained(gteq: 0, lteq: 9999)
                      .optional
+                      .constructor do |input|


This constructor is now 9 lines. Can we create a separate method for this? I'm pretty sure Rubocop will complain about this :)

Look ma, one-liner ...

.constructor do |input| input.to_s.empty? ? nil : Integer(input.to_s) end

Optimizing the logic for brevity and skipping the rescue. No one should send random user input directly at this and if they do that is their problem. We are not end user facing, we are developer facing and can expect that the input has already been sanitised. So allow for reasonable things - like nil and string/symbol versions - where it makes sense, but do not try to defend from all bad inputs.

ehannes · 2018-06-18T07:08:07Z

lib/fortnox/api/types.rb

+                          input
+                        end
+                      rescue ArgumentError
+                        input


Is this case tested?

Not all valid/invalid cases are tested. We could test for everything but right now, with my suggested changes, we do test the two main cases: Things that respond to to_s and has a valid value will convert, the rest will cause a Dry types constraint error...

d-Pixie · 2018-06-18T07:26:51Z

spec/fortnox/api/types/account_number_spec.rb

    include_examples 'raises ConstraintError', -1
  end
+
+  context 'when AccountNumber created with an invalid string' do


Firstly this should be describe Fortnox::API::Types::AccountNumber surely? The spec is named as specifically for that anyway :) That will also mean that we can skip the "when AccountNumber ..." for all contexts and you will have just context 'created with an invalid string' do for example.

Basically this test could be anything that we don't explicitly allow, so make it with invalid input and just make sure we get a Dry types error for now.

Yes, it should be describe Fortnox::API::Types::AccountNumber. My bad!

d-Pixie · 2018-06-18T07:29:10Z

spec/fortnox/api/types/account_number_spec.rb

+  context 'when AccountNumber created with valid string' do
+    subject { klass['1234'] }
+
+    it 'casts it to a number' do


We don't really care HOW it does something here, just that we get the correct result. So this could be:

context 'created with valid string' do subject { klass['1234'] } it { is_expected.to eq 1234 } end

Utilizing the readability of RSpecs one line syntax.

Actually if we consider how we do things we can change it to 'created with valid value that responds to to_s', since that is actually what we allow.

d-Pixie requested changes Jun 8, 2018

View reviewed changes

samuel02 force-pushed the correct-type-for-customer-sales-account branch from 97c686c to 3f8f142 Compare June 14, 2018 17:47

Cast AccountNumber string to integer

d6c099b

samuel02 force-pushed the correct-type-for-customer-sales-account branch from 3f8f142 to d6c099b Compare June 14, 2018 18:20

samuel02 changed the title ~~Set correct type for sales_account on Customer~~ Cast AccountNumber string to integer Jun 14, 2018

ehannes requested changes Jun 14, 2018

View reviewed changes

samuel02 force-pushed the correct-type-for-customer-sales-account branch 4 times, most recently from b931c20 to 26042da Compare June 15, 2018 06:50

Improve tests for AccountNumber

26042da

d-Pixie requested changes Jun 15, 2018

View reviewed changes

Raise in case of empty string

51bbce1

ehannes requested changes Jun 18, 2018

View reviewed changes

d-Pixie reviewed Jun 18, 2018

View reviewed changes

ehannes force-pushed the development branch 2 times, most recently from e19d08d to bd64a28 Compare February 17, 2025 07:01

Cast AccountNumber string to integer #158

Are you sure you want to change the base?

Cast AccountNumber string to integer #158

Uh oh!

Conversation

samuel02 commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuel02 commented Jun 14, 2018

Uh oh!

ehannes 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!

ehannes 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!

d-Pixie Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

d-Pixie Jun 18, 2018 •

edited

Loading