Differentiate individual and enterprise on customers#14294
Differentiate individual and enterprise on customers#14294pacodelaluna wants to merge 8 commits into
Conversation
9171071 to
5fffb5a
Compare
9ec7e68 to
74980c4
Compare
74980c4 to
f11746e
Compare
pacodelaluna
left a comment
There was a problem hiding this comment.
I finished a first iteration on this topic, I put some comments, there must be some adjustments. I will add screenshots below.
| enum :customer_type, { | ||
| individual: "individual", | ||
| enterprise: "enterprise" | ||
| }, default: "individual" |
There was a problem hiding this comment.
Using an enum for our purpose, it seems a good fit.
|
|
||
| attr_accessor :gateway_recurring_payment_client_secret, :gateway_shop_id | ||
|
|
||
| attribute :enterprise_charges_sales_tax, :boolean, default: false |
There was a problem hiding this comment.
The enterprise_charges_sales_tax field is handled with a checkbox input on the form, so the presence validation is an issue, as the checkbox can be empty. I choose the approach of giving it a default value. Tell me if you prefer an other approach.
| = @order.customer.email | ||
| - if @order&.customer&.enterprise_acn.present? | ||
| %br | ||
| = @order.customer.enterprise_acn |
There was a problem hiding this comment.
I didn't have mockups, so I put the enterprise_acn at the end of the customer details, tell me if it has to be in an other position.
| code { SecureRandom.base64(150) } | ||
| user | ||
| bill_address { create(:address) } | ||
| customer_type { "individual" } |
There was a problem hiding this comment.
We have a default value now, but I kept this here, as a kind of documentation. I can remove it if it does not make sense.
| expect(presenter.enterprise_name).to be_nil | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This class was not tested, so I added them, but I am wondering if they are really useful.
| expect(json['data']['attributes']['email']).to eq(customer.email) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Same here, not sure these tests are much important, I added them a bit by reflex.
There was a problem hiding this comment.
Thanks, I guess that most of this is already covered by spec/requests/api/v1/customers_spec.rb.
|
The failing check seems to be a flaky test, not related to my changes. |
|
It makes me think that we may need to adjust enterprise data in our Anonymization task, it looks like we are missing some parts on this side. I will take a look after this. |
rioug
left a comment
There was a problem hiding this comment.
Looks good, thanks for adding the extra tests !
There was a problem hiding this comment.
Great job, this looks mostly good.
I have a small suggested change to simplify the code, and I suggest using DB enums but would be happy if you don't have time to try it.
In reviewing this, I also needed to consider the bigger picture and added some comments on the issue. I'm not a fan of the field name enterprise_acn.
But I'm not exactly sure of the best option, so am interested to hear other ideas. If "acn" is agreed, I will approve it :)
| class AddEnterpriseRelatedFieldsToCustomers < ActiveRecord::Migration[7.1] | ||
| def change | ||
| change_table :customers, bulk: true do |t| | ||
| t.string :customer_type, default: "individual", null: false |
There was a problem hiding this comment.
This will store a string for every record. We could instead make use of database-level enums which would be more efficient:
https://guides.rubyonrails.org/active_record_postgresql.html#enumerated-types
Would you be willing to try this?
There was a problem hiding this comment.
Sure, I will implement it.
| } | ||
| validates :customer_type, | ||
| presence: true, | ||
| inclusion: { in: customer_types.keys } |
There was a problem hiding this comment.
± It looks like you don't need to create this validation, you can just enable it on the enum definition above with validate: true
(https://stackoverflow.com/a/77025739/421243)
There was a problem hiding this comment.
I tried this method initially, but I faced an issue, I don't remember exactly which one... I will try again and spot it if it pops again.
| expect(json['data']['attributes']['email']).to eq(customer.email) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Thanks, I guess that most of this is already covered by spec/requests/api/v1/customers_spec.rb.
|
@dacook Thanks for your review! About About the |
|
Thanks Paco. I'm still not sure of the best name for the field, but I don't want to hold things up further so I'm happy to approve it as-is (once you have tried the other changes). |


What? Why?
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.