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

ODC-7531: Added the ApprovalTask List Tabbed Page #13651

Merged
merged 1 commit into from Mar 22, 2024
Merged

ODC-7531: Added the ApprovalTask List Tabbed Page #13651

merged 1 commit into from Mar 22, 2024

Conversation

Lucifergene
Copy link
Contributor

@Lucifergene Lucifergene commented Mar 6, 2024 •
edited

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

Depends on:
https://issues.redhat.com/browse/ODC-7519

Screen shots / Gifs for design review:
image

image

Demo:

Tab-Pipelines.OKD.1.mp4

Unit test coverage report:
Fixed broken tests. New tests will be added as part of another story.

Test setup:

  1. Install Pipelines operator and Install the ApprovalTask Controller following this Doc.
  2. Execute the following gist: https://gist.github.com/Lucifergene/f491504d7eda8a97e86a132029a9bc7b

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 6, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 6, 2024 •
edited by openshift-ci bot

@Lucifergene: This pull request references ODC-7531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Fixes:
https://issues.redhat.com/browse/ODC-7519
https://issues.redhat.com/browse/ODC-7531

Screen shots / Gifs for design review:
TBD

Unit test coverage report:
TBD

Test setup:
TBD

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added component/pipelines Related to pipelines-plugin kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Mar 6, 2024
@Lucifergene Lucifergene changed the title ODC-7519, ODC-7531: Added the Approval/Custom Nodes and ApprovalTask List Tabbed Page ODC-7531: Added the ApprovalTask List Tabbed Page Mar 12, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 12, 2024 •
edited by openshift-ci bot

@Lucifergene: This pull request references ODC-7531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

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

Screen shots / Gifs for design review:
TBD

Unit test coverage report:
TBD

Test setup:
TBD

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 12, 2024 •
edited by openshift-ci bot

@Lucifergene: This pull request references ODC-7531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

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

Depends on:
https://issues.redhat.com/browse/ODC-7519

Screen shots / Gifs for design review:
TBD

Unit test coverage report:
TBD

Test setup:
TBD

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

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 openshift-eng/jira-lifecycle-plugin repository.

@Lucifergene Lucifergene force-pushed the Approvals-Tab branch 2 times, most recently from 32ebfc2 to 77a172f Compare March 12, 2024 18:54
@openshift-ci openshift-ci bot added component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared kind/cypress Related to Cypress e2e integration testing labels Mar 12, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 12, 2024 •
edited by openshift-ci bot

@Lucifergene: This pull request references ODC-7531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

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

Depends on:
https://issues.redhat.com/browse/ODC-7519

Screen shots / Gifs for design review:
image

image

Demo:
https://github.com/openshift/console/assets/47265560/8dfb9ab8-1c2e-48b0-aa59-11ac22dbff51

Unit test coverage report:
Fixed broken tests. New tests will be added as part of another story.

Test setup:

  1. Install Pipelines operator and Install the ApprovalTask Controller following this Doc.
  2. Execute the following gist: https://gist.github.com/Lucifergene/f491504d7eda8a97e86a132029a9bc7b

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

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 openshift-eng/jira-lifecycle-plugin repository.

@Lucifergene
Copy link
Contributor Author

/cc @vikram-raj @lokanandaprabhu
/uncc @abhinandan13jan

@openshift-ci openshift-ci bot requested review from lokanandaprabhu and removed request for abhinandan13jan March 12, 2024 19:12
@Lucifergene
Copy link
Contributor Author

/retest

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 14, 2024 •
edited by openshift-ci bot

@Lucifergene: This pull request references ODC-7531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

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

Depends on:
https://issues.redhat.com/browse/ODC-7519

Screen shots / Gifs for design review:
image

image

Demo:

Tab-Pipelines.OKD.1.mp4

Unit test coverage report:
Fixed broken tests. New tests will be added as part of another story.

Test setup:

  1. Install Pipelines operator and Install the ApprovalTask Controller following this Doc.
  2. Execute the following gist: https://gist.github.com/Lucifergene/f491504d7eda8a97e86a132029a9bc7b

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

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 openshift-eng/jira-lifecycle-plugin repository.

@invincibleJai
Copy link
Member

This PR is dependent on #13613

@MariaLeonova
Copy link

MariaLeonova commented Mar 15, 2024 •
edited

@Lucifergene looks good! One question regarding the design in the Approvers column: it looks like the task was approved by 1 out of 4 approvers, and yet it's overall status is 'Approved'. How can that be?

@Lucifergene
Copy link
Contributor Author

@Lucifergene looks good! One question regarding the design in the Approvers column: it looks like the task was approved by 1 out of 4 approvers, and yet it's overall status is 'Approved'. How can that be?

@MariaLeonova, in the ApprovalTask resource YAML, we can specify the number of approvals required for the job. In this case, it was set to 1. Therefore, even on getting just one approval, it got approved.

