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.

Sign up for GitHub

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

Jump to bottom

Bug 1797652: Fix crash on Edit of applications #4207

Merged
merged 1 commit into from Feb 8, 2020
Merged

Bug 1797652: Fix crash on Edit of applications #4207

merged 1 commit into from Feb 8, 2020

Conversation

jeff-phillips-18
Copy link
Member

@jeff-phillips-18 jeff-phillips-18 commented Feb 5, 2020 •
edited

Fixes:
https://issues.redhat.com/browse/ODC-2932

Analysis / Root cause:
The kind field for the given object is optional and if not set an invalid navigation location was being derived. Also, k8s services should not have the Edit action, only workloads should have this option.

Solution Description:
If the kind field for the object is not set, get the value from the given model.
Remove the k8s service model from the kinds list that the Edit item supports.
It was also determined that Edit is confusing given the other options in the kebab menu. Added the name of the object being edited to the Edit label (now Edit ${resource name}).

Included is a change to the node labels to truncate the middle text to be consistent with other instances of name truncation throughout the console UI.

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

Sample Screenshots:
image

image

cc @openshift/team-devconsole-ux

@openshift-ci-robot
Copy link
Contributor

@jeff-phillips-18: This pull request references Bugzilla bug 1797652, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1797652: Fix crash on Edit of applications

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/dev-console Related to dev-console labels Feb 5, 2020
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 5, 2020
@jeff-phillips-18
Copy link
Member Author

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 5, 2020
@openshift-ci-robot
Copy link
Contributor

@jeff-phillips-18: This pull request references Bugzilla bug 1797652, which is valid.

In response to this:

Bug 1797652: Fix crash on Edit of applications

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

christianvogt
christianvogt reviewed Feb 6, 2020
View reviewed changes
frontend/packages/dev-console/src/utils/truncate-utils.ts Outdated
};

export const shouldTruncate = (text, options = {}): boolean => {
const { length, minTruncateChars } = Object.assign({}, DEFAULT_OPTIONS, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I prefer the es6 spread syntax:

Suggested change
const { length, minTruncateChars } = Object.assign({}, DEFAULT_OPTIONS, options);
const { length, minTruncateChars } = {...DEFAULT_OPTIONS, ...options};

@christianvogt
Copy link
Contributor

So much better with the middle truncation.

@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 Feb 6, 2020
@jeff-phillips-18
Copy link
Member Author

Updated per comments. Includes change to truncation length for topology objects to 20 (from 16) to maintain consistency between topology objects and the menus and the rest of the UI (per slack discussion with @serenamarie125).

serenamarie125
serenamarie125 approved these changes Feb 6, 2020
View reviewed changes
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

UX requested that we have middle truncation as well as be sure that the number of chars was consistent in the Edit Menu and the Component Label in topology.

This looks great!

spadgett
spadgett reviewed Feb 6, 2020
View reviewed changes
frontend/packages/dev-console/src/utils/truncate-utils.ts Outdated
// is greater than the `maxLength` + `minTruncateChars` values.
// By default the middle is truncated, set the options.middle to false to truncate at the end.
// Truncated text is replaced with the provided omission option (ellipsis character by default);
export const truncateText = (text: string, options: TruncateOptions = {}): string => {
Copy link
Member

Choose a reason for hiding this comment

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

FYI, we do have an existing truncateMiddle helper: https://github.com/spadgett/console/blob/master/frontend/public/components/utils/truncate-middle.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this util is a bit different. I tried to use truncateMiddle but it fails as this is done as part of initialization of the plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ya there's that util...
@jeff-phillips-18 can you explain further what the issue is with using that util?
Can we enhance the existing util with what we need. There shouldn't be an issue re-using the module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was having some issues importing it from @console/internal/components/utils, but it works fine if I import it from @console/internal/components/utils/truncate-middle.

I've enhanced the existing util and imported as above.

frontend/packages/dev-console/src/actions/modify-application.ts
label: 'Edit',
href: `/edit?name=${obj.metadata.name}&kind=${obj.kind}`,
label: `Edit ${truncateText(obj.metadata.name)}`,
href: `/edit?name=${obj.metadata.name}&kind=${obj.kind || model.kind}`,
Copy link
Member

@spadgett spadgett Feb 6, 2020 •
edited

Choose a reason for hiding this comment

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

I would just use model.kind always. But should we be using referenceForModel? kind isn't unique.

It sounds like the menu shouldn't have shown up, though, since the particular resource in the bug wasn't creating through the dev console "Add" flow.

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't have. I've removed it at https://github.com/openshift/console/pull/4207/files#diff-2c9504a8d75ebb51fe3d613cf73328f2L8

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm not clear on the fix in that case. Should we be looking at an annotation or label on the resource to know it was created from the console add flow? I see the Edit action showing up for deployments and other resources that were created outside console.

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly we should. This PR does not fix that only the know crashing issue. I can file a separate issue for that or fix as part of this PR if you'd prefer.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I'm good tracking it as a separate BZ.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://issues.redhat.com/browse/ODC-2975

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Feb 7, 2020
@jeff-phillips-18
Copy link
Member Author

/retest

@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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-bot
Copy link
Contributor

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 27e8b61 into openshift:master Feb 8, 2020
@openshift-ci-robot
Copy link
Contributor

@jeff-phillips-18: All pull requests linked via external trackers have merged. Bugzilla bug 1797652 has been moved to the MODIFIED state.

In response to this:

Bug 1797652: Fix crash on Edit of applications

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@spadgett spadgett added this to the v4.4 milestone Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett left review comments

@christianvogt christianvogt christianvogt left review comments

@serenamarie125 serenamarie125 serenamarie125 approved these changes

@abhinandan13jan abhinandan13jan Awaiting requested review from abhinandan13jan

@nemesis09 nemesis09 Awaiting requested review from nemesis09

Assignees

@christianvogt christianvogt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/core Related to console core functionality component/dev-console Related to dev-console kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jeff-phillips-18 @openshift-ci-robot @christianvogt @openshift-bot @spadgett @serenamarie125 @openshift-merge-robot