ROX-12343: unauthenticated email integration by bradr5 · Pull Request #2984 · stackrox/stackrox

bradr5 · 2022-09-07T06:32:41Z

Description

We want to allow users the ability to configure email integrations without having to authenticate with SMTP

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

Testing Performed

On create

---

On edit

On create (vulnerability management modal)

ghost · 2022-09-07T07:01:45Z

Images are ready for the commit at dfe22f1.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.71.x-503-gdfe22f16c5.

openshift-ci · 2022-09-07T08:29:45Z

@bradr5: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/gke-ui-e2e-tests	`e3eea58`	link	false	`/test gke-ui-e2e-tests`

Full PR test history. Your PR dashboard.

Details

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.

pedrottimark · 2022-09-07T13:16:39Z

The integration test failure is not related to these changes.

should display all the columns expected in node components list page

Timed out retrying after 5000ms: cy.wait() timed out waiting 5000ms for the 1st request to the route: auth/status. No request ever occurred.

When it fits your priorities, we can investigate it together as a learning experience.

pedrottimark · 2022-09-07T13:22:02Z

ui/apps/platform/src/Containers/Integrations/IntegrationForm/Forms/EmailIntegrationForm.tsx

+                                    onBlur={handleBlur}
+                                />
+                                <ToggledPopoverElement
+                                    popoverContent="Enable unauthenticated SMTP will allow you to setup the RHACS email notifiers if you don’t have authenticated email services."


Not a blocker to merge:

Unless there is a difference in behavior between Red Hat and open-source StackRox, we omit reference to product branding.

Good callout. We can change that but who typically signs off on copy changes?

Good question. I will answer via direct message.

Let's just drop the part that reads "the RHACS"; that qualification is unnecessary to the meaning.

Also, for a small change for clarity like this, I agree with Grace Hopper, "If it's a good idea, go ahead and do it. It's much easier to apologize than it is to get permission."

Suggested change

popoverContent="Enable unauthenticated SMTP will allow you to setup the RHACS email notifiers if you don’t have authenticated email services."

popoverContent="Enable unauthenticated SMTP will allow you to setup an email notifier if you don’t have authenticated email services."

pedrottimark · 2022-09-07T13:28:36Z

ui/apps/platform/src/Containers/Integrations/IntegrationForm/Forms/EmailIntegrationForm.tsx

 import FormCancelButton from 'Components/PatternFly/FormCancelButton';
 import FormTestButton from 'Components/PatternFly/FormTestButton';
 import FormSaveButton from 'Components/PatternFly/FormSaveButton';
+import ToggledPopoverElement from 'Components/ToggledPopoverElement';


Not a blocker to merge: To reduce the effort to distinguish the many classic StackRox theme components from the fewer and newer components that are intended for PatternFly pages, we create them in PatternFly subfolder as in the import statements above.

Great info, I'll go ahead and get that moved over

pedrottimark · 2022-09-07T13:36:25Z

ui/apps/platform/src/Containers/Integrations/IntegrationForm/Forms/EmailIntegrationForm.tsx

+                        errors={errors}
+                    >
+                        <>
+                            <div className="pf-u-display-flex pf-u-align-items-flex-start">


Not a blocker to merge: See if the following example supports an equivalent popover via FormLabelGroup props without the need for flex alignment.

https://www.patternfly.org/v4/components/form#form-group-with-additional-label-info

Yeah I wasn't a huge fan of having to do it this way. I originally tried adding the isInline prop to the FormLabelGroup component and using that but patternfly added a pretty large margin between the icon and text. So I'm not sure if theres a way to do what we want without a few utility classes. I'll dig a little more though

pedrottimark

Clear changes to achieve the goal.

storedUsername is excellent attention to detail for interactive behavior
3 comments for your consideration for possible follow up contributions.

vjwilson

Left a high-level suggestion, a couple of comments, and a usage question.

vjwilson · 2022-09-07T14:46:01Z

ui/apps/platform/src/Components/ToggledPopoverElement/ToggledPopoverElement.tsx

