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

Fix to allow unassinged application selection for deploy image flow #2815

Merged

Conversation

jeff-phillips-18
Copy link
Member

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/dev-console Related to dev-console labels Sep 23, 2019
@andrewballantyne
Copy link
Contributor

Good tests and cleanup of the import schema.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2019
@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2019
@andrewballantyne
Copy link
Contributor

andrewballantyne commented Sep 24, 2019

During testing I noticed something.

image

Which makes sense (if not missing the *)...

However, when you don't have any applications you get:

image

Which shouldn't be required - as it's not intended to slow down the creation.

@andrewballantyne
Copy link
Contributor

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2019
@jeff-phillips-18
Copy link
Member Author

jeff-phillips-18 commented Sep 25, 2019

@andrewballantyne The application name will get auto-filled with a default value once the Git Repo URLl is entered. Until then it is left blank, but since the url value is required, it will not slow down the creation at all, though I see it could be a bit confusing.

However, IMO, the drop down should still be shown when there are no existing applications. I have logged a bug at https://jira.coreos.com/browse/ODC-1941

@jeff-phillips-18
Copy link
Member Author

/retest

@andrewballantyne
Copy link
Contributor

@andrewballantyne The application name will get auto-filled with a default value once the Git Repo URLl is entered. Until then it is left blank, but since the url value is required, it will not slow down the creation at all, though I see it could be a bit confusing.

However, IMO, the drop down should still be shown when there are no existing applications. I have logged a bug at https://jira.coreos.com/browse/ODC-1941

Slowing down the application flow in this case doesn't directly refer to completing this form itself. In this case it means "I do not want an application - how do I get that when I do my first import". With your change, there is no way to achieve this anymore (they would need to address this after creation). Yes, we do auto fill it (hopefully to the helpfulness of the user), but if they don't want our name and want to opt out, there is no direct way.

PM did ask for this feature directly https://jira.coreos.com/browse/ODC-1646 - so I don't think we can remove it.

I can see your point about ODC-1941 - we should probably have a cleaner way for the user to opt out of our naming and go with nothing. "Create or Unassign" being the dropdown options. When the work was done to remove the application dropdown, it was because the only option was create - which was misleading.

cc @serenamarie125 - if you have a moment, some UX input could be useful here

selectedKey: yup.string(),
selectedKey: yup.string().required('Required'),
name: yup.string().when('selectedKey', {
is: (selectedKey) => selectedKey !== UNASSIGNED_KEY,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the default for selectedKey UNASSIGNED_KEY?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is to set the name to not be required when the selectedKey is UNASSIGNED_KEY. The default is either an existing application or CREATE_APPLICATION_KEY

@jeff-phillips-18
Copy link
Member Author

jeff-phillips-18 commented Sep 25, 2019

I understand the issue better now. With this change, the application name is required. Since we do not show the application selection dropdown, there is no way to choose Unassigned and the user would need to clear out the application name field in order to specify not to use an application grouping. But, this would not allow the import since the application name is required...

I believe we should always show the application selection dropdown which would default to Create Application when there are no existing applications. This would fix ODC-1941 as well.

Will verify with UX and update this PR accordingly.

@christianvogt
Copy link
Contributor

@jeff-phillips-18 The UX was decided to not show the dropdown in the scenario when there are no applications. Although this was before the introduction of the unassigned option.
cc @serenamarie125 @parvathyvr

@jeff-phillips-18
Copy link
Member Author

Spoke with @serenamarie125, we do NOT what the dropdown when there are no existing applications. Reworking...

@gijohn
Copy link
Contributor

gijohn commented Sep 26, 2019

/retest

@andrewballantyne
Copy link
Contributor

@jeff-phillips-18 correct me if I am wrong, but I don't think the Application Name is needed when selecting an existing application? I don't recall seeing it and I can't think of a reason why we'd want it.

image

@@ -46,7 +46,7 @@ const ApplicationSelector: React.FC<ApplicationSelectorProps> = ({
const noApplicationsAvailable = _.isEmpty(applicationList);
setApplicationsAvailable(!noApplicationsAvailable);
if (noApplicationsAvailable) {
setFieldValue('application.selectedKey', CREATE_APPLICATION_KEY);
setFieldValue('application.selectedKey', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this change was needed because CREATE_APPLICATION_KEY is tied to showing the dropdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct.

@@ -85,9 +85,10 @@ const ApplicationSelector: React.FC<ApplicationSelectorProps> = ({
/>
</FormGroup>
)}
{(noProjectsAvailable || selectedKey.value === CREATE_APPLICATION_KEY) && (
{(noProjectsAvailable || selectedKey.value !== UNASSIGNED_KEY) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue with selecting an existing application and showing the Application Name field is tied to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe noProjectsAvailable || noApplicationsAvailable || selectedKey.value === CREATE_APPLICATION_KEY?

Not sure why a project check is needed :/ I must have not noticed that during the other review when this went in. Can't have applications without projects. (this logic is mirrored in the above condition for the dropdown)

Could go with noApplicationsAvailable || is CREATE_APPLICATION_KEY - but I am weary about changing that logic without knowing more about it.

selectedKey: yup.string().required('Required'),
selectedKey: yup.string(),
name: yup.string().when('selectedKey', {
is: (selectedKey) => selectedKey === CREATE_APPLICATION_KEY,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just do is: CREATE_APPLICATION_KEY now that you're doing straight equality.

@@ -85,9 +85,12 @@ const ApplicationSelector: React.FC<ApplicationSelectorProps> = ({
/>
</FormGroup>
)}
{(noProjectsAvailable || selectedKey.value === CREATE_APPLICATION_KEY) && (
{(noProjectsAvailable ||
!selectedKey.value ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm - I wonder if this is a bit vague to the reason why. It's essentially "no applications means we want the input", where as the above logic reads "no dropdown item selected means we want the input". It's a nit for sure, but I favour being explicit. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't seem to come up with anything more readable other than creating a function to do the check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Made the comment without seeing your PR updated - my suggestion is here:
#2815 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

The other choice could be to add another selectedKey value, like NO_APPLICATIONS_KEY

Copy link
Contributor

Choose a reason for hiding this comment

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

The other choice could be to add another selectedKey value, like NO_APPLICATIONS_KEY

I thought of that too - but I feel it's equally weird since it's not a selectable option itself. I think we should avoid making keys that are and are not used in the dropdown. Might just be unnecessarily confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed

@andrewballantyne
Copy link
Contributor

/lgtm
Thanks for bearing with me through the nits.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2019
@andrewballantyne
Copy link
Contributor

andrewballantyne commented Sep 27, 2019

/lgtm
@christianvogt @rohitkrai03 need an approval nevermind, one exists from earlier.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, jeff-phillips-18

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 2bfcf2e into openshift:master Sep 27, 2019
@spadgett spadgett added this to the v4.3 milestone Oct 4, 2019
@jeff-phillips-18 jeff-phillips-18 deleted the import-no-app branch November 19, 2019 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants