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

Add email validation to RequestResetPassword #621

Merged
merged 27 commits into from
Feb 3, 2020

Conversation

Amr3zzat
Copy link
Contributor

Add email Validation to RequestResetPassword
#618

@Amr3zzat Amr3zzat requested a review from a team as a code owner January 10, 2020 12:25
@Amr3zzat Amr3zzat force-pushed the Add-Reset-password-Validation branch from b25b7ba to 0f65d97 Compare January 10, 2020 12:29
@mamazu mamazu force-pushed the Add-Reset-password-Validation branch from f5522ac to 133f48c Compare January 11, 2020 12:11
@mamazu
Copy link
Member

mamazu commented Jan 11, 2020

I have fixed the codestyle in your branch and added the information about the change in behavior in the change logs and rebased it onto the master so the pipeline should be green now.

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up. Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "Add-Reset-password-validation" to update your local branch. Feel free to ask for assistance when you get stuck. 👍

@Amr3zzat
Copy link
Contributor Author

@mamazu Thanks for your review I will check them, I have updated my local branch, I got stuck for some time, but It worked thanks

@Amr3zzat Amr3zzat requested review from mamazu and removed request for a team January 11, 2020 21:33
Copy link
Member

@mamazu mamazu left a comment

Choose a reason for hiding this comment

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

The pull request looks good. However the pipeline still fails with codestyle issues. You can run composer fix to fix them.

@Amr3zzat Amr3zzat requested review from mamazu and lchrusciel January 21, 2020 20:03
Copy link
Member

@mamazu mamazu left a comment

Choose a reason for hiding this comment

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

Looks good to me, if you can fix the tests. But I definitely want to wait until @lchrusciel also approves.

Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work. My comments are mostly small CS related stuff.

src/Controller/Customer/RequestPasswordResettingAction.php Outdated Show resolved Hide resolved
src/Controller/Customer/RequestPasswordResettingAction.php Outdated Show resolved Hide resolved
src/Exception/UserNotFoundException.php Outdated Show resolved Hide resolved
src/Exception/UserNotFoundException.php Outdated Show resolved Hide resolved
src/Exception/UserNotFoundException.php Outdated Show resolved Hide resolved
Amr3zzat and others added 7 commits January 22, 2020 23:16
Co-Authored-By: Łukasz Chruściel <lchrusciel@gmail.com>
Co-Authored-By: Łukasz Chruściel <lchrusciel@gmail.com>
Co-Authored-By: Łukasz Chruściel <lchrusciel@gmail.com>
Co-Authored-By: Łukasz Chruściel <lchrusciel@gmail.com>
Co-Authored-By: Łukasz Chruściel <lchrusciel@gmail.com>
@Amr3zzat
Copy link
Contributor Author

@mamazu @lchrusciel Thanks for your review, it was so helpful, I have fixed the specs.

@mamazu
Copy link
Member

mamazu commented Jan 26, 2020

If you can fix the tests then it should be good to go.

@Amr3zzat
Copy link
Contributor Author

@mamazu Many Thanks for your help, I fixed them

@mamazu mamazu merged commit 4ade871 into Sylius:master Feb 3, 2020
@mamazu
Copy link
Member

mamazu commented Feb 3, 2020

Thanks, Amr! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants