diff --git a/tests/webapp/api/test_performance_alertsummary_api.py b/tests/webapp/api/test_performance_alertsummary_api.py index bc80cf84330..82a571e078b 100644 --- a/tests/webapp/api/test_performance_alertsummary_api.py +++ b/tests/webapp/api/test_performance_alertsummary_api.py @@ -93,6 +93,7 @@ def test_alert_summaries_get( "revision", "push_timestamp", "prev_push_revision", + "original_prev_push_revision", "performance_tags", } assert len(resp.json()["results"][0]["alerts"]) == 1 @@ -173,6 +174,7 @@ def test_alert_summaries_get_onhold( "revision", "push_timestamp", "prev_push_revision", + "original_prev_push_revision", "performance_tags", } assert len(resp.json()["results"][0]["alerts"]) == 1 @@ -240,6 +242,37 @@ def test_alert_summaries_put( assert PerformanceAlertSummary.objects.get(id=1).assignee == test_user +def test_performance_alert_summary_change_from_revision( + client, test_perf_alert_summary, test_sheriff, test_push +): + client.force_authenticate(user=test_sheriff) + + # verify we can set revision + assert PerformanceAlertSummary.objects.get(id=1).prev_push.revision != test_push.revision + resp = client.put( + reverse("performance-alert-summaries-list") + "1/", + {"prev_push_revision": test_push.revision}, + ) + assert resp.status_code == 200 + assert PerformanceAlertSummary.objects.get(id=1).prev_push.revision == test_push.revision + + # verify we cannot set non-existing revision + resp = client.put( + reverse("performance-alert-summaries-list") + "1/", + {"prev_push_revision": "no-push-revision"}, + ) + assert resp.status_code == 400 + + # revert revision + original_revision = PerformanceAlertSummary.objects.get(id=1).original_prev_push.revision + resp = client.put( + reverse("performance-alert-summaries-list") + "1/", + {"prev_push_revision": original_revision}, + ) + assert resp.status_code == 200 + assert PerformanceAlertSummary.objects.get(id=1).prev_push.revision == original_revision + + def test_performance_alert_summary_change_revision( client, test_perf_alert_summary, test_sheriff, test_push ): @@ -254,7 +287,7 @@ def test_performance_alert_summary_change_revision( assert resp.status_code == 200 assert PerformanceAlertSummary.objects.get(id=1).push.revision == test_push.revision - # verify we can set non-existing revision + # verify we cannot set non-existing revision resp = client.put( reverse("performance-alert-summaries-list") + "1/", {"revision": "no-push-revision"}, diff --git a/treeherder/perf/migrations/0058_performancealertsummary_original_prev_push_and_more.py b/treeherder/perf/migrations/0058_performancealertsummary_original_prev_push_and_more.py new file mode 100644 index 00000000000..a7a1370a185 --- /dev/null +++ b/treeherder/perf/migrations/0058_performancealertsummary_original_prev_push_and_more.py @@ -0,0 +1,64 @@ +# Generated by Django 5.1.4 on 2025-03-26 17:29 + +import django.db.models.deletion +from django.db import migrations, models + + +def update_summary_original_push(apps, schema_editor): + performancealertsummary = apps.get_model('perf', 'PerformanceAlertSummary') + for alert in performancealertsummary.objects.all(): + alert.original_prev_push = alert.prev_push + alert.save() + + performancealertsummarytesting = apps.get_model('perf', 'PerformanceAlertSummaryTesting') + for alert in performancealertsummarytesting.objects.all(): + alert.original_prev_push = alert.prev_push + alert.save() + + performancetetemetryalertsummary = apps.get_model('perf', 'PerformanceTelemetryAlertSummary') + for alert in performancetetemetryalertsummary.objects.all(): + alert.original_prev_push = alert.prev_push + alert.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ("perf", "0057_performancealert_confidence_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="performancealertsummary", + name="original_prev_push", + field=models.ForeignKey( + default=None, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="+", + to="model.push", + ), + ), + migrations.AddField( + model_name="performancealertsummarytesting", + name="original_prev_push", + field=models.ForeignKey( + default=None, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="+", + to="model.push", + ), + ), + migrations.AddField( + model_name="performancetelemetryalertsummary", + name="original_prev_push", + field=models.ForeignKey( + default=None, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="+", + to="model.push", + ), + ), + ] diff --git a/treeherder/perf/models.py b/treeherder/perf/models.py index fabe010e53e..f671f8f6a34 100644 --- a/treeherder/perf/models.py +++ b/treeherder/perf/models.py @@ -311,6 +311,9 @@ class PerformanceAlertSummaryBase(models.Model): framework = models.ForeignKey(PerformanceFramework, on_delete=models.CASCADE) prev_push = models.ForeignKey(Push, on_delete=models.CASCADE, related_name="+") + original_prev_push = models.ForeignKey( + Push, on_delete=models.CASCADE, related_name="+", null=True, default=None + ) push = models.ForeignKey(Push, on_delete=models.CASCADE, related_name="+") original_push = models.ForeignKey( Push, on_delete=models.CASCADE, related_name="+", null=True, default=None @@ -389,6 +392,9 @@ def save(self, *args, update_fields=None, **kwargs): if not self.original_push: self.original_push = self.push + if not self.original_prev_push: + self.original_prev_push = self.prev_push + super().save(*args, update_fields=update_fields, **kwargs) self.__prev_bug_number = self.bug_number diff --git a/treeherder/webapp/api/performance_serializers.py b/treeherder/webapp/api/performance_serializers.py index 7101e78798a..9e68e467f82 100644 --- a/treeherder/webapp/api/performance_serializers.py +++ b/treeherder/webapp/api/performance_serializers.py @@ -283,7 +283,14 @@ class PerformanceAlertSummarySerializer(serializers.ModelSerializer): ) push_timestamp = TimestampField(source="push", read_only=True) prev_push_revision = serializers.SlugRelatedField( - read_only=True, slug_field="revision", source="prev_push" + read_only=False, + slug_field="revision", + source="prev_push", + required=False, + queryset=Push.objects.all(), + ) + original_prev_push_revision = serializers.SlugRelatedField( + read_only=True, slug_field="revision", source="original_prev_push" ) assignee_username = serializers.SlugRelatedField( slug_field="username", @@ -333,6 +340,7 @@ class Meta: "revision", "push_timestamp", "prev_push_revision", + "original_prev_push_revision", "assignee_username", "assignee_email", "performance_tags", diff --git a/ui/perfherder/alerts/AlertHeader.jsx b/ui/perfherder/alerts/AlertHeader.jsx index 23b418f2c3d..39b1240739d 100644 --- a/ui/perfherder/alerts/AlertHeader.jsx +++ b/ui/perfherder/alerts/AlertHeader.jsx @@ -32,19 +32,37 @@ const AlertHeader = ({ updateViewState, }) => { const [inEditMode, setInEditMode] = useState(false); - const [newRevision, setNewRevision] = useState(alertSummary.revision); + const [newRevisionTo, setnewRevisionTo] = useState(alertSummary.revision); + const [newRevisionFrom, setnewRevisionFrom] = useState( + alertSummary.prev_push_revision, + ); + const revisionToType = 'to'; const handleEditMode = () => { - setNewRevision(''); + setnewRevisionTo(''); + setnewRevisionFrom(''); setInEditMode(true); }; - const handleRevisionChange = (event) => { - setNewRevision(event.target.value); + const handleRevisionChange = (revisionType) => (event) => { + // revisionType can only be "to" or "from" + if (revisionType === revisionToType) setnewRevisionTo(event.target.value); + else setnewRevisionFrom(event.target.value); }; const saveRevision = async () => { + const trimmedRevisionTo = + newRevisionTo.trim() === '' + ? alertSummary.revision + : newRevisionTo.trim(); + const trimmedRevisionFrom = + newRevisionFrom.trim() === '' + ? alertSummary.prev_push_revision + : newRevisionFrom.trim(); + const longHashMatch = /\b[a-f0-9]{40}\b/; - const trimmedRevision = newRevision.trim(); - if (!longHashMatch.test(trimmedRevision)) { + if ( + !longHashMatch.test(trimmedRevisionTo) || + !longHashMatch.test(trimmedRevisionFrom) + ) { updateViewState({ errorMessages: [ `Invalid Revision format, expected a 40 character hash.`, @@ -52,7 +70,10 @@ const AlertHeader = ({ }); return; } - const response = await changeRevision(trimmedRevision); + const response = await changeRevision( + trimmedRevisionTo, + trimmedRevisionFrom, + ); if (!response.failureStatus) { setInEditMode(false); } @@ -66,8 +87,11 @@ const AlertHeader = ({ ); return issueTrackerUrl + alertSummary.bug_number; }; - const handleRevertRevision = async () => { - await changeRevision(alertSummary.original_revision); + const handleRevertRevision = (revisionType) => () => { + // revisionType can only be "to" or "from" + if (revisionType === revisionToType) + setnewRevisionTo(alertSummary.original_revision); + else setnewRevisionFrom(alertSummary.original_prev_push_revision); }; const bugNumber = alertSummary.bug_number ? `Bug ${alertSummary.bug_number}` @@ -82,7 +106,7 @@ const AlertHeader = ({ - + - - - PerfCompare comparison - - - - {inEditMode ? ( - - { - if (event.key === 'Enter') { - event.preventDefault(); - saveRevision(); - } - if (event.key === 'Escape') cancelEditMode(); - }} - autoFocus - /> - - - - ) : ( + {user.isStaff && ( + - )} - - {user.isStaff && - alertSummary.original_revision !== alertSummary.revision && ( - - - - )} + + )} + + + PerfCompare comparison + + {(alertSummary.original_revision !== alertSummary.revision || + alertSummary.original_prev_push_revision !== + alertSummary.prev_push_revision) && ( + Revisions have been modified. + )} + {performanceTags.length > 0 && ( @@ -247,6 +237,95 @@ const AlertHeader = ({ )} + {inEditMode && ( +
+ + + Current From: + + + + {`${alertSummary.prev_push_revision.slice(0, 12)}`}{' '} + + + + + + + + + + + + + + + Current To: + + + {formattedSummaryRevision} + + + + + + + + + + + + + + + + +
+ )} ); }; diff --git a/ui/perfherder/alerts/AlertTable.jsx b/ui/perfherder/alerts/AlertTable.jsx index 3c5be4c327a..371d6484a59 100644 --- a/ui/perfherder/alerts/AlertTable.jsx +++ b/ui/perfherder/alerts/AlertTable.jsx @@ -236,16 +236,16 @@ export default class AlertTable extends React.Component { return { failureStatus }; }; - changeRevision = async (newRevision) => { + changeRevision = async (newRevisionTo, newRevisionFrom) => { const { updateAlertSummary, updateViewState, fetchAlertSummaries, } = this.props; const { alertSummary } = this.state; - const { data, failureStatus } = await updateAlertSummary(alertSummary.id, { - revision: newRevision, + revision: newRevisionTo, + prev_push_revision: newRevisionFrom, }); if (!failureStatus) { @@ -253,7 +253,7 @@ export default class AlertTable extends React.Component { fetchAlertSummaries(alertSummary.id); } else { updateViewState({ - errorMessages: [`Failed to set revision "${newRevision}". (${data})`], + errorMessages: [`Failed to set revisions. (${data})`], }); }