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

Refresh user information at start-up #670

Merged
merged 11 commits into from
Feb 16, 2017
Merged

Refresh user information at start-up #670

merged 11 commits into from
Feb 16, 2017

Conversation

bergie
Copy link
Member

@bergie bergie commented Feb 16, 2017

Follow-up to #660. Will fix #632

Also adds a much clearer way for users to grant the app access to their GitHub repositories:

screenshot 2017-02-16 at 12 40 54

@bergie bergie changed the title WIP: Refresh user information at start-up Refresh user information at start-up Feb 16, 2017
@bergie bergie requested a review from jonnor February 16, 2017 11:48
if (!this.user) {
return;
}
if (!this.user.plan || this.user.plan.type === 'free') {
Copy link
Member

Choose a reason for hiding this comment

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

Why more logic in Polymer elements? We should be removing logic, not adding.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is UI-side logic so I found this to be the right place for it.

Copy link
Member

Choose a reason for hiding this comment

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

Storing askForScope in the widget introduces (more) statefulness. Is there at least a less stateful way to represent this transformation of user info to these scopes?
And is this the scopes we have asked for, or will ask for? The naming is not clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are scopes we offer to the user as options to allow. The "grant access to X" buttons are populated from this.

@@ -74,90 +52,6 @@
detached: function () {
document.getElementById('container').classList.remove('blur');
},
checkFlowhubUser: function (callback) {
Copy link
Member

Choose a reason for hiding this comment

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

Good, less logic in view elements.


unless navigator.onLine
# When offline we can't refresh
out.pass.send data
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider sending the not-logged-in and not-online cases to different ports. That way the semantics of the component is visible from outside, in the graph.

Copy link
Member Author

Choose a reason for hiding this comment

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

This component doesn't really care whether logged in or not. The actual thing the component does should be pretty well reflected in the ports:

  • User information was fine as-is (whether logged in or not)
  • User information was invalid (non-working OAuth token etc)
  • User information was updated from IdP

Copy link
Member

Choose a reason for hiding this comment

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

Ok

@jonnor jonnor merged commit 74b91d4 into master Feb 16, 2017
@jonnor jonnor deleted the refresh_user branch February 16, 2017 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fetching runtime data with existing login fails silently
2 participants