Bug 1680433 - Fix wrong pushlog from alert dropdown

* pick the correct previous push
* add backend validation
* provide test coverage for it
* ensure data points maintain chronological order

tests/webapp/api/test_performance_alertsummary_api.py

@@ -209,7 +209,7 @@ def test_alert_summaries_put(
     assert PerformanceAlertSummary.objects.get(id=1).assignee == test_user
 
 
-def test_alert_summary_post(
+def test_auth_for_alert_summary_post(
     client,
     test_repository,
     test_issue_tracker,
@@ -218,8 +218,6 @@ def test_alert_summary_post(
     test_user,
     test_sheriff,
 ):
-    # this blob should be sufficient to create a new alert summary (assuming
-    # the user of this API is authorized to do so!)
     post_blob = {
         'repository_id': test_repository.id,
         'framework_id': test_perf_signature.framework.id,
@@ -238,9 +236,25 @@ def test_alert_summary_post(
     assert resp.status_code == 403
     assert PerformanceAlertSummary.objects.count() == 0
 
+
+def test_alert_summary_post(
+    authorized_sheriff_client,
+    test_repository,
+    test_issue_tracker,
+    push_stored,
+    test_perf_signature,
+    test_user,
+    test_sheriff,
+):
+    post_blob = {
+        'repository_id': test_repository.id,
+        'framework_id': test_perf_signature.framework.id,
+        'prev_push_id': 1,
+        'push_id': 2,
+    }
+
     # verify that we succeed if authenticated + staff
-    client.force_authenticate(user=test_sheriff)
-    resp = client.post(reverse('performance-alert-summaries-list'), post_blob)
+    resp = authorized_sheriff_client.post(reverse('performance-alert-summaries-list'), post_blob)
     assert resp.status_code == 200
 
     assert PerformanceAlertSummary.objects.count() == 1
@@ -253,11 +267,35 @@ def test_alert_summary_post(
 
     # verify that we don't create a new performance alert summary if one
     # already exists (but also don't throw an error)
-    resp = client.post(reverse('performance-alert-summaries-list'), post_blob)
+    resp = authorized_sheriff_client.post(reverse('performance-alert-summaries-list'), post_blob)
     assert resp.status_code == 200
     assert PerformanceAlertSummary.objects.count() == 1
 
 
+def test_push_range_validation_for_alert_summary_post(
+    authorized_sheriff_client,
+    test_repository,
+    test_issue_tracker,
+    push_stored,
+    test_perf_signature,
+    test_user,
+    test_sheriff,
+):
+    identical_push = 1
+    post_blob = {
+        'repository_id': test_repository.id,
+        'framework_id': test_perf_signature.framework.id,
+        'prev_push_id': identical_push,
+        'push_id': identical_push,
+    }
+
+    # verify that we succeed if authenticated + staff
+    resp = authorized_sheriff_client.post(reverse('performance-alert-summaries-list'), post_blob)
+    assert resp.status_code == 400
+
+    assert PerformanceAlertSummary.objects.count() == 0
+
+
 @pytest.mark.parametrize(
     'modification', [{'notes': 'human created notes'}, {'bug_number': 123456, 'issue_tracker': 1}]
 )

tests/webapp/api/test_performance_data_api.py

@@ -523,6 +523,28 @@ def test_perf_summary(client, test_perf_signature, test_perf_data):
     assert resp2.json() == expected
 
 
+def test_data_points_from_same_push_are_ordered_chronologically(
+    client, test_perf_signature, test_perf_data
+):
+    """
+    The chronological order for data points associated to a single push
+    is based upon the order of their related job. If related jobs are
+    ordered, the data points are considered ordered.
+
+    As job ids are auto incremented, older jobs have smaller ids than newer ones.
+    Thus, these ids are sufficient to check for chronological order.
+    """
+    query_params = '?repository={}&framework={}&interval=172800&no_subtests=true&startday=2013-11-01T23%3A28%3A29&endday=2013-11-30T23%3A28%3A29'.format(
+        test_perf_signature.repository.name, test_perf_signature.framework_id
+    )
+
+    response = client.get(reverse('performance-summary') + query_params)
+    assert response.status_code == 200
+
+    job_ids = response.json()[0]['job_ids']
+    assert job_ids == sorted(job_ids)
+
+
 def test_no_retriggers_perf_summary(
     client, push_stored, test_perf_signature, test_perf_signature_2, test_perf_data
 ):

treeherder/webapp/api/performance_data.py

@@ -457,6 +457,11 @@ class PerformanceAlertSummaryViewSet(viewsets.ModelViewSet):
     def create(self, request, *args, **kwargs):
         data = request.data
 
+        if data['push_id'] == data['prev_push_id']:
+            return Response(
+                "IDs of push & previous push cannot be identical", status=HTTP_400_BAD_REQUEST
+            )
+
         alert_summary, _ = PerformanceAlertSummary.objects.get_or_create(
             repository_id=data['repository_id'],
             framework=PerformanceFramework.objects.get(id=data['framework_id']),

ui/perfherder/graphs/GraphTooltip.jsx

@@ -80,13 +80,16 @@ const GraphTooltip = ({
   let prevRevision;
   let prevPushId;
   let pushUrl;
-  const firstTriggerIndex = testDetails.data.findIndex(
+  const originalDataPointIdx = testDetails.data.findIndex(
     (e) => e.revision === dataPointDetails.revision,
   );
-  if (prevFlotDataPointIndex !== -1 && firstTriggerIndex > 0) {
-    prevRevision = testDetails.data[firstTriggerIndex - 1].revision;
-    prevPushId = testDetails.data[prevFlotDataPointIndex].pushId;
+
+  if (prevFlotDataPointIndex !== -1 && originalDataPointIdx > 0) {
+    const prevDataPointIdx = originalDataPointIdx - 1;
     const repoModel = new RepositoryModel(repositoryName);
+
+    prevRevision = testDetails.data[prevDataPointIdx].revision;
+    prevPushId = testDetails.data[prevDataPointIdx].pushId;
     pushUrl = repoModel.getPushLogRangeHref({
       fromchange: prevRevision,
       tochange: dataPointDetails.revision,