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

Public beta updates for the signup/login section in the deployment flow. #2541

Merged
merged 6 commits into from Mar 2, 2017

Conversation

@kadams54
Copy link
Contributor

kadams54 commented Mar 1, 2017

Fixes #2497.

@kadams54 kadams54 added this to the dauntless dax milestone Mar 1, 2017
@jujugui

This comment has been minimized.

Copy link
Contributor

jujugui commented Mar 1, 2017

Refer to this link for build results (access rights to CI server needed):
http://ci-cge.jujugui.org:8080//job/juju-gui/6867/

@kadams54

This comment has been minimized.

Copy link
Contributor Author

kadams54 commented Mar 1, 2017

QA

Note: Requires guiproxy. Grab it with go get github.com/frankban/guiproxy if you don't have it.

  1. make clean-gui fast-babel gui run
  2. $GOPATH/bin/guiproxy -controller jimm.jujucharms.com:443 -config 'gisf: true, jujuEnvUUID:""'
  3. Go to http://localhost:8042/new.
  4. Add an application to the model, then deploy.
  5. The deploy panel that comes up should resemble: Login design

Note that there are a few differences: our "Sign up" link is a different color (black) than what's in the design (blue) and we don't have anything have text in place of the red line in the design.

Verify that logging in works and returns you to the normal deployment flow. We seem to have a bug with the model name field, but I'll open a separate issue for that. Verify that the normal deployment flow still works.

@spencerbygraves

This comment has been minimized.

Copy link

spencerbygraves commented Mar 1, 2017

The text under the red line above should read:

“You will need to sign in with an Ubuntu One account to deploy your model with Juju-as-a-Service.”

@jujugui

This comment has been minimized.

Copy link
Contributor

jujugui commented Mar 1, 2017

Refer to this link for build results (access rights to CI server needed):
http://ci-cge.jujugui.org:8080//job/juju-gui/6869/

component. It accept a props param, which can be used to change defaults (on
required props) or provide values (on optional props).
*/
const createDeploymentFlow = (props = {}) => {

This comment has been minimized.

Copy link
@huwshimi

huwshimi Mar 2, 2017

Member

Hmmm... I'm not sure about this, I think the scoping will introduce a bunch of new problems. Happy to be convinced otherwise... maybe we should hold off on big changes like this to our tests until we have a discussion about testing next week.

This comment has been minimized.

Copy link
@frankban

frankban Mar 2, 2017

Member

This does not seem to me a big change in how we are testing the component. It seems that we are abstracting out the instantiation of the component instead, and this allows for avoiding a lot of repetition in the tests. It seems a good idea to me.

Copy link
Member

frankban left a comment

The code looks good, thanks!
Will QA asap.

component. It accept a props param, which can be used to change defaults (on
required props) or provide values (on optional props).
*/
const createDeploymentFlow = (props = {}) => {

This comment has been minimized.

Copy link
@frankban

frankban Mar 2, 2017

Member

This does not seem to me a big change in how we are testing the component. It seems that we are abstracting out the instantiation of the component instead, and this allows for avoiding a lot of repetition in the tests. It seems a good idea to me.

@@ -95,14 +96,29 @@ YUI.add('usso-login-link', function() {
},

/**
If the component has child elements, they are used as the content for the
link; otherwise the provided default string will be used.

This comment has been minimized.

Copy link
@frankban

frankban Mar 2, 2017

Member

Unnecessary newline.

@frankban

This comment has been minimized.

Copy link
Member

frankban commented Mar 2, 2017

QA:
logging in works as expected.
The only problem is when you initially load the GUI as a logged in user, for instance with guiproxy when visiting http://localhost:8042. That page properly redirects me to my profile page at http://localhost:8042/u/frankban. But then, when I clean the "create new" button from there, the spinner is displayed and the GUI blocks.

@kadams54 kadams54 force-pushed the kadams54:initial-signup-flow branch from 6505229 to 6ac2aa0 Mar 2, 2017
@jujugui

This comment has been minimized.

Copy link
Contributor

jujugui commented Mar 2, 2017

Refer to this link for build results (access rights to CI server needed):
http://ci-cge.jujugui.org:8080//job/juju-gui/6875/

@jaycee

This comment has been minimized.

Copy link
Member

jaycee commented Mar 2, 2017

The issue that @frankban saw is an issue fixed in develop.

QA OK.

@hatched
hatched approved these changes Mar 2, 2017
disabled={!this._deploymentAllowed()}
type="positive"
title={deployTitle} />
if (this.props.isLegacyJuju || this.state.loggedIn) {

This comment has been minimized.

Copy link
@hatched

hatched Mar 2, 2017

Member

nice improvement

@hatched

This comment has been minimized.

Copy link
Member

hatched commented Mar 2, 2017

:shipit:

@jujugui

This comment has been minimized.

Copy link
Contributor

jujugui commented Mar 2, 2017

Status: merge request accepted. Url: http://ci-gce.jujugui.org:8080/job/juju-gui-merge

@jujugui jujugui merged commit 1f06f6d into juju:develop Mar 2, 2017
1 check passed
1 check passed
default Build finished.
Details
@kadams54 kadams54 deleted the kadams54:initial-signup-flow branch Mar 2, 2017
@anthonydillon

This comment has been minimized.

Copy link
Member

anthonydillon commented Mar 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.