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
Bug #10951 [Account] fix sync username with customer email #11048
Conversation
src/Sylius/Bundle/CoreBundle/EventListener/DefaultUsernameORMListener.php
Show resolved
Hide resolved
fc453bc
to
e9b92f9
Compare
src/Sylius/Bundle/CoreBundle/EventListener/DefaultUsernameORMListener.php
Outdated
Show resolved
Hide resolved
b74c0c1
to
d3efcfe
Compare
Hey @hatem20. The build is red and it seems, that it may be related to your PR. Can you rebase to the newest master and fix the build if it will not resolve all issues? |
@lchrusciel failed again, and not related to PR |
if applied, this commit will -make sync behaviour works on both customer and shop user, resulting in username will never by setting username value whenever shopuser or customer are inserted or updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Hatem,
it is been a while, but let's finally merge it ;) If you will find a moment, can you, please add a spec for new behavior as well?
Thanks, Hatem! 🥇 |
Hey Hatem. Can you append your work with spec class? Your PR is not working as expected: https://master.demo.sylius.com/admin/customers/ however it seems, that what you have proposed is a useful fix. I would prefer to have at least spec for that. AFAIR, we are not allowing for shop user email update, so you won't be able to test it with behat. If you will not cover it with spec, I will have to revert it before the nearest tag. |
@lchrusciel sure will add spec thanks, should I open new PR? |
yes, please :) |
@lchrusciel I added spec as requested, but the PR is working as expected, |
It seems, that you've spotted my mistake! I didn't up merge changes to the master, my fault. Nonetheless, the spec is a good addition to your fix. Thanks a lot for your work ;) |
…atem20) This PR was merged into the 1.7 branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | 1.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | #10951 | License | MIT I added spec as request in #11048 <!-- - Bug fixes must be submitted against the 1.7 branch (the lowest possible) - Features and deprecations must be submitted against the master branch - Make sure that the correct base branch is set To be sure you are not breaking any Backward Compatibilities, check the documentation: https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html --> Commits ------- c5e245f [Account] Add spec to sync username with customer email
If applied, this commit will
-make sync behavior works on both customer and shop user,
resulting in username will never by setting username value whenever
shop user or customer is inserted or updated.
I could have used the solution proposed here
#10951 (comment)
but it might be better if the username is always in sync with customer email