In CreateFromBuilder and OrderService wizards, correct error handling of ProjectNameTaken #550

dtaylor113 · 2017-11-06T14:53:55Z

Partial fix for console#1955

Immediate form validation error if project name already exists for user. Form validation error when 'Project Name already exists' error returned from server (project name not unique across cluster).
Form validation error cleared when user starts to type in new project name

spadgett · 2017-11-06T17:20:36Z

src/components/select-project/select-project.component.ts

@@ -4,6 +4,7 @@ export const selectProject: angular.IComponentOptions = {
  bindings: {
    selectedProject: '=',
    nameTaken: '<',
+    onNewProjectNameChanged: '&',


I don't adding a new callback is necessary... The embedding page can just watch selectedProject.metadata.name.

this.$scope.$watch('$ctrl.selectedProject.metadata.name', () => { this.ctrl.projectNameTaken = false; });

spadgett · 2017-11-06T17:22:41Z

src/components/select-project/selectProject.html

@@ -58,7 +58,8 @@ <h3 ng-if="$ctrl.canOnlyCreateProject()">Create Project</h3>
    ng-if="$ctrl.isNewProject()">
  <div class="form-group">
    <label for="name" class="control-label required">Project Name</label>
-    <div ng-class="{'has-error': ($ctrl.forms.createProjectForm.name.$error.pattern && $ctrl.forms.createProjectForm.name.$touched) || $ctrl.nameTaken}">
+    <div ng-class="{'has-error': ($ctrl.forms.createProjectForm.name.$error.pattern && $ctrl.forms.createProjectForm.name.$touched) ||
+                                 ($ctrl.nameTaken || $ctrl.forms.createProjectForm.name.$error.oscUnique)}">


The parens around the last two conditions are unnecessary.

<div ng-class="{'has-error': ($ctrl.forms.createProjectForm.name.$error.pattern && $ctrl.forms.createProjectForm.name.$touched) || $ctrl.nameTaken || $ctrl.forms.createProjectForm.name.$error.oscUnique}">

spadgett · 2017-11-06T17:24:45Z

src/components/create-from-builder/create-from-builder.controller.ts

@@ -106,6 +106,7 @@ export class CreateFromBuilderController implements angular.IController {
      onShow: this.showResults
    };
    this.ctrl.steps = [this.infoStep, this.configStep, this.bindStep, this.reviewStep];
+    this.ctrl.currentStep = "Information";


Do we need to set this everywhere? I'd think it would only be necessary in the error handler when we change steps back to Configuration.

We need to set it so that when the error happens, it navigates correctly, else the currentStep will always be "Configuration".

Is the "Create" button disabled until the user changes the name after an AlreadyExists error?

Yes

spadgett · 2017-11-06T17:27:16Z

Is the "Create" button disabled until the user changes the name after an AlreadyExists error?

spadgett · 2017-11-06T18:09:16Z

src/components/create-from-builder/create-from-builder.controller.ts

+      if (e.data.reason === 'AlreadyExists') {
+        this.ctrl.projectNameTaken = true;
+        this.ctrl.wizardDone = false;
+        this.ctrl.currentStep = "Configuration";


This does this result in any flash / flicker when it changes to the results step and back?

Yes it does, if that is an issue I can look into wiring up nextCallback() and return `false' to not navigate, not sure if that'll do the trick though.

I think that's what we do in the process-template-dialog.

yes, and then we use eventing to navigate to Results upon success, just more overhead than what I'm doing at the moment, but if we want to avoid the flicker...

If we do this, then there should be no need to set currentStep at all.

The problem is OrderService where the wizard steps are: Configure(create/sel proj) -> Bindings -> try to createProjIf neccesary() then createService(), -> Results. I was thinking Configure -> try to createProjIfNeccesary(), if error stay on Conifg page & show error, if success goto Bindings -> createService() -> Results. But it sounded like we didn't want to create proj. in Configure step, rather be able to test if proj name is unique cross-cluster -but there didn't seem to be a method to do this.

In general, I think there should be a "+" next to the project selector which would launch the Create Project dlg, then default the project selector to newly created project. The "Create Project" shouldn't be in the select project dropdown. Perhaps a change for 3.8?

Not sure what to do in the meantime. IMO jumping back to Configure step on projNameAlreadyTaken is a barely noticeable flicker and is a step in the right direction until we address the overall design in 3.8.

@beanh66, @ncameronbritt UX thoughts? -thanks! :-)

@dtaylor113 What you have works for me for 3.7. I had forgotten about the bind step in this dialog.

Ok, then I believe I have addressed all review comments. Should I create an issue about the proposed "+" next to the select project dropdown?

spadgett · 2017-11-07T19:40:33Z

src/components/select-project/select-project.controller.ts

@@ -114,11 +113,6 @@ export class SelectProjectController implements angular.IController {
    }
  }

-  public onNewProjectNameChange() {
-    this.ctrl.nameTaken = false;
-    this.ctrl.forms.createProjectForm.name.$setValidity('nameTaken', !this.ctrl.nameTaken);


Is there a reason not to set form validity? I suspect at least one of the dialogs relies on this.

… of ProjectNameTaken

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 6, 2017

dtaylor113 mentioned this pull request Nov 6, 2017

Project 'nameTaken' not being set properly in controllers openshift/origin-web-console#1955

Closed

spadgett requested changes Nov 6, 2017

View reviewed changes

spadgett reviewed Nov 6, 2017

View reviewed changes

dtaylor113 force-pushed the projectNameTaken branch from 86c1198 to 19700be Compare November 7, 2017 17:14

spadgett requested changes Nov 7, 2017

View reviewed changes

In CreateFromBuilder and OrderService wizards, correct error handling…

9391600

… of ProjectNameTaken

dtaylor113 force-pushed the projectNameTaken branch from 19700be to 9391600 Compare November 7, 2017 19:43

spadgett approved these changes Nov 7, 2017

View reviewed changes

spadgett merged commit 69571f0 into openshift:master Nov 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In CreateFromBuilder and OrderService wizards, correct error handling of ProjectNameTaken #550

In CreateFromBuilder and OrderService wizards, correct error handling of ProjectNameTaken #550

dtaylor113 commented Nov 6, 2017

spadgett Nov 6, 2017

dtaylor113 Nov 7, 2017

spadgett Nov 6, 2017

dtaylor113 Nov 7, 2017

spadgett Nov 6, 2017

dtaylor113 Nov 6, 2017

spadgett commented Nov 6, 2017

spadgett Nov 6, 2017

dtaylor113 Nov 6, 2017

spadgett Nov 6, 2017

dtaylor113 Nov 6, 2017

jeff-phillips-18 Nov 6, 2017

dtaylor113 Nov 7, 2017 •

edited

spadgett Nov 7, 2017

dtaylor113 Nov 7, 2017

spadgett Nov 7, 2017

In CreateFromBuilder and OrderService wizards, correct error handling of ProjectNameTaken #550

In CreateFromBuilder and OrderService wizards, correct error handling of ProjectNameTaken #550

Conversation

dtaylor113 commented Nov 6, 2017

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

spadgett commented Nov 6, 2017

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

dtaylor113 Nov 7, 2017 • edited

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

dtaylor113 Nov 7, 2017 •

edited