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

Add Status of Findings when Close Finding #7068

Merged
merged 14 commits into from
Nov 11, 2022
Merged

Add Status of Findings when Close Finding #7068

merged 14 commits into from
Nov 11, 2022

Conversation

italvi
Copy link
Contributor

@italvi italvi commented Nov 2, 2022

This is a try to resolve the issue discussed in #7059

Result:
grafik

adjusted to include finding status when close a finding
changed close_finding to include status of a finding
updated serializer to include finding status when closing finding
added finding status to finding_close
@github-actions github-actions bot added the apiv2 label Nov 2, 2022
changed false_positive to false_p and tried to remove flake8
fixing Flak8 E231
removed whitespaces
changed false_positive to false_p
revert mistakenly changing close
@italvi
Copy link
Contributor Author

italvi commented Nov 2, 2022

@alles-klar as far as I know you are an expert to the k8 deployment: Do you know why it fails?
"Error: looks like "https://charts.bitnami.com/bitnami" is not a valid chart repository or cannot be reached: Get "https://charts.bitnami.com/bitnami/index.yaml": dial tcp: lookup charts.bitnami.com on 127.0.0.53:53: no such host"

@mtesauro
Copy link
Contributor

mtesauro commented Nov 2, 2022

I re-ran the failed ones and it seems to be working now.

dojo/api_v2/views.py Outdated Show resolved Hide resolved
italvi and others added 2 commits November 3, 2022 09:09
Co-authored-by: Cody Maffucci <46459665+Maffooch@users.noreply.github.com>
fixed a missing '
@italvi
Copy link
Contributor Author

italvi commented Nov 3, 2022

@Maffooch you are right with the .get. Was a bit lazy from my side to do it in one file but not another. I implemented your suggestion and added a missing apostrophe.

@italvi
Copy link
Contributor Author

italvi commented Nov 3, 2022

I have 2 improvements that I would like to add in the future:

  1. For the duplicates field there should be the possiblity to choose from findings which is the "original"
  2. Remove the possibility to edit the status in the EditForm, s.t. nobody can just change the status without adding a note.

What do you think? Would this be also good PR?

@italvi italvi requested a review from Maffooch November 3, 2022 09:49
@Maffooch
Copy link
Contributor

Maffooch commented Nov 4, 2022

For the duplicates field there should be the possiblity to choose from findings which is the "original"

This seems sensical to me when manually setting the flag.

Remove the possibility to edit the status in the EditForm, s.t. nobody can just change the status without adding a note.

I think this would be a little annoying tbh. Especially for smaller teams or folks just getting to know dojo. I think it would be best to leave that to the users

@italvi
Copy link
Contributor Author

italvi commented Nov 4, 2022

Remove the possibility to edit the status in the EditForm, s.t. nobody can just change the status without adding a note.

I think this would be a little annoying tbh. Especially for smaller teams or folks just getting to know dojo. I think it would be best to leave that to the users

@Maffooch could you explain me https://github.com/DefectDojo/django-DefectDojo/blob/master/dojo/finding/views.py#L744 then? It seems to me that somebody had the inital thought of checking whether the finding includes any notes? If so, then the contributor made a mistake with closing_disabled!=0, which should be closing_disabled==0 as note_type_activation will be 0 when there are no notes and therefore closing_disabled will remain 0

@Maffooch
Copy link
Contributor

Maffooch commented Nov 4, 2022

Looks like something to do with note types from this #1539
From just a brief skim of the PR description, it looks like if you have a specific note type configured, it requires that a note of that type be present in the finding before it is actually closed. I like this approach

@italvi
Copy link
Contributor Author

italvi commented Nov 9, 2022

@mtesauro could you please review the PR too or is something missing?

@mtesauro
Copy link
Contributor

@italvi LGTM - approving and merging

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

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

Approve

@mtesauro mtesauro merged commit b57c25a into DefectDojo:dev Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants