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 2013996: Project detail page: Action "Delete Project" does nothing for the default project #10588

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Bug 2013996: Project detail page: Action "Delete Project" does nothing for the default project #10588

merged 1 commit into from
Dec 7, 2021

Conversation

yozaam
Copy link
Contributor

@yozaam yozaam commented Dec 1, 2021 •
edited
Loading

Fixes:

https://bugzilla.redhat.com/show_bug.cgi?id=2013996
https://issues.redhat.com/browse/OCPBUGSM-36171

Analysis / Root cause:

An action either has a callback or else a redirect
But when these are not present, the action will do 'nothing'

console/frontend/public/components/utils/dropdown.jsx

Lines 637 to 649 in 7044928

const onClick = (event, option) => {
event.preventDefault();
if (option.callback) {
option.callback();
}
if (option.href) {
history.push(option.href);
}
this.hide();
};

Solution Description:

Disable any action which does 'nothing'

Screen shots / Gifs for design review:

default project ( disabled )

Screenshot from 2021-12-01 10-43-56

normal project ( enabled )

Screenshot from 2021-12-01 10-45-01

Unit test coverage report:

in frontend we can use the new file yarn test ./public/components/utils/__tests__/kebab.spec.tsx
image

Test setup:

  1. Switch to developer perspective
  2. Navigate to 'Project' and select 'Project default' from the project list or the project dropdown
  3. Click Actions => 'Delete Project'

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci openshift-ci bot added the component/core Related to console core functionality label Dec 1, 2021
jerolimov
jerolimov approved these changes Dec 3, 2021
View reviewed changes
Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Did you know why the callback is undefined for the default project?
Can you add a test?

These questions should not block merging this PR. I would like to add LGTM as well at the end of today. :)

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021
@yozaam yozaam changed the title Bug 2013996 - Project detail page: Action "Delete Project" does nothing for the default project Bug 2013996: Project detail page: Action "Delete Project" does nothing for the default project Dec 3, 2021
@openshift-ci openshift-ci bot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Dec 3, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2021

@yozaam: This pull request references Bugzilla bug 2013996, 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.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2013996: Project detail page: Action "Delete Project" does nothing for the default project

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 openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Dec 3, 2021
@yozaam yozaam force-pushed the disable-default-project-delete branch from de1276b to 14c5514 Compare December 3, 2021 09:54
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2021

@yozaam: This pull request references Bugzilla bug 2013996, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2013996: Project detail page: Action "Delete Project" does nothing for the default project

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.

@yozaam yozaam force-pushed the disable-default-project-delete branch from 14c5514 to 09106d4 Compare December 3, 2021 10:40
@yozaam
Copy link
Contributor Author

yozaam commented Dec 3, 2021

have added unit tests
image

@jerolimov
Copy link
Member

Really nice! Thanks for adding these tests also for the cases you don't have changed 👍

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2021
@openshift-bot
Copy link
Contributor

/retest-required

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

7 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@jerolimov
Copy link
Member

The code contains a TypeScript error which breaks all tests. Please take a look next week. :)

/lgtm cancel
/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 3, 2021
@yozaam yozaam force-pushed the disable-default-project-delete branch from 09106d4 to f4fb200 Compare December 6, 2021 06:31
@jerolimov
Copy link
Member

Just fixed the TS build errors.

/approve
/lgtm
/cancel hold

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, yozaam

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2021
@yozaam
Copy link
Contributor Author

yozaam commented Dec 7, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2021
@openshift-merge-robot openshift-merge-robot merged commit 572cb05 into openshift:master Dec 7, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 7, 2021

@yozaam: All pull requests linked via external trackers have merged:

Bugzilla bug 2013996 has been moved to the MODIFIED state.

In response to this:

Bug 2013996: Project detail page: Action "Delete Project" does nothing for the default project

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.10 milestone Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerolimov jerolimov jerolimov approved these changes

@kyoto kyoto Awaiting requested review from kyoto

@TheRealJon TheRealJon Awaiting requested review from TheRealJon

@sanketpathak sanketpathak Awaiting requested review from sanketpathak

Assignees

@jerolimov jerolimov

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. 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 lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.10
Development

Successfully merging this pull request may close these issues.
5 participants
@yozaam @jerolimov @openshift-bot @spadgett @openshift-merge-robot