-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
apiv2: set mitigated date if applicable #3285
Conversation
keenan-v1
commented
Nov 17, 2020
- Sets or clears mitigated date based on the value of is_Mitigated
- Sets or clears mitigated_by based on the value of is_Mitigated and current user context
- Sets or clears mitigated date based on the value of is_Mitigated - Sets or clears mitigated_by based on the value of is_Mitigated
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.
thanks. i was confused a bit, but the logic looks ok. could you make it so we have 2 booleans and call save if one of them is True. so 'push_to_jira' and 'set_mitigated'? now if you quickyl read it could make readers think in some cases we will not save the instance at all due to the naming of the boolean.
should the same logic be in perform_create? if somebody creates a finding that is already marked as mitigated? |
dojo/api_v2/serializers.py
Outdated
instance.mitigated_by = self.context['request'].user | ||
if settings.USE_TZ: | ||
instance.mitigated = timezone.make_aware(instance.mitigated, timezone.get_default_timezone()) | ||
elif not instance.is_Mitigated and instance.mitigation is not None: |
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.
should this check for instance.mitigated
instead of instance.mitigation
?
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.
Good catch. My testing didn't catch this. Will fix.
I've added the requested changes and fixed the bug in variable naming. Tested the functionality and it is working as intended. |
thanks for the PR |
As a push to jira will actually overwrite the jira description field, this update may come as a surprise to some users. On the UI, there is the |
Isn't that more a problem with the push-to-jira patch and less of a problem with this one? I'm not entirely sure how the Jira stuff works as I don't use it yet. I simply worked around the Jira code here as it was overriding the same areas as my original fix for the mitigated date did. |