Updated Python3 Version #69
Conversation
|
|
||
| import pytz | ||
| from google.appengine.ext import ndb | ||
| from google.cloud import ndb |
There was a problem hiding this comment.
Very neat, I didn't know about this! This ndb library is not mentioned at all in the datastore migration documentation. Apparently it uses Cloud Firestore internally. Have you thought about how we're going to transfer the data? Or would we start with a new, fresh datastore?
Otherwise we could try out python-ndb, which should allow us to access the data we're currently using.
There was a problem hiding this comment.
I may be wrong but it appears the python-ndb is the same as google-cloud-ndb. The docs actually show to pip install google-cloud-ndb here.
Also I believe google migrated all legacy datastore db's to use firestore in datastore mode. Shootie was upgraded using the same libraries so we wouldn't need to transfer any data and will still use the same datastore.
There was a problem hiding this comment.
Great! That's perfect then!
models/employee.py
Outdated
|
|
||
| def get_gravatar(self): | ||
| """Creates gravatar URL from email address.""" | ||
| m = hashlib.md5() | ||
| m.update(self.user.email()) | ||
| m.update((self.username + '@' + config.DOMAIN).encode()) |
There was a problem hiding this comment.
You could make email a property, like full_name is already.
util/csrf.py
Outdated
| token = session.get('_csrf_token') | ||
| payload = request.form.get('_csrf_token') | ||
| # request form of csrf token is returned as a string of a bytestring, slicing to just retrieve the bytestring | ||
| # Not sure how else to do this :/ |
There was a problem hiding this comment.
That's because it's already set like that:
<input name="_csrf_token" type="hidden" value="b'f6c75f2e3c191a162335e5ad99b8089d'">This is a bug in the app related to Python 3, we'll need to fix how we set the csrf token.
There was a problem hiding this comment.
In line 27, we have this code:
session['_csrf_token'] = binascii.hexlify(os.urandom(16))hexlify returrns a bytestring. The fix is to decode it:
session['_csrf_token'] = binascii.hexlify(os.urandom(16)).decode('ascii')There was a problem hiding this comment.
I wasn't sure if the csrf token needed to be of type bytes. Will fix!
util/decorators.py
Outdated
| if not users.get_current_user(): | ||
| return redirect(users.create_login_url(request.url)) | ||
| username = oidc.user_getfield('email').split('@')[0] | ||
| isAdmin = Employee.query( |
There was a problem hiding this comment.
How about we reuse util.auth.is_admin() here?
There was a problem hiding this comment.
Completely forgot about that function! Will fix.
| @@ -4,7 +4,7 @@ | |||
| # and then run "tox" from this directory. | |||
|
|
|||
| [tox] | |||
| envlist = py27 | |||
| envlist = py37 | |||
There was a problem hiding this comment.
Your travis build is failing because it can't find Python 3.7. You'll need to update .travis.yml as well.
| @@ -199,5 +199,6 @@ def _send_love(recipient_key, message, sender_key, secret): | |||
| if not secret: | |||
| logic.event.add_event( | |||
| logic.event.LOVESENT, | |||
| '/tasks/subscribers/notify', | |||
There was a problem hiding this comment.
Accidentally deleted this line before my original PR.
|
I spent some time today looking into what we can do to get the tests to run. The main problem is that NoseGAE is not compatible with Python 3 - both the code itself as well as the fact that it expects and interfaces with the Python 2 App Engine runtime. The project hasn't had any activity for the past three years, we might want to migrate off of it. @koskorya, could you add me as a contributor to your fork? That way I can collaborate on it, I might have time tomorrow to work on this. |
|
added you @sjaensch! |
|
My progress today:
|
Didn't touch the precommit or tests. This code is also running here. Hopefully we can team up to update the tests and any other things that can be improved!