-
Notifications
You must be signed in to change notification settings - Fork 108
Adding sorting layer to the compliance alerts #1581 #1632
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
base: main
Are you sure you want to change the base?
Adding sorting layer to the compliance alerts #1581 #1632
Conversation
12a2b28
to
583c38a
Compare
Signed-off-by: AbanoubAziz <[email protected]>
Signed-off-by: AbanoubAziz <[email protected]>
3973b41
to
2e6f4a7
Compare
Signed-off-by: AbanoubAziz <[email protected]>
Signed-off-by: AbanoubAziz <[email protected]>
Good morning, I have made some changes to the code logic and updated the test file to check for the compliance order by severity; please check them out. I would really appreciate your feedback. @pombredanne. |
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++ @abanoub-samy-farhan, this works nicely. See comments for some minor nits for your consideration.
Could you also make sure all the tests are passing?
scanpipe/pipes/compliance.py
Outdated
@@ -72,9 +72,21 @@ def group_compliance_alerts_by_severity(queryset): | |||
string representations of the instances associated with that severity. | |||
""" | |||
compliance_alerts = defaultdict(list) | |||
severity_levels = ['error', 'warning', 'missing'] |
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.
Instead of adding a new list here with the values, we should extract the already defined values dict at https://github.com/aboutcode-org/scancode.io/blob/main/scanpipe/models.py#L2200 and use the keys from there.
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 for the feedback, I have implemented a method in the ComplianceAlertQuerySetMixin
class to retrive the order instantly from the queryset
def compliance_alerts_ordered_by_severity(self):
"""
Return a list of compliance alerts ordered by severity.
"""
compliance = self.model.Compliance
return [
compliance.ERROR.value,
compliance.WARNING.value,
compliance.MISSING.value,
]
now instead of hard coding a list, I can call this method
severity_levels = queryset.compliance_alerts_ordered_by_severity()
Kindly, if you would like me to define a global variable or use another method, please inform me.
scanpipe/pipes/compliance.py
Outdated
# Sort keys for consistent ordering (["error", "warning", "missing"]) | ||
sorted_keys = sorted( | ||
compliance_alerts.keys(), | ||
key=lambda label: severity_levels.index(label) if label in severity_levels else len(severity_levels) |
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 have the if statement in a new line? This is failing code style checks.
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.
Noted I have fixed it
Signed-off-by: Abanoub Aziz <[email protected]>
Signed-off-by: Abanoub Aziz <[email protected]>
I have worked on the bug found in #1581
I have added a sorting layer before returning the list of compliance alerts by using the possible labels found.
If there are any comments, I would love to hear on how I can improve it to be more compatible.
@pombredanne