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.

Sign up for GitHub

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

Jump to bottom

LoginController only for login flow #747

Merged
merged 21 commits into from
Nov 29, 2015
Merged

LoginController only for login flow #747

merged 21 commits into from
Nov 29, 2015

Conversation

slaveek
Copy link
Contributor

@slaveek slaveek commented Nov 9, 2015

This pull request contains changes related to #549 issue.
Sorry for a small typo in branch name ;)

slaveek and others added 18 commits October 25, 2015 11:40
@slaveek
Merge pull request #2 from panique/develop
9ef38da 
Develop update from head fork
Create UserController
ec0756c
Create RegisterController
33865fd
Move register and verify view files to view/register
60149a2
Remove register and verify view files from view/login directory
41f82c2
Move register and user action methods to their new controllers
35d1e12
Rename view/register/register.php to index.php
68e7b00
Changed View->render and Redirect::to()
8d6c6b3
Move view user action files to view/user directory
3991e31
Fixed logout if CSRF not valid
3683f0a 
Logout action should be do in other way. Repeat this 3 lines every time
we need valid CSRF, will be confusing.
Rename showProfile.php to index.php
314e245
Fixed showCaptcha
17cca66
Changed form actions in user views
c084b99
Change form action in register views
94f8473
Fixed links in right menu
fa2cc4c
Fixed typo
8904ac2
Move Auth::checkAuthentication() to constructor
1c3faf9 
All pages (user actions) in UserController are visible for logged-in
users
Fixed register link
90d6f2b
@panique
Copy link
Owner

panique commented Nov 11, 2015

Wow, thanks, this looks really good! :) Please gimme some days for testing!

@slaveek
Copy link
Contributor Author

slaveek commented Nov 12, 2015

No problem! ;)

geozak
geozak reviewed Nov 20, 2015
View reviewed changes
application/controller/RegisterController.php
* Register page action
* POST-request after form submit
*/
public function register_action()
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be better as index_action() to fit the naming pattern used for action-action_post_handler theme used throughout this framework.

Corresponding change would be needed here: https://github.com/panique/huge/pull/747/files?diff=unified#diff-5ac2d5faa50e65202dd471632e2632f2R11 (application/view/register/index.php L11)

@geozak
Copy link
Contributor

geozak commented Nov 20, 2015

Major changes missed:

  1. application/config/config.development.php L149: Need to change "login/verify" to "register/verify".

Minor changes missed:

  1. (application/view/login/showProfile.php ? application/view/user/index.php) L2: Forgot to change "LoginController/showProfile" to "UserController/index".
  2. (application/view/login/editUsername.php ? application/view/user/editUsername.php) L2: Forgot to change "LoginController/editUsername" to "UserController/editUsername".
  3. README.md L378: Need to change "application/view/login/editUsername.php" to "application/view/user/editUsername.php"
  4. (application/view/login/editUserEmail.php → application/view/user/editUserEmail.php) L2: Forgot to change "LoginController/editUserEmail" to "UserController/editUserEmail"
  5. (application/view/login/changeUserRole.php → application/view/user/changeUserRole.php) L2: Forgot to change "LoginController/changeUserRole" to "UserController/changeUserRole"
  6. README.md L363: Need to change "/login/changeUserRole" to "/user/changeUserRole"
  7. application/view/index/index.php L10: Need to change "/login/register" to "/register/index" or other example of choice.
  8. (application/view/login/changePassword.php → application/view/user/changePassword.php) L2: Forgot to change "LoginController/changePassword" to "UserController/changePassword"
  9. (application/view/login/register.php → application/view/register/index.php) L33: Forgot to change "YOURURL/login/showcaptcha" to "YOURURL/register/showcaptcha"

Potential changes to discuss:

  1. Where should the reset password functionality exist? Login, Register, User, or elsewhere.
  2. In application/view/_templates/header.php should the list items from line 51 to 68 be using checkForActiveControllerAndAction instead of checkForActiveController?

Summary:

There are a few small quickly fixable errors that I have listed above but otherwise this pull request is solid.
I have reviewed every changed line in the in the files changed section. And every method that was deleted has a corresponding addition.
I searched the whole repository, and every code reference to a deleted/moved line/method/file has been updated except for the instances I mentioned above.
None of these changes change how things flow further than the indented of breaking up the login, user, and register components into their own controls.

I hope this analysis helps get this request accepted faster.

@panique
Copy link
Owner

panique commented Nov 20, 2015

Thanks, I'll have a look on this on the weekend

@slaveek
Copy link
Contributor Author

slaveek commented Nov 20, 2015

Major and Minor changes missed
Agree!
All thinks that @geozak described MUST be definitely fixed.

Potential changes to discuss
I'd like extend @geozak description:
3. Where should the Captcha functionality exist? Register or it's own controller or in core directory?

Captcha can be used in other functionalities not register user only.

geozak added a commit to geozak/huge that referenced this pull request Nov 20, 2015
@geozak
Made the major and minor changes I (geozak) commented about in paniqu…
c7ed5f5 
…e/huge panique#747
@geozak
Copy link
Contributor

geozak commented Nov 20, 2015

@slaveek The captcha functionality is probably okay in the register controller because captchas as I've seen captcha have only been used in while registering user accounts.

I made the major and minor on a clean branch that I pulled slaveek/huge:login-cotroller into but I don't see slaveek/huge:login-cotroller on the list of forks to make to make a pull request to.
Here is a link to the branch https://github.com/geozak/huge/tree/login-controller-fixes
I didn't want to submit it straight to panique/huge until this main one get resolved.

@slaveek
Copy link
Contributor Author

slaveek commented Nov 21, 2015

OK! I'll fix it and I'll add more commits in this branch.

@panique
Copy link
Owner

panique commented Nov 29, 2015

Thanks a lot, looks really good! :)

panique added a commit that referenced this pull request Nov 29, 2015
@panique
Merge pull request #747 from slaveek/login-cotroller
d2c076e 
LoginController only for login flow
@panique panique merged commit d2c076e into panique:develop Nov 29, 2015
@panique panique mentioned this pull request Nov 29, 2015
Closed
@slaveek slaveek deleted the login-cotroller branch December 19, 2015 16:30
npuichigoB added a commit to npuichigoB/panique2 that referenced this pull request Feb 2, 2022
@npuichigoB
Fixed major and minor changes missed
4775046 
Bugs found by @geozak see:
panique/huge#747 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@slaveek @panique @geozak