-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat(ui): bulk remove vulnerabilities #168
feat(ui): bulk remove vulnerabilities #168
Conversation
Any reason why some of the content of your PR description is quoted? |
Nope just wanted it to be easier to read |
Sept 4th 2024 Update :This PR will add the following features :
On my behalf I have tested it, and it seems to work 💯 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0b3ud Thanks for this.
Could you use the Blue color for the open state, red is already used for vuln severity and it could be confusing
To be consistent with other tables could you move the mass actions buttons to the left side like in the subdomain table
Hey
|
@0b3ud Can you check the checkboxes where applicable? e.g. beginning of this PR and your latest post. Not checking them indicates it's not done. |
Done :) |
Don't have tested yet. |
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request implements a new feature for bulk removal of vulnerabilities in the UI. The changes include adding a select-all checkbox for vulnerabilities, implementing bulk deletion functionality, and modifying the vulnerability status change mechanism. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @0b3ud - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Avoid hardcoding credentials in the code. (link)
Overall Comments:
- Consider adding an extra confirmation step or more explicit UI warnings for bulk delete operations to prevent accidental data loss.
- Some JavaScript functions, particularly those handling bulk operations, could be optimized for better performance with large datasets.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Please finish this up so I can test this (after sourcery's suggestions). |
Done you can test it now :) |
Near perfect, just one thing, you need to disable mass change buttons if no checkbox is checked. function onchange(event) {
toggleMultipleSubdomainButton()
}
function toggleMultipleSubdomainButton() {
var checked_count = checkedCount();
if (checked_count > 0) {
$("[data-button=subdomain_btns]").removeClass("disabled");
$('#subdomain_selected_count').text(checked_count + ' Subdomains Selected x');
} else {
$("[data-button=subdomain_btns]").addClass("disabled");
$('#subdomain_selected_count').empty();
}
} |
Hey @psyray |
@@ -298,7 +293,7 @@ | |||
|
|||
$('#vulnerability_results').on('click', 'tr' , function (e) { | |||
console.log(e.target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this console log
8b6313b
into
Security-Tools-Alliance:release/2.1.0
Reading this issue I was able to split this feature in two tasks :
Selecting all vulnerabilities in vulnerability page :
Results :
I would appreciate it if you can do some tests on your side and notify me if there are any bugs
Summary by Sourcery
Implement bulk operations for vulnerabilities, including selecting all, resolving, and deleting multiple vulnerabilities at once. Enhance the user interface to support these operations and improve the dynamic status update of vulnerabilities.
New Features:
Enhancements: