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

Enhabce oVirt provider create, view/edit credentials help text fields #785

Merged

Conversation

sgratch
Copy link
Collaborator

@sgratch sgratch commented Nov 21, 2023

Reference: #646

Rephrase all oVirt fields help text messages and add hints for the following dialogs:

  1. Create oVirt provider,
  2. oVirt Credentials view/edit modes.

The main changes include:

  1. Rephrase help text messages for user, password, insecureSkipVerify, cacert fields (name and url fields were already rephrased in separate PRs)
  2. Support 'warning' validation state for the 'user' field.
  3. Add help text tooltips for insecureSkipVerify, cacert fields.

This fix assumes that leaving the 'cacert' field empty, will trigger verifying the the system CA certificates.

Screenshots

Create

Screenshot from 2024-01-03 21-19-42

View Credentials

Screenshot from 2024-01-03 21-20-29

Edit Credentials

Screenshot from 2024-01-03 21-21-27

@yaacov
Copy link
Member

yaacov commented Nov 21, 2023

can you add a little space above the blue hint box ?

screenshot-user-images githubusercontent com-2023 11 21-18_03_09

@yaacov yaacov added this to the 2.6.0 milestone Nov 21, 2023
@yaacov yaacov added the enhancement Categorizes issue or PR as related to a new feature. label Nov 21, 2023
@sgratch sgratch force-pushed the enhance-ovirt-provider-help-text-fields branch 2 times, most recently from 7e73fdc to bbc98e2 Compare November 21, 2023 17:03
@sgratch
Copy link
Collaborator Author

sgratch commented Nov 21, 2023

can you add a little space above the blue hint box ?

@yaacov Done, please check the new screenshot added to the description comment

@sgratch sgratch force-pushed the enhance-ovirt-provider-help-text-fields branch from bbc98e2 to b6663e8 Compare November 21, 2023 17:12
@sgratch
Copy link
Collaborator Author

sgratch commented Nov 21, 2023

cc:// @RichardHoch @anarnold97
Please review text changes

@sgratch
Copy link
Collaborator Author

sgratch commented Nov 21, 2023

This fix assumes that leaving the 'cacert' field empty, will trigger verifying the the system CA certificates.

@ahadas
Do we still support the combination of unchecking the 'skip cert validation' and leaving the oVirt provider's CA field empty for retrieving the system engine CA certificate by the webhook?

Based on https://redhat-internal.slack.com/archives/C049WBVTRD2/p1694592350447549?thread_ts=1694517736.539569&cid=C049WBVTRD2, there was an issue with that. It's currently not working for me as well. What is the status of this?

@RichardHoch
If above issue is still supported and should work then we need to document it as well since currently this option is not mentioned for the oVirt provider.

@sgratch sgratch requested review from yaacov and rszwajko November 21, 2023 17:30
@sgratch sgratch force-pushed the enhance-ovirt-provider-help-text-fields branch 4 times, most recently from 905051c to 533c808 Compare November 22, 2023 19:42
<Hint>
<HintBody>
{cacertHelperTextMsgs.hint}
<Button
Copy link
Member

Choose a reason for hiding this comment

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

please move the button inside the hint msg

Copy link
Collaborator Author

@sgratch sgratch Nov 28, 2023

Choose a reason for hiding this comment

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

For supporting a href link, I can't add the link text into the text message, but rather just replace Button with ExternalLink and wrap both with the Trans component.
The text message split is still required, please see the fix.

@sgratch sgratch force-pushed the enhance-ovirt-provider-help-text-fields branch 3 times, most recently from 9d02c05 to 9c05a4c Compare November 27, 2023 18:06
@ahadas
Copy link
Member

ahadas commented Nov 28, 2023

This fix assumes that leaving the 'cacert' field empty, will trigger verifying the the system CA certificates.

@ahadas Do we still support the combination of unchecking the 'skip cert validation' and leaving the oVirt provider's CA field empty for retrieving the system engine CA certificate by the webhook?

Based on https://redhat-internal.slack.com/archives/C049WBVTRD2/p1694592350447549?thread_ts=1694517736.539569&cid=C049WBVTRD2, there was an issue with that. It's currently not working for me as well. What is the status of this?

it is supposed to work once the certificate is added to the system certification pool but we didn't test it

@sgratch sgratch force-pushed the enhance-ovirt-provider-help-text-fields branch from 9c05a4c to 0c44e76 Compare November 28, 2023 12:34
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

@sgratch sgratch requested a review from yaacov November 28, 2023 12:59
const insecureSkipVerifyHelperTextMsgs = {
error: t('Error: this field must be set to a boolean value.'),
success: t("If true (check box is checked), the provider's CA certificate won't be validated."),
hint: t(
Copy link
Member

Choose a reason for hiding this comment

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

the hint is not a validation help message, e.g. success, warninig, error, it's a hint that is always the same, regardless of the validation state.
please create a new const for it.

Copy link
Collaborator Author

@sgratch sgratch Nov 28, 2023

Choose a reason for hiding this comment

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

The const name insecureSkipVerifyHelperTextMsgs doesn't include the word 'verify' so I see it as a collection of help text messages for this field and this specific hint is relevant for the insecureSkipVerify field only (explaining the enable/disable values).

But if you think that separating the help text based on component appearance is better then it's ok as well.

Copy link
Member

Choose a reason for hiding this comment

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

yes, the idea is that you can do insecureSkipVerifyHelperTextMsgs[ callVerificationMethod(text) ] in cases it makes sense, do the keys of the text array should be the same as the verification options.

hint: t(
'Note: enter the Manager CA certificate. unless it was replaced by a third-party certificate, in which case enter the Manager Apache CA certificate. You can retrieve the Manager CA certificate at: ',
),
hintHyperReference:
Copy link
Member

Choose a reason for hiding this comment

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

the hint reference is part of the hint text, you can use to create one string that include the reference

<trans> some text <link> some link </link> </transe>

) : (
<Hint>
<HintBody>
<Trans t={t} ns="plugin__forklift-console-plugin">
Copy link
Member

Choose a reason for hiding this comment

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

trans should include the text, this should look:
<HintBody>{hintBody}</HintBody>

description: t(
"If true (check box is checked), the provider's CA certificate won't be validated.",
),
hint: t(
Copy link
Member

Choose a reason for hiding this comment

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

hint is not specific to specific a field, hint is a form thing, please create a new const for the hint text, or just use it inline

@yaacov
Copy link
Member

yaacov commented Nov 29, 2023

I see that mixing #646 for the help msgs and #541for the hints is not easy, lets separate the PR to one that do help msgs for the tasks in #646 and another for adding hints as required by #541, this way way we can focus better on each task

Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Dec 30, 2023
@yaacov yaacov removed the Stale label Jan 2, 2024
Reference: kubev2v#646

Rephrase all oVirt fields help text messages and add tooltips for the following dialogs:
1. Create oVirt provider,
2. oVirt Credentials view/edit modes.

The main changes include:
1. Rephrase all help text messages.
2. Support 'warning' validation state for the 'user' field.
3. Add tooltips with a helpIcon for insecureSkipVerify, cacert fields.

This fix assumes that leaving the 'cacert' field empty, will trigger verifying the the system CA certificates.

Signed-off-by: Sharon Gratch <[email protected]>
@sgratch sgratch force-pushed the enhance-ovirt-provider-help-text-fields branch from 0c44e76 to 549afeb Compare January 3, 2024 19:07
Copy link

sonarqubecloud bot commented Jan 3, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.4% Duplication on New Code

See analysis details on SonarCloud

@sgratch
Copy link
Collaborator Author

sgratch commented Jan 3, 2024

  • After having an offline discussion, it was decided to convert both hints into tooltips that will include relevant fields help text for now.

    Reminder: It seems that that help text included relevant info for explaining how to use the fields, but since it's an extra info then tooltips seems preferable and less space expending.
    We can discuss using hints in the separate 🐾 Add information Hint in the create provider form #541 issue.

  • This current fix addressed all comments mentioned above.

  • I also updated the main comment and screenshots with the changes

@yaacov yaacov merged commit 402ebac into kubev2v:main Jan 3, 2024
7 checks passed
@sgratch sgratch deleted the enhance-ovirt-provider-help-text-fields branch January 3, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants