Add verdicts view filtering capabilities #6062. (#7322)

*  Add verdicts view filtering capabilities #6062.

* Code review changes.

- Refactor tests to be parametrized.
- Pass `_query` to `route_path` in template.
- Remove `is None` from filter query, it adds nothing.

Author: xmunoz

tests/unit/admin/views/test_verdicts.py

@@ -14,37 +14,176 @@
 
 from random import randint
 
-import pretend
 import pytest
 
 from pyramid.httpexceptions import HTTPBadRequest, HTTPNotFound
 
 from warehouse.admin.views import verdicts as views
+from warehouse.malware.models import VerdictClassification, VerdictConfidence
 
-from ....common.db.malware import MalwareVerdictFactory
+from ....common.db.malware import MalwareCheckFactory, MalwareVerdictFactory
 
 
 class TestListVerdicts:
     def test_none(self, db_request):
-        assert views.get_verdicts(db_request) == {"verdicts": []}
+        assert views.get_verdicts(db_request) == {
+            "verdicts": [],
+            "check_names": set(),
+            "classifications": set(["threat", "indeterminate", "benign"]),
+            "confidences": set(["low", "medium", "high"]),
+        }
 
     def test_some(self, db_request):
-        verdicts = [MalwareVerdictFactory.create() for _ in range(10)]
+        check = MalwareCheckFactory.create()
+        verdicts = [MalwareVerdictFactory.create(check=check) for _ in range(10)]
 
-        assert views.get_verdicts(db_request) == {"verdicts": verdicts}
+        assert views.get_verdicts(db_request) == {
+            "verdicts": verdicts,
+            "check_names": set([check.name]),
+            "classifications": set(["threat", "indeterminate", "benign"]),
+            "confidences": set(["low", "medium", "high"]),
+        }
 
     def test_some_with_multipage(self, db_request):
-        verdicts = [MalwareVerdictFactory.create() for _ in range(60)]
+        check1 = MalwareCheckFactory.create()
+        check2 = MalwareCheckFactory.create()
+        verdicts = [MalwareVerdictFactory.create(check=check2) for _ in range(60)]
 
         db_request.GET["page"] = "2"
 
-        assert views.get_verdicts(db_request) == {"verdicts": verdicts[25:50]}
-
-    def test_with_invalid_page(self):
-        request = pretend.stub(params={"page": "not an integer"})
-
+        assert views.get_verdicts(db_request) == {
+            "verdicts": verdicts[25:50],
+            "check_names": set([check1.name, check2.name]),
+            "classifications": set(["threat", "indeterminate", "benign"]),
+            "confidences": set(["low", "medium", "high"]),
+        }
+
+    @pytest.mark.parametrize(
+        "check_name", ["check0", "check1", ""],
+    )
+    def test_check_name_filter(self, db_request, check_name):
+        result_verdicts, all_verdicts = [], []
+        for i in range(3):
+            check = MalwareCheckFactory.create(name="check%d" % i)
+            verdicts = [MalwareVerdictFactory.create(check=check) for _ in range(5)]
+            all_verdicts.extend(verdicts)
+            if check.name == check_name:
+                result_verdicts = verdicts
+
+        # Emptry string
+        if not result_verdicts:
+            result_verdicts = all_verdicts
+
+        response = {
+            "verdicts": result_verdicts,
+            "check_names": set(["check0", "check1", "check2"]),
+            "classifications": set(["threat", "indeterminate", "benign"]),
+            "confidences": set(["low", "medium", "high"]),
+        }
+
+        db_request.GET["check_name"] = check_name
+        assert views.get_verdicts(db_request) == response
+
+    @pytest.mark.parametrize(
+        "classification", ["benign", "indeterminate", "threat", ""],
+    )
+    def test_classification_filter(self, db_request, classification):
+        check1 = MalwareCheckFactory.create()
+        result_verdicts, all_verdicts = [], []
+        for c in VerdictClassification:
+            verdicts = [
+                MalwareVerdictFactory.create(check=check1, classification=c)
+                for _ in range(5)
+            ]
+            all_verdicts.extend(verdicts)
+            if c.value == classification:
+                result_verdicts = verdicts
+
+        # Emptry string
+        if not result_verdicts:
+            result_verdicts = all_verdicts
+
+        db_request.GET["classification"] = classification
+        response = {
+            "verdicts": result_verdicts,
+            "check_names": set([check1.name]),
+            "classifications": set(["threat", "indeterminate", "benign"]),
+            "confidences": set(["low", "medium", "high"]),
+        }
+        assert views.get_verdicts(db_request) == response
+
+    @pytest.mark.parametrize(
+        "confidence", ["low", "medium", "high", ""],
+    )
+    def test_confidence_filter(self, db_request, confidence):
+        check1 = MalwareCheckFactory.create()
+        result_verdicts, all_verdicts = [], []
+        for c in VerdictConfidence:
+            verdicts = [
+                MalwareVerdictFactory.create(check=check1, confidence=c)
+                for _ in range(5)
+            ]
+            all_verdicts.extend(verdicts)
+            if c.value == confidence:
+                result_verdicts = verdicts
+
+        # Emptry string
+        if not result_verdicts:
+            result_verdicts = all_verdicts
+
+        response = {
+            "verdicts": result_verdicts,
+            "check_names": set([check1.name]),
+            "classifications": set(["threat", "indeterminate", "benign"]),
+            "confidences": set(["low", "medium", "high"]),
+        }
+
+        db_request.GET["confidence"] = confidence
+        assert views.get_verdicts(db_request) == response
+
+    @pytest.mark.parametrize(
+        "manually_reviewed", [1, 0],
+    )
+    def test_manually_reviewed_filter(self, db_request, manually_reviewed):
+        check1 = MalwareCheckFactory.create()
+        result_verdicts = [
+            MalwareVerdictFactory.create(
+                check=check1, manually_reviewed=bool(manually_reviewed)
+            )
+            for _ in range(5)
+        ]
+
+        # Create other verdicts to ensure filter works properly
+        for _ in range(10):
+            MalwareVerdictFactory.create(
+                check=check1, manually_reviewed=not bool(manually_reviewed)
+            )
+
+        db_request.GET["manually_reviewed"] = str(manually_reviewed)
+
+        response = {
+            "verdicts": result_verdicts,
+            "check_names": set([check1.name]),
+            "classifications": set(["threat", "indeterminate", "benign"]),
+            "confidences": set(["low", "medium", "high"]),
+        }
+
+        assert views.get_verdicts(db_request) == response
+
+    @pytest.mark.parametrize(
+        "invalid_param",
+        [
+            ("page", "invalid"),
+            ("check_name", "NotACheck"),
+            ("confidence", "NotAConfidence"),
+            ("classification", "NotAClassification"),
+            ("manually_reviewed", "False"),
+        ],
+    )
+    def test_errors(self, db_request, invalid_param):
+        db_request.GET[invalid_param[0]] = invalid_param[1]
         with pytest.raises(HTTPBadRequest):
-            views.get_verdicts(request)
+            views.get_verdicts(db_request)
 
 
 class TestGetVerdict:

warehouse/admin/templates/admin/base.html

@@ -131,7 +131,7 @@
           </a>
         </li>
         <li>
-          <a href="{{ request.route_path('admin.verdicts.list') }}">
+          <a href="{{ request.route_path('admin.verdicts.list', _query={'classification':'threat', 'manually_reviewed':'0'}) }}">
             <i class="fa fa-gavel"></i> <span>Verdicts</span>
           </a>
         </li>

warehouse/admin/templates/admin/malware/verdicts/index.html

@@ -17,20 +17,61 @@
 
 {% block title %}Malware Verdicts{% endblock %}
 
+{% set check_name = request.params.get("check_name") %}
+{% set classification = request.params.get("classification") %}
+{% set confidence = request.params.get("confidence") %}
+{% set manually_reviewed = request.params.get("manually_reviewed") %}
+
 {% block breadcrumb %}
   <li class="active">Verdicts</li>
 {% endblock %}
 
 {% block content %}
 <div class="box box-primary">
+  <div class="box-header with-border">
+    <form class="form-inline">
+      <div class="form-group">
+        <select class="form-control" name="check_name">
+          <option {{ "selected" if check_name is none else "" }} disabled value="">Check Name</option>
+          {% for c in check_names %}
+          <option {{ "selected" if check_name == c else "" }}>{{c}}</option>
+          {% endfor %}
+        </select>
+      </div>
+      <div class="form-group">
+        <select class="form-control" name="classification">
+          <option  {{ "selected" if classification is none else "" }} disabled value="">Classification</option>
+          {% for c in classifications %}
+          <option {{ "selected" if classification == c else "" }}>{{c}}</option>
+          {% endfor %}
+        </select>
+      </div>
+      <div class="form-group">
+        <select class="form-control" name="confidence">
+          <option {{ "selected" if confidence is none else "" }} disabled value="">Confidence</option>
+          {% for c in confidences %}
+          <option {{ "selected" if confidence == c else "" }}>{{c}}</option>
+          {% endfor %}
+        </select>
+      </div>
+      <div class="form-group">
+        <select class="form-control" name="manually_reviewed">
+          <option {{ "selected" if manually_reviewed is none else "" }} disabled value="">Needs Review?</option>
+          <option {{ "selected" if manually_reviewed == "0" else "" }} value="0">Needs Review</option>
+          <option {{ "selected" if manually_reviewed == "1" else "" }} value="1">Already Reviewed</option>
+        </select>
+      </div>
+      <button type="submit" class="btn btn-primary">Filter</button>
+    </form>
+  </div>
   <div class="box-body table-responsive no-padding">
     <table class="table table-hover">
       <tr>
         <th>Object</th>
         <th>Check</th>
         <th>Classification</th>
         <th>Confidence</th>
-        <th>Detail</th>
+        <th>Review</th>
       </tr>
       {% for verdict in verdicts %}
       <tr>

warehouse/admin/views/verdicts.py

@@ -14,7 +14,12 @@
 from pyramid.httpexceptions import HTTPBadRequest, HTTPNotFound
 from pyramid.view import view_config
 
-from warehouse.malware.models import MalwareVerdict
+from warehouse.malware.models import (
+    MalwareCheck,
+    MalwareVerdict,
+    VerdictClassification,
+    VerdictConfidence,
+)
 from warehouse.utils.paginate import paginate_url_factory
 
 
@@ -26,23 +31,23 @@
     uses_session=True,
 )
 def get_verdicts(request):
-    try:
-        page_num = int(request.params.get("page", 1))
-    except ValueError:
-        raise HTTPBadRequest("'page' must be an integer.") from None
-
-    verdicts_query = request.db.query(MalwareVerdict).order_by(
-        MalwareVerdict.run_date.desc()
+    result = {}
+    result["check_names"] = set(
+        [name for (name,) in request.db.query(MalwareCheck.name)]
     )
+    result["classifications"] = set([c.value for c in VerdictClassification])
+    result["confidences"] = set([c.value for c in VerdictConfidence])
+
+    validate_fields(request, result)
 
-    verdicts = SQLAlchemyORMPage(
-        verdicts_query,
-        page=page_num,
+    result["verdicts"] = SQLAlchemyORMPage(
+        generate_query(request.db, request.params),
+        page=int(request.params.get("page", 1)),
         items_per_page=25,
         url_maker=paginate_url_factory(request),
     )
 
-    return {"verdicts": verdicts}
+    return result
 
 
 @view_config(
@@ -59,3 +64,41 @@ def get_verdict(request):
         return {"verdict": verdict}
 
     raise HTTPNotFound
+
+
+def validate_fields(request, validators):
+    try:
+        int(request.params.get("page", 1))
+    except ValueError:
+        raise HTTPBadRequest("'page' must be an integer.") from None
+
+    validators = {**validators, **{"manually_revieweds": set(["0", "1"])}}
+
+    for key, possible_values in validators.items():
+        # Remove the trailing 's'
+        value = request.params.get(key[:-1])
+        additional_values = set([None, ""])
+        if value not in possible_values | additional_values:
+            raise HTTPBadRequest(
+                "Invalid value for '%s': %s." % (key[:-1], value)
+            ) from None
+
+
+def generate_query(db, params):
+    """
+    Returns an SQLAlchemy query wth request params applied as filters.
+    """
+    query = db.query(MalwareVerdict)
+    if params.get("check_name"):
+        query = query.join(MalwareCheck)
+        query = query.filter(MalwareCheck.name == params["check_name"])
+    if params.get("confidence"):
+        query = query.filter(MalwareVerdict.confidence == params["confidence"])
+    if params.get("classification"):
+        query = query.filter(MalwareVerdict.classification == params["classification"])
+    if params.get("manually_reviewed"):
+        query = query.filter(
+            MalwareVerdict.manually_reviewed == bool(int(params["manually_reviewed"]))
+        )
+
+    return query.order_by(MalwareVerdict.run_date.desc())