vikram-raj
vikram-raj reviewed Mar 15, 2024
View reviewed changes
...tend/packages/pipelines-plugin/src/components/tasks/list-page/approval-tasks/ApprovalRow.tsx

React.useEffect(() => {
if (user.username === 'kube:admin') {
user.username = 'kubernetes-admin';
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for changing the username from kube:admin to kubernetes-admin? IMO go with kube:admin. We are using the same username at other places in the console.
image

...d/packages/pipelines-plugin/src/components/tasks/list-page/approval-tasks/ApprovalHeader.tsx
const ApprovalHeader = () => {
return [
{
title: i18next.t('pipelines-plugin~PipelineRun Name'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: i18next.t('pipelines-plugin~PipelineRun Name'),
title: i18next.t('pipelines-plugin~PipelineRun name'),

...d/packages/pipelines-plugin/src/components/tasks/list-page/approval-tasks/ApprovalHeader.tsx
id: 'approvers',
},
{
title: i18next.t('pipelines-plugin~Task Name'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: i18next.t('pipelines-plugin~Task Name'),
title: i18next.t('pipelines-plugin~Task name'),

...d/packages/pipelines-plugin/src/components/tasks/list-page/approval-tasks/ApprovalHeader.tsx
id: 'taskName',
},
{
title: i18next.t('pipelines-plugin~Current Status'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: i18next.t('pipelines-plugin~Current Status'),
title: i18next.t('pipelines-plugin~Current status'),

frontend/packages/pipelines-plugin/locales/en/pipelines-plugin.json
"Approval status": "Approval status",
"ApprovalTasks": "ApprovalTasks",
"An error occurred. Please try again": "An error occurred. Please try again",
"<0>{type === 'approve'\n ? 'Are you sure you want to approve'\n : 'Please provide a reason for not approving'} <2></2><3>{name}</3> in <6></6><7></7><8>{pipelineRunName}</8>?</0>": "<0>{type === 'approve'\n ? 'Are you sure you want to approve'\n : 'Please provide a reason for not approving'} <2></2><3>{name}</3> in <6></6><7></7><8>{pipelineRunName}</8>?</0>",
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look fine. The translation string contains conditions as well. Can you fix it.

...ges/pipelines-plugin/src/components/tasks/list-page/approval-tasks/ApprovalTasksListPage.tsx
return (
<>
<Helmet>
<title>{t('pipelines-plugin~ApprovalTasks')}</title>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<title>{t('pipelines-plugin~ApprovalTasks')}</title>
<title>{t('pipelines-plugin~Approval tasks')}</title>

@jerolimov
Copy link
Member

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Mar 19, 2024
vikram-raj
vikram-raj reviewed Mar 20, 2024
View reviewed changes
Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

Verified it. This feature works as expected. But there is some improvement suggested by UX and PM which can be done in the followup PR. As PR owner is on PTO.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Lucifergene, vikram-raj

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 Mar 20, 2024
@vikram-raj
Copy link
Member

/label doc-approved
/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Mar 20, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 20, 2024

@vikram-raj: The label(s) /label doc-approved cannot be applied. These labels are supported: acknowledge-critical-fixes-only, platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, no-qe, downstream-change-needed, rebase/manual, cluster-config-api-changed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label doc-approved
/label px-approved

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.

@vikram-raj
Copy link
Member

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Mar 20, 2024
@sanketpathak
Copy link
Contributor

Verified happy paths with admin and non-admin users. As edge cases will be dealt in another pr

Screen.Recording.2024-03-21.at.8.34.18.PM.mov

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Mar 21, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 21, 2024 •
edited by openshift-ci bot

@Lucifergene: This pull request references ODC-7531 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

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

Depends on:
https://issues.redhat.com/browse/ODC-7519

Screen shots / Gifs for design review:
image

image

Demo:

Tab-Pipelines.OKD.1.mp4

Unit test coverage report:
Fixed broken tests. New tests will be added as part of another story.

Test setup:

  1. Install Pipelines operator and Install the ApprovalTask Controller following this Doc.
  2. Execute the following gist: https://gist.github.com/Lucifergene/f491504d7eda8a97e86a132029a9bc7b

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 82b7a68 and 2 for PR HEAD 1ac81eb in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD cc3dedb and 1 for PR HEAD 1ac81eb in total

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 22, 2024

@Lucifergene: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 12e9fb5 into openshift:master Mar 22, 2024
6 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build openshift-enterprise-console-container-v4.16.0-202403221044.p0.g12e9fb5.assembly.stream.el8 for distgit openshift-enterprise-console.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vikram-raj vikram-raj vikram-raj left review comments

@lokanandaprabhu lokanandaprabhu Awaiting requested review from lokanandaprabhu

Assignees

@vikram-raj vikram-raj

Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin component/pipelines Related to pipelines-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. kind/cypress Related to Cypress e2e integration testing kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Lucifergene @openshift-ci-robot @invincibleJai @MariaLeonova @jerolimov @vikram-raj @sanketpathak @openshift-bot