tddwizard / magento2-fixtures

Fixture library for Magento 2 integration tests by @schmengler (@integer-net)
http://tddwizard.com/
MIT License
143 stars 28 forks source link

Improvements for AddressBuilder #45

Closed bartoszkubicki closed 4 years ago

bartoszkubicki commented 4 years ago

Hello @schmengler ,

I would like to propose improvent for address builder. In Poland, where I live and work if we build store for B2B clients it is really common to use vat id heavily. Lately we have to add validation for vat id and make it required. I had problems with integration tests, beacause by using builder I have no access to address object and I couldn't set vat id, so I had to wrap builder with mine.

I would like to add vat id with some dummy value here and add function allowing setting this value manually. What do you think? If you agree I will prepare PR.

mam08ixo commented 4 years ago

I would rather not change the factory method but only add the setter. I don't think adding a VAT ID by default is useful. With the additional setter, you can easily add the property to faker-generated addresses like this:

AddressBuilder::anAddress(null, 'pl_PL')->withVatId('PL123456')

When creating the PR, then please also add tests to assert that the VAT ID has the expected value on the built address. Thank you!

schmengler commented 4 years ago

Agree @mam08ixo , a VAT ID by default could become a problem for B2C shops for example. But I can imagine an additional factory method like aCompanyAddress() with a $vatId parameter. The parameter could have a default value of null and take a dummy from Faker in that case. Would that be useful for you @bartoszkubicki ?

bartoszkubicki commented 4 years ago

Solution proposed by @mam08ixo is really good I could go this way. But @schmengler solution is even better, because its much more expressive.

schmengler commented 4 years ago

@bartoszkubicki A PR would be great

schmengler commented 4 years ago

Thanks again @bartoszkubicki !