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

Improve wording for TLS-related options #115

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

smortex
Copy link
Contributor

@smortex smortex commented Sep 21, 2023

When configuring a SMTP sender, the user is prompted for an "Encryption method" and given a drop-down list to choose from. The list currently contain "SSL", "TLS" and "None" which is a bit confusing given that TLS is built on deprecated SSL and both refer to the same thing.

In this list, "SSL" actually mean TLS, and "TLS" actually mean StartTLS (aka Opportunistic TLS).

Adjust wording to wake this more obvious. The new wording match the one used in Thunderbird when configuring connection security.

@smortex
Copy link
Contributor Author

smortex commented Sep 22, 2023

@Hailong-am one of the failing CI seems to be a transient error but the other one is caused by me who missed the test 😶. I fixed my commit and force-pushed. Ready for another CI run!

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Merging #115 (2d63e5f) into main (e435e31) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #115   +/-   ##
=======================================
  Coverage   86.70%   86.70%           
=======================================
  Files          52       52           
  Lines        1497     1497           
  Branches      373      373           
=======================================
  Hits         1298     1298           
  Misses        196      196           
  Partials        3        3           
Flag Coverage Δ
dashboards-notifications 86.70% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@smortex
Copy link
Contributor Author

smortex commented Sep 22, 2023

Hum, not sure I am reading the windows failure log correctly: I feel like the errors logged on each test of some components are JS errors which can be ignored because the test pass (the errors are logged after the test result is displayed?):

