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

CONSOLE-2393: i18n user management page #7098

Merged

Conversation

wansc2016
Copy link
Contributor

@wansc2016 wansc2016 commented Nov 4, 2020

The PR includes i18n support for the following pages:
User management

  • Users
  • Groups
  • Service Accounts
  • Roles
  • Role Bindings

CONSOLE-2393

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Nov 4, 2020
@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 4, 2020
@openshift-ci-robot
Copy link
Contributor

Hi @wansc2016. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 component/knative Related to knative-plugin component/shared Related to console-shared labels Nov 4, 2020
@spadgett spadgett changed the title i18n user management page CONSOLE-2393: i18n user management page Nov 4, 2020
@wansc2016 wansc2016 force-pushed the i18n-usermanagement branch 2 times, most recently from 717093a to e2588d1 Compare December 1, 2020 06:33
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

I apologize for not reviewing sooner. I had the comments finished, but forgot to submit the review. Please feel free to reach out if you need a code review. Thanks.

Looks good overall. We are switching to use these conventions for capitalization as we externalize the strings: http://openshift.github.io/openshift-origin-design/conventions/documentation/capitalization.html

@@ -64,12 +66,12 @@ export const flatten = (resources) =>
return ret;
});

const menuActions = ({ subjectIndex, subjects }, startImpersonate) => {
const menuActions = ({ subjectIndex, subjects }, startImpersonate_) => {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you needed to add the underscore to startImpersonate? We've been trying to avoid trailing underscores where possible in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I will revoke it back.

frontend/public/components/RBAC/bindings.jsx Outdated Show resolved Hide resolved
frontend/public/components/RBAC/bindings.jsx Outdated Show resolved Hide resolved
frontend/public/components/RBAC/bindings.jsx Outdated Show resolved Hide resolved
frontend/public/components/RBAC/bindings.jsx Outdated Show resolved Hide resolved
frontend/public/components/modals/remove-user-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/service-account.jsx Outdated Show resolved Hide resolved
frontend/public/components/service-account.jsx Outdated Show resolved Hide resolved
frontend/public/components/user.tsx Outdated Show resolved Hide resolved
frontend/public/components/user.tsx Outdated Show resolved Hide resolved
@spadgett
Copy link
Member

spadgett commented Jan 6, 2021

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 6, 2021
@spadgett spadgett added kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 8, 2021
@openshift-ci-robot openshift-ci-robot added component/backend Related to backend component/ceph Related to ceph-storage-plugin component/dashboard Related to dashboard component/dev-console Related to dev-console component/git-service Related to git-service component/insights Related to insights plugin component/kubevirt Related to kubevirt-plugin component/lso Related to local-storage-operator-plugin component/metal3 Related to metal3-plugin component/monitoring Related to monitoring component/network-attachment-definition Related to network-attachment-definition labels Jan 10, 2021
@wansc2016
Copy link
Contributor Author

@spadgett Should we change Role Binding to RoleBinding in all places? Will it be inconsistent with Navigation Menu?

@spadgett
Copy link
Member

@spadgett Should we change Role Binding to RoleBinding in all places? Will it be inconsistent with Navigation Menu?

Yes, #7277 updates the navigation menu.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

LGTM, except it looks like we have a test failure on the role bindings page. This is ready for testing and approvals, however.

/assign @yapei @ahardin-rh @reestr


const bindingKinds = [
{
value: 'RoleBinding',
title: 'Namespace Role Binding (RoleBinding)',
desc: 'Grant the permissions to a user or set of users within the selected namespace.',
title: i18next.t('bindings~Namespace RoleBinding (RoleBinding)'),
Copy link
Member

Choose a reason for hiding this comment

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

nit: Here and below I would use "role binding" since it's not exactly a k8s kind in this case. (The kind appears beside in parentheses.)

Suggested change
title: i18next.t('bindings~Namespace RoleBinding (RoleBinding)'),
title: i18next.t('bindings~Namespace role binding (RoleBinding)'),

title: 'Cluster-wide Role Binding (ClusterRoleBinding)',
desc:
'Grant the permissions to a user or set of users at the cluster level and in all namespaces.',
title: i18next.t('bindings~Cluster-wide RoleBinding (ClusterRoleBinding)'),
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('bindings~Cluster-wide RoleBinding (ClusterRoleBinding)'),
title: i18next.t('bindings~Cluster-wide role binding (ClusterRoleBinding)'),

@yanpzhan
Copy link
Contributor

Reported a bug about the pr: https://bugzilla.redhat.com/show_bug.cgi?id=1915723

@yanpzhan
Copy link
Contributor

yanpzhan commented Jan 13, 2021

"Add More" is not marked for translation on add user model:
Screenshot from 2021-01-13 16-27-35

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2021
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2021
@wansc2016
Copy link
Contributor Author

@yanpzhan https://bugzilla.redhat.com/show_bug.cgi?id=1915723 has been fixed.

"Add more" issue will be fixed with https://issues.redhat.com/browse/CONSOLE-2474

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 13, 2021

@wansc2016: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/kubevirt-plugin 97c1268 link /test kubevirt-plugin

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.

@yanpzhan
Copy link
Contributor

/label qe-approved

@openshift-ci-robot openshift-ci-robot added the qe-approved Signifies that QE has signed off on this PR label Jan 14, 2021
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold for docs approval

@ahardin-rh PTAL, thanks!

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 14, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spadgett, wansc2016

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2021
@ahardin-rh
Copy link

/label docs-approved

@openshift-ci-robot openshift-ci-robot added the docs-approved Signifies that Docs has signed off on this PR label Jan 14, 2021
@spadgett
Copy link
Member

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 14, 2021
@spadgett
Copy link
Member

Thanks @wansc2016 for helping us with this!

@openshift-merge-robot openshift-merge-robot merged commit 26c4405 into openshift:master Jan 14, 2021
@spadgett spadgett added this to the v4.7 milestone Jan 18, 2021
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/core Related to console core functionality docs-approved Signifies that Docs has signed off on this PR 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants