10000 IAP Refresh Session Example. by jeffmendoza · Pull Request #1230 · GoogleCloudPlatform/python-docs-samples · GitHub
[go: up one dir, main page]
Skip to content

IAP Refresh Session Example. #1230

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
Merged
merged 3 commits into from
Nov 28, 2017
Merged

IAP Refresh Session Example. #1230

merged 3 commits into from
Nov 28, 2017

Conversation

jeffmendoza
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 22, 2017
@jeffmendoza
Copy link
Contributor Author

Updated, fixed lint.

@jeffmendoza
Copy link
Contributor Author

@tswast Tim, can you take a look at the frontend code? Thanks!

theacodes
theacodes suggested changes Nov 27, 2017
View reviewed changes
iap/refresh/main.py Outdated
import webapp2


class MainPage(webapp2.RequestHandler):
Copy link
Contributor

Choose a reason for hiding this comment

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

I strongly prefer all new samples for App Engine standard use Flask over Webapp2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I didn't switch from jinja, do I need to?

iap/refresh/main.py Outdated
nickname = None
logout_url = None
login_url = users.create_login_url('/')
template_values = {
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: please put a blank line between the end of a indented block and the following statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

iap/refresh/js/poll.js Outdated
setTimeout(function() {
var statusText = document.getElementById('status');
statusText.innerHTML = 'Polling';
var xhr = new XMLHttpRequest();
Copy link
Contributor

Choose a reason for hiding this comment

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

Use fetch? https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API

Copy link
Contributor

Choose a reason for hiding this comment

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

It has pretty wide support at this point https://caniuse.com/#feat=fetch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sound's like the fetch/promise polyfill won't be needed then, I'll switch it over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please check my promise usage, not sure if it is correct.

iap/refresh/js/poll.js Outdated
})();

// [START refresh_session]
var iap_session_refresh_window = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd strongly prefer if you scoped this inside of a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jon, can you give me an example? my jsfoo is lacking.

iap/refresh/js/poll.js
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Copy link
Contributor

Choose a reason for hiding this comment

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

'use strict'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

iap/refresh/js/poll.js Outdated
// limitations under the License.

(function poll() {
setTimeout(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer you give this function a name and invoke setTimeout separately, e.g.:

function pollForLoginState() {
    ...
}

setTimeout(pollForLoginState, 10000);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, changed to setInterval

iap/refresh/js/poll.js Outdated
// [START refresh_session]
var iap_session_refresh_window = null;

function session_refresh_clicked() {
Copy link
Contributor

Choose a reason for hiding this comment

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

google style for function names in js is lowerCamelCase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tswast
tswast requested changes Nov 27, 2017
View reviewed changes
iap/refresh/index.html Outdated
@@ -0,0 +1,13 @@
<html><body>
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the HTML style guide, this should have

<!DOCTYPE html>

The <html> and <body> tags are optional and should be removed.

Also, add <meta charset="utf-8"> and a <title> tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

iap/refresh/js/poll.js Outdated
poll();
};
xhr.send(null);
}, 10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a comment saying 10000 means 10 seconds.

Copy link
Contributor Author
17AE

Choose a reason for hiding this comment

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

Done.

iap/refresh/js/poll.js Outdated
else {
statusText.innerHTML = xhr.statusText;
}
poll();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like poll() is always called. Maybe you could use setInterval instead of setTimeout in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jeffmendoza
Copy link
Contributor Author

Comments addressed, PTAL.

@jeffmendoza jeffmendoza force-pushed the iap-refresh-sample branch 3 times, most recently from 5962357 to a4d8598 Compare November 28, 2017 18:41
tswast
tswast approved these changes Nov 28, 2017
View reviewed changes
Copy link
Contributor
@tswast tswast left a comment

Choose a reason for hiding this comment

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

LGTM for frontend stuff.

Yes, jinja2 is the library to use for templates with Flask.

theacodes
theacodes reviewed Nov 28, 2017
View reviewed changes
iap/refresh/main.py Outdated
}

template = jinja_environment.get_template('index.html')
return template.render(template_values)
Copy link
Contributor

Choose a reason for hiding this comment

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

fun fact, jinja is built-in to flask so you can just do flask.render_template('index.html', template_values).

theacodes
theacodes reviewed Nov 28, 2017
View reviewed changes
iap/refresh/main.py Outdated

import os

from flask import Flask
Copy link
Contributor

Choose a reason for hiding this comment

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

import modules, not members. please just import flask and use flask.Flask.

@theacodes theacodes merged commit 87f67e6 into GoogleCloudPlatform:master Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tswast tswast tswast approved these changes

@theacodes theacodes theacodes approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jeffmendoza @tswast @theacodes @googlebot
0