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

Public registration & oAuth2 \o/ #1436

Merged
merged 13 commits into from
Oct 6, 2015
Merged

Public registration & oAuth2 \o/ #1436

merged 13 commits into from
Oct 6, 2015

Conversation

nicosomb
Copy link
Member

  • public registration
  • remove WSSE implementation
  • add oAuth2 implementation

@nicosomb
Copy link
Member Author

Sorry, I created a big PR.

@nicosomb
Copy link
Member Author

Todo:

  • a nice form for registration
  • we can't access to /api/doc and I don't know why
  • tests

@j0k3r
Copy link
Member

j0k3r commented Sep 29, 2015

Wow, good job.

@nicosomb
Copy link
Member Author

I need to talk with you about passwords.

return;
}

$config = new Config($event->getUser());
Copy link
Member

Choose a reason for hiding this comment

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

Could you put a comment on that class to explain that it is called after a user is registered?
Or maybe renamed it to RegistratioConfirmedListener to be more explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed.

@nicosomb
Copy link
Member Author

I added tests for public registration. Do you think it's enough for this feature? And for this PR?

For example, I didn't write tests for email validation.

@j0k3r
Copy link
Member

j0k3r commented Sep 29, 2015

You can add unit test for RegistrationConfirmedListener.
And you can run phpunit --coverage-html build/coverage, wait 30 minutes (...) and see where you still need some tests.

Well, validation email are already tested in FosUser I guess.

@nicosomb
Copy link
Member Author

Added a test on RegistrationConfirmedListener.

@nicosomb nicosomb changed the title [WIP] Public registration & oAuth2 \o/ Public registration & oAuth2 \o/ Sep 29, 2015
@nicosomb nicosomb mentioned this pull request Sep 29, 2015
@j0k3r
Copy link
Member

j0k3r commented Sep 30, 2015

Thanks for the functional test but I was talking about a unit test (which doesn't load all Symfony) but only the RegistrationConfirmedListener class. Like in UsernameRssTokenConverterTest.

@nicosomb
Copy link
Member Author

nicosomb commented Oct 2, 2015

oops, sorry for the container :-(

@nicosomb
Copy link
Member Author

nicosomb commented Oct 2, 2015

and thank you for the test 👍 !
This PR is ready for review !

@j0k3r
Copy link
Member

j0k3r commented Oct 2, 2015

Next step: remove what we've done about forgot password, login, etc .. to use FOS

@nicosomb
Copy link
Member Author

nicosomb commented Oct 2, 2015

Planned. And also create a UserBundle.

@nicosomb
Copy link
Member Author

nicosomb commented Oct 3, 2015

Ooooh I don't know what I did :( all these commits ...

@j0k3r
Copy link
Member

j0k3r commented Oct 3, 2015

I think I've fixed the mess :)
I guess you killed your repo by rebasing the branch against the v2, maybe in a wrong way.

I've cherry-picked your 2 commits and fixed few things.
Also, now with the UserBundle, testing with coverage is like ... taking ages ... 😕

PHPUnit 4.8.10 by Sebastian Bergmann and contributors.

................................................................. 65 / 86 ( 75%)
.....................

Time: 25.59 minutes, Memory: 287.50Mb

OK (86 tests, 337 assertions)

Generating code coverage report in HTML format ... done

- remove unecessary routing for UserBundle
- remove unused form type
@nicosomb
Copy link
Member Author

nicosomb commented Oct 4, 2015

thank you for your help!

@j0k3r
Copy link
Member

j0k3r commented Oct 5, 2015

There are still some work to be done :)

  • baggy registration page
  • a better forgot password page
  • a functional test on registration (because we use a, even small, custom form type)

@nicosomb
Copy link
Member Author

nicosomb commented Oct 5, 2015

graby registration page

What is it?

I will work on the other tasks soon.

@j0k3r
Copy link
Member

j0k3r commented Oct 5, 2015

Woops, I said graby but I wanted to say baggy 😄

@tcitworld
Copy link
Member

Why is the name field a textarea ? Be sure to make it optional.

@nicosomb
Copy link
Member Author

nicosomb commented Oct 5, 2015

It's already deleted in next commits ;-)

@nicosomb
Copy link
Member Author

nicosomb commented Oct 5, 2015

a functional test on registration (because we use a, even small, custom form type)

I removed RegistrationType.php. Is the test still necessary?

I have to work on baggy form, but material is now OK :-)

@j0k3r
Copy link
Member

j0k3r commented Oct 5, 2015

I tried to remove RegistrationType but got an error in the test suite ..
Oh maybe because I didn't update config.yml :)

@nicosomb
Copy link
Member Author

nicosomb commented Oct 5, 2015

RegistrationType was removed in my last commit and test suite seems to be green.

@nicosomb
Copy link
Member Author

nicosomb commented Oct 5, 2015

Ready for last review.

@j0k3r
Copy link
Member

j0k3r commented Oct 6, 2015

Should be ok for now, we'll see later if we need improvment.
Good job 👍

j0k3r added a commit that referenced this pull request Oct 6, 2015
Public registration & oAuth2 \o/
@j0k3r j0k3r merged commit 16dabc3 into v2 Oct 6, 2015
@j0k3r j0k3r deleted the v2-register branch October 6, 2015 07:19
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.

None yet

3 participants