METRON-1944: Unable to Delete a Comment in Alerts UI #1307
Conversation
|
@ruffle1986 It looks like Travis is failing, but it's failing during the integration tests that don't have to do with the UI (where your work happened). Specifically: From the looks of it, the VM just randomly crashed? Could you try closing/reopening this PR to re-run Travis? |
|
Is there any way to add a unit test for this? I know a lot of the testing code and infra has been a bit in flux, so I'm not quite sure how much of an ask that is. |
|
@ruffle1986 If the amount of mocking and setup you need to do is minimal, I think writing a unit test specific to the comment functionality of the alert details component should be done in this PR. However, I do see there are no unit tests currently setup for the alert details component. If the scope of setting up these tests is very time consuming, I think you should create a separate JIRA to cover this component with unit tests. That way, we aren't delaying the bug fix getting into master. |
ruffle1986
force-pushed
the
METRON-1944
branch
from
5e477f3
to
6aaf835
Compare
ruffle1986
force-pushed
the
METRON-1944
branch
from
6aaf835
to
86dd301
Compare
|
EDIT: @ruffle1986 After further testing, I think I caught a bug in this code (though it may have existed before and didn't show because of the comment deletion being broken). Here are the steps to reproduce:
|
|
@sardell Thanks for the report! Nice catch! Yes, it was there before probably. In order the enable comment deletion, I had to slightly modify the existing code. However the issue you're struggling with is critical, I respectfully think it's out of the scope of this PR. When it comes to removing comments (and adding as well!!), the app deals with HTTP requests which are not organized properly. When you successfully removed or added new comments, the server responses with the fresh new list of comments attached to the given alert. But if you're fast, adding/removing a bunch of comments then immediately closing and opening the comments pane again could cause certain issues. The problem is that the API endpoints for the deletion/addition returns with a list of comments and when you open the pane, it also returns the list of comments and the two are not necessarily the same at a certain point of time. They're not synced yet. Long story short, the current design and the implementation is not prepared for this kind of user quickness, the solution for this is not trivial and needs to have a further discussion for sure how to deal with it. If you don't think otherwise, I would open a Jira ticket about the aforementioned syncing issue and ship the patch in a different PR. |
|
@ruffle1986 I see. After reading your explanation, I agree that resolving this issue is going to take more thought, and will likely result in a pretty big change in implementation. I also agree that this is an issue bigger in scope than the original task of resolving a front-end specific bug which prevented a user from deleting a comment. +1 pending @ruffle1986 creating and sharing a JIRA ticket to track the syncing issue described above. |
|
I created the ASF Jira ticket which describes the aforementioned problem. I also made a video and added the scenario to reproduce the problem. Here you are: https://issues.apache.org/jira/browse/METRON-2001 |
| () => { | ||
| this.alertCommentsWrapper.map(alertsWrapper => alertsWrapper.alertComment) | ||
| }, | ||
| null, |
There was a problem hiding this comment.
I removed the next callback here because it makes no sense to map over an array and does nothing with the mapped array.
map does not mutate the array on which you call it. It creates a brand new array with the mapped elements. The code above does nothing with the return value of map so it can be removed.
There was a problem hiding this comment.
Yeah, that makes sense to remove since we do not know the original intention of the line of code.
@justinleet It looks like this line of code was added here. I'm going to assume it's fine to remove, but I'm curious what the original intention of the code was. Since there isn't a PR attached I'm not even sure you were the author of the code:
e0fc475#diff-bce5ae5c6be579250a35b6ff4f6da594R271
|
+1. Thanks for the contribution, @ruffle1986! |
Contributor Comments
Link to the original ASF Jira ticket: https://issues.apache.org/jira/browse/METRON-1944
The problem
The problem and its solution are quite trivial in this case. The code which is responsible for the deletion of the comment tried to address the deleted comment in an array after it was removed from it.
splicereturns with an array of the removed elements so we can keep working with that to persist the operation for the given comment on the server.Description from the ASF Jira ticket:
I am unable to delete a comment in the Alerts UI. While it appears that the comment is deleted, it is only deleted locally, but not server side. An exception is thrown in the browser console.
Steps To Replicate
Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
For all changes:
For code changes:
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
For documentation related changes:
Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via
site-book/target/site/index.html:Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.