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

[Cases] [104961] Remove conditional preventing additional comment field to be blank #117901

Conversation

jamster10
Copy link
Contributor

@jamster10 jamster10 commented Nov 8, 2021

Issue: #104961

Summary

A conditional prevented additional comments field from being blank. The fix was to remove said conditional.

Working:
recording

Release notes

Fix inability to clear out additional comments field in connectors.

@jamster10 jamster10 added release_note:fix v8.0.0 Team:Threat Hunting Security Solution Threat Hunting Team auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 Team:Threat Hunting:Cases v7.16.1 labels Nov 8, 2021
@jamster10 jamster10 self-assigned this Nov 8, 2021
@jamster10 jamster10 changed the title Remove conditional preventing additional comment field to be blank [Cases] [104961] Remove conditional preventing additional comment field to be blank Nov 8, 2021
@jamster10 jamster10 force-pushed the 104961-bug-unable-to-clear-additional-comments-field branch from 3a44c45 to 566ca87 Compare November 8, 2021 18:06
@jamster10 jamster10 marked this pull request as ready for review November 8, 2021 18:08
@jamster10 jamster10 requested a review from a team as a code owner November 8, 2021 18:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting-cases (Team:Threat Hunting:Cases)

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

@jamster10
Copy link
Contributor Author

@elasticmachine merge upstream

@jamster10 jamster10 enabled auto-merge (squash) November 16, 2021 17:54
@elastic elastic deleted a comment from kibanamachine Nov 16, 2021
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I test all incident manager connectors and is working as expected. Code LTGM!

In the Jira connector if you add a comment and then delete it you get this error when you run the test

Screenshot 2021-11-18 at 11 27 39 AM

I think this is happening because at the begging the comment is set to undefined. Then when you type and delete the comment is set to an empty string. It seems that Jira does not accept empty comments. I think the backend should not post a comment if it is empty (only for the Jira connector)

@cnasikas cnasikas removed the v7.16.1 label Nov 18, 2021
@@ -80,6 +80,9 @@ const pushToServiceHandler = async ({
if (comments && Array.isArray(comments) && comments.length > 0) {
res.comments = [];
for (const currentComment of comments) {
if (!currentComment.comment) {
break;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should continue because we need the other comments to be created in the external service.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree with @cnasikas here, we want to continue rather than break otherwise the remaining comments will be skipped.

@@ -80,6 +80,9 @@ const pushToServiceHandler = async ({
if (comments && Array.isArray(comments) && comments.length > 0) {
res.comments = [];
for (const currentComment of comments) {
if (!currentComment.comment) {
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree with @cnasikas here, we want to continue rather than break otherwise the remaining comments will be skipped.

@jamster10
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 778.0KB 778.0KB -60.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jamster10

@jamster10 jamster10 merged commit 782f152 into elastic:main Nov 22, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 22, 2021
…ld to be blank (elastic#117901)

* Remove conditional preventing additional comment field to be blank

* Remove tests for unwanted feature

* Update server to prevent empty string comments

* PR fix

Co-authored-by: Kristof-Pierre Cummings <kristofpierre.cummings@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 22, 2021
…ld to be blank (#117901) (#119396)

* Remove conditional preventing additional comment field to be blank

* Remove tests for unwanted feature

* Update server to prevent empty string comments

* PR fix

Co-authored-by: Kristof-Pierre Cummings <kristofpierre.cummings@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kristof C <kpac.ja@gmail.com>
Co-authored-by: Kristof-Pierre Cummings <kristofpierre.cummings@elastic.co>
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
…ld to be blank (#117901)

* Remove conditional preventing additional comment field to be blank

* Remove tests for unwanted feature

* Update server to prevent empty string comments

* PR fix

Co-authored-by: Kristof-Pierre Cummings <kristofpierre.cummings@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
…ld to be blank (elastic#117901)

* Remove conditional preventing additional comment field to be blank

* Remove tests for unwanted feature

* Update server to prevent empty string comments

* PR fix

Co-authored-by: Kristof-Pierre Cummings <kristofpierre.cummings@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022
…ld to be blank (elastic#117901)

* Remove conditional preventing additional comment field to be blank

* Remove tests for unwanted feature

* Update server to prevent empty string comments

* PR fix

Co-authored-by: Kristof-Pierre Cummings <kristofpierre.cummings@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:fix Team:Threat Hunting Security Solution Threat Hunting Team v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting] Unable to clear "Additional Comments" field in some connector tests
7 participants