2023-09-22T09:26:36.6989476Z PASS public/pages/CreateChannel/__tests__/CreateChannel.test.tsx (79.85 s)
2023-09-22T09:26:36.8064594Z     console.error
2023-09-22T09:26:36.8068959Z       Warning: An update to CreateChannel inside a test was not wrapped in act(...).
2023-09-22T09:26:36.8071060Z       
2023-09-22T09:26:36.8072340Z       When testing, code that causes React state updates should be wrapped into act(...):
2023-09-22T09:26:36.8072852Z       
2023-09-22T09:26:36.8073292Z       act(() => {
2023-09-22T09:26:36.8073716Z         /* fire events that update state */
2023-09-22T09:26:36.8074152Z       });
2023-09-22T09:26:36.8074589Z       /* assert on the output */
2023-09-22T09:26:36.8075000Z       
2023-09-22T09:26:36.8079779Z       This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
2023-09-22T09:26:36.8080836Z           in CreateChannel
2023-09-22T09:26:36.8081179Z 
2023-09-22T09:26:36.8081598Z       180 |       const type = response.config_type as keyof typeof CHANNEL_TYPE;
2023-09-22T09:26:36.8082188Z       181 |       setIsEnabled(response.is_enabled);
2023-09-22T09:26:36.8082661Z     > 182 |       setName(response.name);
2023-09-22T09:26:36.8083104Z           |       ^
2023-09-22T09:26:36.8083823Z       183 |       setDescription(response.description || '');
2023-09-22T09:26:36.8084376Z       184 |       setChannelType(type);
2023-09-22T09:26:36.8084761Z       185 |
2023-09-22T09:26:36.8085377Z 
2023-09-22T09:26:36.8086029Z       at printWarning (../../node_modules/react-dom/cjs/react-dom.development.js:88:30)
2023-09-22T09:26:36.8086847Z       at error (../../node_modules/react-dom/cjs/react-dom.development.js:60:5)
2023-09-22T09:26:36.8087747Z       at warnIfNotCurrentlyActingUpdatesInDev (../../node_modules/react-dom/cjs/react-dom.development.js:23284:7)
2023-09-22T09:26:36.8089011Z       at setName (../../node_modules/react-dom/cjs/react-dom.development.js:15656:9)
2023-09-22T09:26:36.8089731Z       at getChannel (public/pages/CreateChannel/CreateChannel.tsx:182:7)
2023-09-22T09:29:05.5432574Z PASS public/pages/Channels/__tests__/Channels.test.tsx (5.159 s)
2023-09-22T09:29:05.5700577Z     console.error
2023-09-22T09:29:05.5728910Z       Warning: Encountered two children with the same key, `row_test-slack`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.
2023-09-22T09:29:05.5729767Z           in tbody (created by EuiTableBody)
2023-09-22T09:29:05.5730398Z           in EuiTableBody (created by EuiBasicTable)
2023-09-22T09:29:05.5731919Z           in table (created by EuiTable)
2023-09-22T09:29:05.5732722Z           in EuiTable (created by EuiBasicTable)
2023-09-22T09:29:05.5733160Z           in div (created by EuiBasicTable)
2023-09-22T09:29:05.5733974Z           in div (created by EuiBasicTable)
2023-09-22T09:29:05.5734404Z           in EuiBasicTable (created by Channels)
2023-09-22T09:29:05.5735127Z           in div (created by ContentPanel)
2023-09-22T09:29:05.5735466Z           in div (created by EuiPanel)
2023-09-22T09:29:05.5735774Z           in EuiPanel (created by ContentPanel)
2023-09-22T09:29:05.5736143Z           in ContentPanel (created by Channels)
2023-09-22T09:29:05.5736438Z           in Channels
2023-09-22T09:29:05.5736595Z 
2023-09-22T09:29:05.5737086Z       at console.call [as error] (../../node_modules/@testing-library/react/dist/act-compat.js:55:34)
2023-09-22T09:29:05.5737755Z       at printWarning (../../node_modules/react-dom/cjs/react-dom.development.js:88:30)
2023-09-22T09:29:05.5740243Z       at error (../../node_modules/react-dom/cjs/react-dom.development.js:60:5)
2023-09-22T09:29:05.5741091Z       at warnOnInvalidKey (../../node_modules/react-dom/cjs/react-dom.development.js:13801:11)
2023-09-22T09:29:05.5741779Z       at reconcileChildrenArray (../../node_modules/react-dom/cjs/react-dom.development.js:13832:21)
2023-09-22T09:29:05.5742463Z       at reconcileChildFibers (../../node_modules/react-dom/cjs/react-dom.development.js:14305:14)
2023-09-22T09:29:05.5743194Z       at reconcileChildren (../../node_modules/react-dom/cjs/react-dom.development.js:16769:28)
2023-09-22T09:29:05.5744135Z       at updateHostComponent (../../node_modules/react-dom/cjs/react-dom.development.js:17302:3)

But I can't identify the actual failure in the log. The following looks like a good candidate but some test setup failure but lacks details:

2023-09-22T09:44:08.7718222Z ##[error]Process completed with exit code 124.

@Hailong-am
Copy link
Collaborator

Hum, not sure I am reading the windows failure log correctly: I feel like the errors logged on each test of some components are JS errors which can be ignored because the test pass (the errors are logged after the test result is displayed?):

2023-09-22T09:26:36.6989476Z PASS public/pages/CreateChannel/__tests__/CreateChannel.test.tsx (79.85 s)
2023-09-22T09:26:36.8064594Z     console.error
2023-09-22T09:26:36.8068959Z       Warning: An update to CreateChannel inside a test was not wrapped in act(...).
2023-09-22T09:26:36.8071060Z       
2023-09-22T09:26:36.8072340Z       When testing, code that causes React state updates should be wrapped into act(...):
2023-09-22T09:26:36.8072852Z       
2023-09-22T09:26:36.8073292Z       act(() => {
2023-09-22T09:26:36.8073716Z         /* fire events that update state */
2023-09-22T09:26:36.8074152Z       });
2023-09-22T09:26:36.8074589Z       /* assert on the output */
2023-09-22T09:26:36.8075000Z       
2023-09-22T09:26:36.8079779Z       This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
2023-09-22T09:26:36.8080836Z           in CreateChannel
2023-09-22T09:26:36.8081179Z 
2023-09-22T09:26:36.8081598Z       180 |       const type = response.config_type as keyof typeof CHANNEL_TYPE;
2023-09-22T09:26:36.8082188Z       181 |       setIsEnabled(response.is_enabled);
2023-09-22T09:26:36.8082661Z     > 182 |       setName(response.name);
2023-09-22T09:26:36.8083104Z           |       ^
2023-09-22T09:26:36.8083823Z       183 |       setDescription(response.description || '');
2023-09-22T09:26:36.8084376Z       184 |       setChannelType(type);
2023-09-22T09:26:36.8084761Z       185 |
2023-09-22T09:26:36.8085377Z 
2023-09-22T09:26:36.8086029Z       at printWarning (../../node_modules/react-dom/cjs/react-dom.development.js:88:30)
2023-09-22T09:26:36.8086847Z       at error (../../node_modules/react-dom/cjs/react-dom.development.js:60:5)
2023-09-22T09:26:36.8087747Z       at warnIfNotCurrentlyActingUpdatesInDev (../../node_modules/react-dom/cjs/react-dom.development.js:23284:7)
2023-09-22T09:26:36.8089011Z       at setName (../../node_modules/react-dom/cjs/react-dom.development.js:15656:9)
2023-09-22T09:26:36.8089731Z       at getChannel (public/pages/CreateChannel/CreateChannel.tsx:182:7)
2023-09-22T09:29:05.5432574Z PASS public/pages/Channels/__tests__/Channels.test.tsx (5.159 s)
2023-09-22T09:29:05.5700577Z     console.error
2023-09-22T09:29:05.5728910Z       Warning: Encountered two children with the same key, `row_test-slack`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.
2023-09-22T09:29:05.5729767Z           in tbody (created by EuiTableBody)
2023-09-22T09:29:05.5730398Z           in EuiTableBody (created by EuiBasicTable)
2023-09-22T09:29:05.5731919Z           in table (created by EuiTable)
2023-09-22T09:29:05.5732722Z           in EuiTable (created by EuiBasicTable)
2023-09-22T09:29:05.5733160Z           in div (created by EuiBasicTable)
2023-09-22T09:29:05.5733974Z           in div (created by EuiBasicTable)
2023-09-22T09:29:05.5734404Z           in EuiBasicTable (created by Channels)
2023-09-22T09:29:05.5735127Z           in div (created by ContentPanel)
2023-09-22T09:29:05.5735466Z           in div (created by EuiPanel)
2023-09-22T09:29:05.5735774Z           in EuiPanel (created by ContentPanel)
2023-09-22T09:29:05.5736143Z           in ContentPanel (created by Channels)
2023-09-22T09:29:05.5736438Z           in Channels
2023-09-22T09:29:05.5736595Z 
2023-09-22T09:29:05.5737086Z       at console.call [as error] (../../node_modules/@testing-library/react/dist/act-compat.js:55:34)
2023-09-22T09:29:05.5737755Z       at printWarning (../../node_modules/react-dom/cjs/react-dom.development.js:88:30)
2023-09-22T09:29:05.5740243Z       at error (../../node_modules/react-dom/cjs/react-dom.development.js:60:5)
2023-09-22T09:29:05.5741091Z       at warnOnInvalidKey (../../node_modules/react-dom/cjs/react-dom.development.js:13801:11)
2023-09-22T09:29:05.5741779Z       at reconcileChildrenArray (../../node_modules/react-dom/cjs/react-dom.development.js:13832:21)
2023-09-22T09:29:05.5742463Z       at reconcileChildFibers (../../node_modules/react-dom/cjs/react-dom.development.js:14305:14)
2023-09-22T09:29:05.5743194Z       at reconcileChildren (../../node_modules/react-dom/cjs/react-dom.development.js:16769:28)
2023-09-22T09:29:05.5744135Z       at updateHostComponent (../../node_modules/react-dom/cjs/react-dom.development.js:17302:3)

But I can't identify the actual failure in the log. The following looks like a good candidate but some test setup failure but lacks details:

2023-09-22T09:44:08.7718222Z ##[error]Process completed with exit code 124.

O
No worries, it seems that osd server can't start up on windows platform, i will take look at this failure

@Hailong-am
Copy link
Collaborator

@smortex would you mind update TLS here https://github.com/opensearch-project/dashboards-notifications/blob/main/.cypress/integration/email_senders_and_groups.spec.js#L70 to STARTTLS, as both options have TLS, we need specify STARTTLS exactly to match start_tls case.

When configuring a SMTP sender, the user is prompted for an "Encryption
method" and given a drop-down list to choose from.  The list currently
contain "SSL", "TLS" and "None" which is a bit confusing given that TLS
is built on deprecated SSL and both refer to the same thing.

In this list, "SSL" actually mean TLS, and "TLS" actually mean StartTLS
(aka Opportunistic TLS).

Adjust wording to wake this more obvious.  The new wording match the one
used in Thunderbird when configuring connection security.

Signed-off-by: Romain Tartière <[email protected]>
@smortex
Copy link
Contributor Author

smortex commented Sep 25, 2023

@Hailong-am done!

@Hailong-am
Copy link
Collaborator

Have separate PR to fix windows platform test #118

@yuye-aws yuye-aws merged commit 2506331 into opensearch-project:main Sep 26, 2023
5 of 6 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 26, 2023
When configuring a SMTP sender, the user is prompted for an "Encryption
method" and given a drop-down list to choose from.  The list currently
contain "SSL", "TLS" and "None" which is a bit confusing given that TLS
is built on deprecated SSL and both refer to the same thing.

In this list, "SSL" actually mean TLS, and "TLS" actually mean StartTLS
(aka Opportunistic TLS).

Adjust wording to wake this more obvious.  The new wording match the one
used in Thunderbird when configuring connection security.

Signed-off-by: Romain Tartière <[email protected]>
(cherry picked from commit 2506331)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@smortex smortex deleted the tls-wording branch September 26, 2023 15:43
yuye-aws pushed a commit that referenced this pull request Sep 27, 2023
When configuring a SMTP sender, the user is prompted for an "Encryption
method" and given a drop-down list to choose from.  The list currently
contain "SSL", "TLS" and "None" which is a bit confusing given that TLS
is built on deprecated SSL and both refer to the same thing.

In this list, "SSL" actually mean TLS, and "TLS" actually mean StartTLS
(aka Opportunistic TLS).

Adjust wording to wake this more obvious.  The new wording match the one
used in Thunderbird when configuring connection security.


(cherry picked from commit 2506331)

Signed-off-by: Romain Tartière <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot added a commit that referenced this pull request Sep 27, 2023
When configuring a SMTP sender, the user is prompted for an "Encryption
method" and given a drop-down list to choose from.  The list currently
contain "SSL", "TLS" and "None" which is a bit confusing given that TLS
is built on deprecated SSL and both refer to the same thing.

In this list, "SSL" actually mean TLS, and "TLS" actually mean StartTLS
(aka Opportunistic TLS).

Adjust wording to wake this more obvious.  The new wording match the one
used in Thunderbird when configuring connection security.

(cherry picked from commit 2506331)

Signed-off-by: Romain Tartière <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 61ceae1)
yuye-aws pushed a commit that referenced this pull request Sep 28, 2023
When configuring a SMTP sender, the user is prompted for an "Encryption
method" and given a drop-down list to choose from.  The list currently
contain "SSL", "TLS" and "None" which is a bit confusing given that TLS
is built on deprecated SSL and both refer to the same thing.

In this list, "SSL" actually mean TLS, and "TLS" actually mean StartTLS
(aka Opportunistic TLS).

Adjust wording to wake this more obvious.  The new wording match the one
used in Thunderbird when configuring connection security.

(cherry picked from commit 2506331)

Signed-off-by: Romain Tartière <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 61ceae1)

Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants