Fixes numerous issues, read the body. #52

NoMan2000 · 2017-08-20T20:38:15Z

Adds requirements.txt file.
Fixed the ability to add a new restaurant menu item.
Fixed the broken OAuth code. There's an attempt to use bytes as a string twice.
Sets up variables in the environment in a .env file rather than hard-coding them in and exposing student credentials.
Removed the public section, protected inside of if statements that use the login_session inside of jinja.
Removed the unused database.
Added gitignore to the directory.
Added .python-version file so that pyenv can pick it up and so it can be easily seen.
Fixed formatting for Pep8. Use Flake8 instead, it’s better.
Removed the check for if Google Auth already exists. This isn’t tied to the session credentials, which means someone can be infinitely unable to use their Google login until they delete all their local storage.
Pushed all code into a single credentials file so that adding new credentials is easy.
Updated the base configuration so that it’s easier to add custom scripts and css to it.
Updated the login page to use extend the base class.
Updated the login page to use the credentials coming in from the server rather than hard-coding them in, so that there’s no mismatch or need to update multiple places.
Added CSS to style the main page a bit better.
Normalized CSS formatting.
Added html tag to show which files a user can edit.
Updated to Bootstrap 3.3.7
Setup the secret on the app so that the user can run flask run to boot up the application
Added readme to give an idea of how to start the application.
Fixes numerous stylistic problems: Unused imports, pep8 coding violations, improper casing, etc. Python uses snake casing, not camel casing.
Fixed issue where cancelling out still acted as if the result was successful. The attribute “type=button” needs to be added, as the specification for all buttons that are not given an explicit type is to be “submit”.
Removed redundant type declarations on buttons.
Set any values that are part of a dictionary to use an iterator to set the values rather than attempting to use a cascading set of if conditions to modify them.

- Adds requirements.txt file - Fixed the ability to add a new restaurant menu item. - Fixed the broken OAuth code. - Sets up variables in the environment in a .env file rather than hard-coding them in and exposing student credentials. - Removed the public section, protected inside of if statements that use the login_session inside of jinja. - Removed the unused database - Added gitignore to the directory. - Added .python-version file so that pyenv can pick it up and so it can be easily seen. - Fixed formatting for Pep8. Use Flake8 instead, it’s better. - Removed the check for if Google Auth already exists. This isn’t tied to the session credentials, which means someone can be infinitely unable to use their Google login until they delete all their local storage. - Pushed all code into a single credentials file so that adding new credentials is easy. - Updated the base configuration so that it’s easier to add custom scripts and css to it. - Updated the login page to use extend the base class. - Updated the login page to use the credentials coming in from the server rather than hard-coding them in, so that there’s no mismatch or need to update multiple places. - Added CSS to style the main page a bit better. Also removed what looks like a css error. - Added html tag to show which files a user can edit. - Updated to Bootstrap 3.3.7 - Setup the secret on the app so that the user can run `flask run` to boot up the application - Added readme to give an idea of how to start the application. - Fixes numerous stylistic problems: Unused imports, pep8 coding violations, improper casing, etc. - Fixed issue where cancelling out still acted as if the result was successful. The attribute “type=button” needs to be added, as the specification for all buttons that are not given an explicit type is to be “submit”. - Removed redundant type declarations on buttons.

NoMan2000

This code is a hot mess. Numerous Python violations, bad coding practices, and it just plain doesn't work in numerous places.

Unless the goal is to royally anger all the students who try to run this, this should be fixed.

And why aren't there any tests for this code?

NoMan2000 · 2017-08-20T20:40:05Z

.python-version

@@ -0,0 +1 @@
+3.6.1


If the code is going to be incompatible between Python 2 and 3, then you should create a separate branch for each of those and remove the master branch. The code I've rewritten should be cross-compatible.

NoMan2000 · 2017-08-20T20:41:08Z

Lesson4/step2/.env.example

@@ -0,0 +1,7 @@
+google_client_id=


Students should not be storing credentials in JSON files, they should learn the ability to follow the 12 factor app and keep any configuration credentials stored in environment files so each deployment environment has its own configuration settings without having to constantly change between git commits and leave credentials exposed.

NoMan2000 · 2017-08-20T20:42:18Z

Lesson4/step2/README.md

@@ -0,0 +1,28 @@
+# Restaurants and Menus


Seriously, no readme file? Each directory should have a readme file that explains all of the setup.

NoMan2000 · 2017-08-20T20:43:13Z

Lesson4/step2/client_secrets.json

@@ -1 +0,0 @@
-{"web":{"auth_uri":"https://accounts.google.com/o/oauth2/auth","client_secret":"YOUR_CLIENT_SECRET_GOES_HERE","token_uri":"https://accounts.google.com/o/oauth2/token","client_email":"13140951618-15nik769cellkubaqnjk5facdib2dh4d@developer.gserviceaccount.com","redirect_uris":["https://localhost:5000/callback","http://localhost:5000/callback"],"client_x509_cert_url":"https://www.googleapis.com/robot/v1/metadata/x509/13140951618-15nik769cellkubaqnjk5facdib2dh4d@developer.gserviceaccount.com","client_id":"YOUR_CLIENT_ID_GOES_HERE.apps.googleusercontent.com","auth_provider_x509_cert_url":"https://www.googleapis.com/oauth2/v1/certs","javascript_origins":["http://localhost","https://udacity-oauth.herokuapp.com","http://udacity-oauth.herokuapp.com","http://localhost:5000"]}}


Don't ever store any form of credentials in plain text on a public repo. 👎 Don't even do it on a private repo.

NoMan2000 · 2017-08-20T20:44:48Z

Lesson4/step2/lotsofmenus.py


-engine = create_engine('sqlite:///restaurantmenuwithusers.db')


Why do we keep duplicating the engine call? This means that if the user wants to perform a migration, they have to update the calls in multiple places to which database engine it's using. I could probably go further and remove all these redundant calls, but this is the big one since this affects multiple files.

NoMan2000 · 2017-08-20T21:12:11Z

Lesson4/step2/project.py

    restaurant = session.query(Restaurant).filter_by(id=restaurant_id).one()
    if login_session['user_id'] != restaurant.user_id:
-        return "<script>function myFunction() {alert('You are not authorized to edit menu items to this restaurant. Please create your own restaurant in order to edit items.');}</script><body onload='myFunction()''>"


I'm not going to fix it, but there should be a standard errors page when a user goes to an invalid location. JavaScript alerts are an ugly hack.

NoMan2000 · 2017-08-20T21:12:55Z

Lesson4/step2/project.py

    if request.method == 'POST':
-        if request.form['name']:


In what circumstance would the request NOT pass on the values in the form?

NoMan2000 · 2017-08-20T21:13:38Z

Lesson4/step2/project.py



 # Disconnect based on provider
 @app.route('/disconnect')
 def disconnect():
    if 'provider' in login_session:
-        if login_session['provider'] == 'google':


Why should anyone care who the provider is? If we're disconnecting, the point is to remove all session variables regardless of where they came from.

NoMan2000 · 2017-08-20T21:16:12Z

Lesson4/step2/project.py

-            editedRestaurant.name = request.form['name']
-            flash('Restaurant Successfully Edited %s' % editedRestaurant.name)
-            return redirect(url_for('showRestaurants'))
+        if request.form['name'] != edited_restaurant.name:


If they didn't change anything, why show the flash message that they did?

NoMan2000 · 2017-08-20T21:17:11Z

Lesson4/step2/project.py

    if 'username' not in login_session:
        return redirect('/login')
    restaurant = session.query(Restaurant).filter_by(id=restaurant_id).one()
    if login_session['user_id'] != restaurant.user_id:
-        return "<script>function myFunction() {alert('You are not authorized to add menu items to this restaurant. Please create your own restaurant in order to add items.');}</script><body onload='myFunction()''>"
-        if request.method == 'POST':


Improper nesting. This code never executes.

And make everything pep8 compliant.

- Adds in Faker data.

NoMan2000 added 2 commits August 19, 2017 11:37

For python 3

8628e46

NoMan2000 commented Aug 20, 2017

View reviewed changes

markm1198 and others added 6 commits September 8, 2017 19:24

Did not specify that a cascade delete was needed. It is. Who knew?

cb89b56

More modifications

f11668b

Reviewer is not that bright.

cf3dddb

Updated file.

ea5d707

Decorate the login function

f5dcc41

And make everything pep8 compliant.

Wrap errors in try/except block

cbe1f02

- Adds in Faker data.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes numerous issues, read the body. #52

Fixes numerous issues, read the body. #52

NoMan2000 commented Aug 20, 2017

NoMan2000 left a comment

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

NoMan2000 Aug 20, 2017

		@@ -1 +0,0 @@
		{"web":{"auth_uri":"https://accounts.google.com/o/oauth2/auth","client_secret":"YOUR_CLIENT_SECRET_GOES_HERE","token_uri":"https://accounts.google.com/o/oauth2/token","client_email":"13140951618-15nik769cellkubaqnjk5facdib2dh4d@developer.gserviceaccount.com","redirect_uris":["https://localhost:5000/callback","http://localhost:5000/callback"],"client_x509_cert_url":"https://www.googleapis.com/robot/v1/metadata/x509/13140951618-15nik769cellkubaqnjk5facdib2dh4d@developer.gserviceaccount.com","client_id":"YOUR_CLIENT_ID_GOES_HERE.apps.googleusercontent.com","auth_provider_x509_cert_url":"https://www.googleapis.com/oauth2/v1/certs","javascript_origins":["http://localhost","https://udacity-oauth.herokuapp.com","http://udacity-oauth.herokuapp.com","http://localhost:5000"]}}


		engine = create_engine('sqlite:///restaurantmenuwithusers.db')

		@@ -0,0 +1 @@
		3.6.1

Fixes numerous issues, read the body. #52

Are you sure you want to change the base?

Fixes numerous issues, read the body. #52

Conversation

NoMan2000 commented Aug 20, 2017

NoMan2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment