Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a customer ID to the user #147

Closed
wants to merge 2 commits into from
Closed

Adding a customer ID to the user #147

wants to merge 2 commits into from

Conversation

jjanvier
Copy link
Contributor

@jjanvier jjanvier commented Jun 7, 2013

This PR refers to #116.
I did not have many feedbacks but I try anyway ;)

The customer ID is made with : 4 letters of last name + 2 letters of first name + random number. The customer ID is 10 characters long.

@umpirsky
Copy link
Contributor

umpirsky commented Jun 7, 2013

So, this is one more ID? More human friendly.

I am trying to get the purpose of having this in default implementation.
On invoice there is usually full name or company name if it is a company.

Is there some other use case except for invoices?

@jjanvier
Copy link
Contributor Author

jjanvier commented Jun 7, 2013

This ID could be used for invoicing and given by the user when he contacts the support of the shop. Nothing more you're right

@jjanvier
Copy link
Contributor Author

jjanvier commented Jun 7, 2013

It can also be easier to retrieve a client in the BO with this kind of ID

public function findUserByCustomerId($customerId)
{
return $this->findUserBy(array('customerId' => $customerId));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While FOSUser handles it differently, I believe that such methods should live in repository - let's make this one private as it's used only in manager and shouldn't be public api. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

@pjedrzejewski
Copy link
Member

Sounds like good addition to me! I'll be happy to merge when rebased + my comments and if nobody has something against it, let me hear your thoughts! Thanks!

@jjanvier
Copy link
Contributor Author

FYO @pjedrzejewski I did not want to do this PR this way. My first approach was to use FOSUserEvents::REGISTRATION_SUCCESS to update the customer ID (so cleaner !!) But this is not available in the FOS User version that Sylius uses. Could we consider updating ?

@pjedrzejewski
Copy link
Member

Oh yes this sounds much better! And I agree we should definitely update. :)

@jjanvier
Copy link
Contributor Author

OK, so I'll close this one and open a new one to update FOS User and set the customer ID ;)

@winzou
Copy link
Contributor

winzou commented Jun 12, 2013

@pjedrzejewski just for you to know, tomorrow is my birthday so if you want to push the update on that day... :D

@jjanvier
Copy link
Contributor Author

Closed in favor of #165

@jjanvier jjanvier closed this Jun 13, 2013
@jjanvier jjanvier deleted the customer-id branch July 8, 2013 19:50
pamil pushed a commit to pamil/Sylius that referenced this pull request Mar 21, 2016
Added support for Configuration Blocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants