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

Project 'nameTaken' not being set properly in controllers #1955

Closed
dtaylor113 opened this issue Aug 21, 2017 · 12 comments
Closed

Project 'nameTaken' not being set properly in controllers #1955

dtaylor113 opened this issue Aug 21, 2017 · 12 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/P2
Milestone

Comments

@dtaylor113
Copy link
Contributor

HTML files for process-template, from-file, and deploy-image all have:

<select-project ... name-taken="$ctrl.projectNameTaken"></select-project>

However, the corresponding controllers do not set the variable projectNameTaken, which should be set from a failure from the server after attempting to create a project which already has the name.

Some comments from @spadgett:

"Normal users can't see all projects in the cluster, so you don't have a complete list of project names (used by osc-unique). We still need to handle errors from the server creating the project because the name is already in use by someone else."

https://github.com/openshift/origin-web-console/search?utf8=%E2%9C%93&q=name-taken&type=

This is also true for order-service-controller in origin-web-catalog.

@dtaylor113
Copy link
Contributor Author

Should be resolved similar to createSecret, or deployImage

@spadgett spadgett added kind/bug Categorizes issue or PR as related to a bug. priority/P2 labels Aug 21, 2017
@spadgett spadgett added this to the 3.7.0 milestone Sep 13, 2017
@dtaylor113
Copy link
Contributor Author

dtaylor113 commented Oct 31, 2017

Just tested the different areas.

deployImage, processTemplate, and fromFile throw a toast notification when a project name is already taken:
image

orderService and createFromBuilder shows an inline error message, this should probably be changed to throw a toast instead.
image

The <select-project> component uses osc-unique to trigger form validation errors immediately upon entering in an existing project name, and the parent controllers (deployImage, fromFile, etc..) which attempt to create the project, handle 'name already taken' errors returned from the server. And, as this issue initially indicated, the $ctrl.projectNameTaken variable isn't in or used in these parent controllers.

So I don't think we need the name-taken binding in

<select-project ... name-taken="$ctrl.projectNameTaken">`

I think initially this component was relying on parent controllers to indicate whether the project name was taken, but <select-project> does the immediate validation via osc-unique and the parent controllers handle 'nameTaken' errors returned from the server.

IMO, what needs to be done for this issue is:

  1. Remove 'name-taken' binding from select-project in console and catalog.
  2. Make toast notifications for project name already taken for orderService and createFromBuilder in catalog.

@spadgett, does this seem reasonable? -thanks

@spadgett
Copy link
Member

@dtaylor113 I think the biggest usability issue is entering a bunch of information into one of these forms and then seeing error on the results page with no way to recover. It's particularly a problem for environments like online that have thousands of projects and you can't know what's already used. I'd like to try to fix at least that aspect of this issue for 3.7.

cc @jwforres

@spadgett
Copy link
Member

I do like that name taken highlights the specific field in error and forces you to change that before resubmitting the form.

@dtaylor113
Copy link
Contributor Author

So for deployImage, processTemplate, and fromFile which do the toast notification you never leave the wizard page, so the form is still filled in and available. We need to switch orderService and createFromBuilder to use the toast notifications in the same way.

@dtaylor113
Copy link
Contributor Author

Hi @spadgett, I think we need to decide if we need both a toast notification and an form validation error. Currently, it only throws a toast notification for 'projNameTaken' error is returned from server, and it only shows the form invalid for osc-unique.
I've wired up deployImage for when it gets a 'projNameTaken' from server to do both the existing toast notification, and through proper use of the <select-project>'s name-taken attribute, the form now also gets the form invalid look...but do we need both?

image

And should I/we be hiding the help text if there is an error?

@jwforres
Copy link
Member

jwforres commented Nov 1, 2017 via email

@dtaylor113
Copy link
Contributor Author

@jwforres, ok that's easy enough change. NotificationService has a 'skipToast' attribute. I'm trying to find an example in the code but do you happen to know what the standard is for these errors? Do we hide the help text when error? Seems strange to have the help text shown in red, seems like we should just show the error msg. -thanks

@jwforres
Copy link
Member

jwforres commented Nov 1, 2017

@dtaylor113 the request failure shouldn't go to NotificationService at all when we get this particular error code back

The help text and the error text need to be in separate nodes, the help text should maintain is normal color. Like in this example from build-config.html
separate help text

@dtaylor113
Copy link
Contributor Author

So we don't want 'project name already exists' failures recorded in the Notification Drawer?

@jwforres
Copy link
Member

jwforres commented Nov 1, 2017 via email

@dtaylor113
Copy link
Contributor Author

Fix for CreateFromBuilder and OrderService wizards up in Catalog PR: openshift/origin-web-catalog#550.
deployImage, processTemplate, and fromFile in Console to follow.

openshift-merge-robot added a commit that referenced this issue Nov 7, 2017
Automatic merge from submit-queue.

Bump origin-web-catalog to 0.0.64

https://github.com/openshift/origin-web-catalog/releases/tag/v0.0.64

Partial fix for #1955

/kind bug
/assign @dtaylor113 @jwforres
openshift-merge-robot added a commit that referenced this issue Nov 8, 2017
Automatic merge from submit-queue.

Correct ProjectNameTaken error handling in various add-to-projects wizards

Updated deployImage, processTemplate, and fromFile wizards.

Fixes #1955 

![image](https://user-images.githubusercontent.com/12733153/32521149-6e627f7c-c3e0-11e7-81d2-e00d5137c5eb.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/P2
Projects
None yet
Development

No branches or pull requests

3 participants