@@ -0,0 +1,35 @@
+import React, { ReactElement, ReactNode } from 'react';


I prefer not to create extra reusable components that really wrap other reusable components (like PatternFly elements) and don't add much value or abstraction to the original reusable components.

Reusable components of reusable components remind me of Aristotle's "third-man" argument against Plato's theory of forms.
https://educalingo.com/en/dic-en/third-man-argument

I went back and forth on this – ultimately decided to create the component because it looks like we're using a button every time the popover is being used (I assume ADA reasons?) but I'm not hard stuck on this being the right approach and I could easily be swayed both ways

vjwilson · 2022-09-07T14:48:50Z

ui/apps/platform/src/Components/ToggledPopoverElement/ToggledPopoverElement.tsx

+            type="button"
+            aria-label={ariaLabel}
+            onClick={(e) => e.preventDefault()}
+            aria-describedby="simple-form-name-01"


I see where you saw the example of both aria-label and aria-describedby together, but I think they are redundant, and in this case the -describedby variant is unhelpful and better dropped. (Could also be dropped to improve the example locations, too.)

Whether you replace this component with direct use of the PatternFly pattern, or keep it, the aria-describedby should be deleted.

vjwilson · 2022-09-07T14:50:28Z

ui/apps/platform/src/Components/ToggledPopoverElement/ToggledPopoverElement.tsx

+            aria-label={ariaLabel}
+            onClick={(e) => e.preventDefault()}
+            aria-describedby="simple-form-name-01"
+            className={className}


When we do use className prop, let's make it optional, either with a default function param (className = '') or allow it to be appended to a necessary foundation className.

I'll go ahead and just remove the component, it looks like the other instances where we use the popover + button are unique enough to where I can see this reusable component getting cluttered

vjwilson · 2022-09-07T15:09:23Z

ui/apps/platform/src/Containers/Integrations/IntegrationForm/Forms/EmailIntegrationForm.tsx

+    function onUpdateUnauthenticatedChange(isChecked) {
+        if (isChecked) {
+            setStoredUsername(values.notifier.email.username);
+            setFieldValue('notifier.email.username', '');


@theencee Should we be clearing out the username when switching to unauthenticated, or is a username still required in certain cases?

(More importantly, is it required for the customer who asked for this feature?)

Unauth has optional username @vjwilson so safe to clear out

ROX-12343: unauthenticated email integration

e3eea58

bradr5 requested review from a team, pedrottimark and vjwilson September 7, 2022 06:32

github-actions bot added the area/ui label Sep 7, 2022

pedrottimark reviewed Sep 7, 2022

View reviewed changes

pedrottimark approved these changes Sep 7, 2022

View reviewed changes

vjwilson reviewed Sep 7, 2022

View reviewed changes

ROX-12343: remove reusable popover button

dfe22f1

bradr5 merged commit 257df54 into master Sep 7, 2022

bradr5 deleted the ROX-12343-unauthenticated-email-integration branch September 7, 2022 17:22

pedrottimark mentioned this pull request Sep 13, 2022

ROX-8674: Replace vmWorkflowUtils helper functions to fix timing problems in integration tests #3068

Merged

2 tasks

	popoverContent="Enable unauthenticated SMTP will allow you to setup the RHACS email notifiers if you don’t have authenticated email services."
	popoverContent="Enable unauthenticated SMTP will allow you to setup an email notifier if you don’t have authenticated email services."

		@@ -0,0 +1,35 @@
		import React, { ReactElement, ReactNode } from 'react';

Conversation

bradr5 commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

On create

On edit

On create (vulnerability management modal)

Uh oh!

ghost commented Sep 7, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Sep 7, 2022

Uh oh!

pedrottimark commented Sep 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrottimark left a comment

Choose a reason for hiding this comment

Uh oh!

vjwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bradr5 commented Sep 7, 2022 •

edited

Loading

ghost commented Sep 7, 2022 •

edited by ghost

Loading