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

Login logout #56

Merged
merged 21 commits into from
Oct 21, 2018
Merged

Login logout #56

merged 21 commits into from
Oct 21, 2018

Conversation

squishykid
Copy link
Member

Implementing some login and logout functionality.

The login info is not checked yet.

@squishykid squishykid requested review from a team as code owners October 11, 2018 04:54
@squishykid squishykid added the work in progress Work in Progress label Oct 11, 2018
# Conflicts:
#	appService.Json
#	templates/appService.Json
#	templates/gulpfile.js
#	test/biz/test_manage_restaurant.py
#	web/friend.py
#	web/static/js/appService.Json
#	web/static/js/gulpfile.js
#	web/static/vendor/jquery-easing/jquery.easing.compatibility.js
#	web/static/vendor/jquery-easing/jquery.easing.js
#	web/static/vendor/jquery-easing/jquery.easing.min.js
#	web/templates/LICENSE
#	web/templates/blank.html
#	web/templates/gulpfile.js
#	web/templates/login.html
# Conflicts:
#	routes.py
#	test/biz/test_manage_restaurant.py
#	web/__init__.py
#	web/login.py
#	web/templates/404.html
#	web/templates/blank.html
#	web/templates/breadcrumb.html
#	web/templates/footer.html
#	web/templates/header.html
#	web/templates/index.html
#	web/templates/login.html
#	web/templates/nav.html
#	web/templates/register.html
#	web/templates/sidebar.html
#	web/templates/tables.html
Copy link
Member

@chrisstime chrisstime left a comment

Choose a reason for hiding this comment

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

I'm going to see if I can help out with this a little bit after work but ideally, we should be using this against the staff.py model to authenticate the user. Right?

@@ -0,0 +1,18 @@
<!-- Logout Modal-->
<div class="modal fade" id="logoutModal" tabindex="-1" role="dialog" aria-labelledby="exampleModalLabel" aria-hidden="true">
Copy link
Member

Choose a reason for hiding this comment

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

Delete this file, it's not being used and looks like it's just doubled up.

return redirect(target)


@LOGIN_BLUEPRINT.route("/", methods=['GET', 'POST'])
Copy link
Member

Choose a reason for hiding this comment

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

in my head this feels like this should be '/login' and '/' should redirect back to '/login' if user isn't logged in

Copy link
Member Author

Choose a reason for hiding this comment

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

it gets mounted in the router as /login/. it's just '/' relative to the blueprint. you can see in __init__.py how the blueprint is mounted under /login.

return render_template('login.html', page_title=page_title)
if request.method == 'POST':
session['username'] = request.form['email']
return redirect_back('index.index')
Copy link
Member

Choose a reason for hiding this comment

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

Why does it look to me like it's only posting the email for login..?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now there's no actual authentication going on. This would be the work for the next pull request, to connect the staff table with this login code.

@squishykid
Copy link
Member Author

squishykid commented Oct 15, 2018 via email

@squishykid
Copy link
Member Author

So i'm just thinking about this a little more. I think that combining this with the staff.py class is out of scope of this pull request. We can make another pull request which implements that.

chrisstime
chrisstime previously approved these changes Oct 20, 2018
Copy link
Member

@chrisstime chrisstime left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@chrisstime
Copy link
Member

Happy to do staff.py in another PR

@chrisstime chrisstime added ready to merge ready to merge and removed work in progress Work in Progress labels Oct 20, 2018
@arosspope
Copy link
Member

arosspope commented Oct 20, 2018

What's the .env file for?

@arosspope
Copy link
Member

Also, in this PR are you actually doing the authentication on the backend (i.e. hitting the db). Or was that going to be in another PR?

@squishykid
Copy link
Member Author

squishykid commented Oct 20, 2018

Gonna double check the necessity of the env file. And there is a comment where we should implement the auth check - which will be another PR

@squishykid
Copy link
Member Author

@arosspope @chrisstime this is ready to go. If you approve just do the squash&merge.

@squishykid squishykid mentioned this pull request Oct 21, 2018
3 tasks
@arosspope arosspope merged commit 673cd1c into master Oct 21, 2018
@arosspope arosspope deleted the login_logout branch October 21, 2018 02:29
@squishykid squishykid self-assigned this Oct 21, 2018
@squishykid squishykid added this to the End of Build 4 milestone Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants