From b1fe4a57579edc5fdfaced09b2f9ee378b6599ee Mon Sep 17 00:00:00 2001 From: Florin Bilt Date: Thu, 27 Mar 2025 16:24:32 +0200 Subject: [PATCH 1/7] migrate DB --- ...lertsummary_original_prev_push_and_more.py | 64 +++++++++++++++++++ treeherder/perf/models.py | 6 ++ .../webapp/api/performance_serializers.py | 10 ++- 3 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 treeherder/perf/migrations/0058_performancealertsummary_original_prev_push_and_more.py 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", From ccc0fcddf05ba1f306d104cef05123eb7c2f47c5 Mon Sep 17 00:00:00 2001 From: Florin Bilt Date: Thu, 3 Apr 2025 14:01:25 +0300 Subject: [PATCH 2/7] Frontend + test backend + fix display edit revisions button --- .../api/test_performance_alertsummary_api.py | 31 ++++ ui/perfherder/alerts/AlertHeader.jsx | 161 +++++++++++++----- ui/perfherder/alerts/AlertTable.jsx | 6 +- 3 files changed, 149 insertions(+), 49 deletions(-) diff --git a/tests/webapp/api/test_performance_alertsummary_api.py b/tests/webapp/api/test_performance_alertsummary_api.py index bc80cf84330..1ad3c259b2f 100644 --- a/tests/webapp/api/test_performance_alertsummary_api.py +++ b/tests/webapp/api/test_performance_alertsummary_api.py @@ -240,6 +240,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 can 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 ): diff --git a/ui/perfherder/alerts/AlertHeader.jsx b/ui/perfherder/alerts/AlertHeader.jsx index 23b418f2c3d..abac149982c 100644 --- a/ui/perfherder/alerts/AlertHeader.jsx +++ b/ui/perfherder/alerts/AlertHeader.jsx @@ -32,19 +32,35 @@ 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 handleEditMode = () => { - setNewRevision(''); + setnewRevisionTo(''); + setnewRevisionFrom(''); setInEditMode(true); }; - const handleRevisionChange = (event) => { - setNewRevision(event.target.value); + const handleRevisionChange = (pushORfrom) => (event) => { + if (pushORfrom === 'push') 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 +68,10 @@ const AlertHeader = ({ }); return; } - const response = await changeRevision(trimmedRevision); + const response = await changeRevision( + trimmedRevisionTo, + trimmedRevisionFrom, + ); if (!response.failureStatus) { setInEditMode(false); } @@ -67,7 +86,9 @@ const AlertHeader = ({ return issueTrackerUrl + alertSummary.bug_number; }; const handleRevertRevision = async () => { - await changeRevision(alertSummary.original_revision); + setnewRevisionTo(alertSummary.original_revision); + setnewRevisionFrom(alertSummary.original_prev_push_revision); + setInEditMode(true); }; const bugNumber = alertSummary.bug_number ? `Bug ${alertSummary.bug_number}` @@ -111,39 +132,8 @@ const AlertHeader = ({ - - {inEditMode ? ( - - { - if (event.key === 'Enter') { - event.preventDefault(); - saveRevision(); - } - if (event.key === 'Escape') cancelEditMode(); - }} - autoFocus - /> - - - - ) : ( + {user.isStaff && ( + - )} - + + )} {user.isStaff && - alertSummary.original_revision !== alertSummary.revision && ( + (alertSummary.original_revision !== alertSummary.revision || + alertSummary.original_prev_push_revision !== + alertSummary.prev_push_revision) && ( )} @@ -247,6 +239,83 @@ const AlertHeader = ({ )} + {inEditMode && ( +
+ + + From: + + + + {`${alertSummary.prev_push_revision.slice(0, 12)}`}{' '} + + + + + + { + if (event.key === 'Enter') { + event.preventDefault(); + saveRevision(); + } + if (event.key === 'Escape') cancelEditMode(); + }} + autoFocus + /> + + + + + + To: + + + {formattedSummaryRevision} + + + + { + if (event.key === 'Enter') { + event.preventDefault(); + saveRevision(); + } + if (event.key === 'Escape') cancelEditMode(); + }} + autoFocus + /> + + + + + + + + + +
+ )} ); }; diff --git a/ui/perfherder/alerts/AlertTable.jsx b/ui/perfherder/alerts/AlertTable.jsx index 3c5be4c327a..9ced0aa9a2d 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) { From 049ec0f4a81d53449e27bdcaedd7792373323927 Mon Sep 17 00:00:00 2001 From: Florin Bilt Date: Thu, 3 Apr 2025 14:18:47 +0300 Subject: [PATCH 3/7] fix error case --- ui/perfherder/alerts/AlertTable.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/perfherder/alerts/AlertTable.jsx b/ui/perfherder/alerts/AlertTable.jsx index 9ced0aa9a2d..371d6484a59 100644 --- a/ui/perfherder/alerts/AlertTable.jsx +++ b/ui/perfherder/alerts/AlertTable.jsx @@ -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})`], }); } From 4b1a76c19ad42d26e75e570e83bb6b625b6004eb Mon Sep 17 00:00:00 2001 From: Florin Bilt Date: Thu, 3 Apr 2025 14:41:58 +0300 Subject: [PATCH 4/7] fix tests --- tests/webapp/api/test_performance_alertsummary_api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/webapp/api/test_performance_alertsummary_api.py b/tests/webapp/api/test_performance_alertsummary_api.py index 1ad3c259b2f..5aa19f68cdb 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 From d846a8eed4238b00eaf0d0b5c3aa2f9a96842fb6 Mon Sep 17 00:00:00 2001 From: Florin Bilt Date: Mon, 7 Apr 2025 17:24:47 +0300 Subject: [PATCH 5/7] update design frontend --- ui/perfherder/alerts/AlertHeader.jsx | 106 ++++++++++++++------------- 1 file changed, 56 insertions(+), 50 deletions(-) diff --git a/ui/perfherder/alerts/AlertHeader.jsx b/ui/perfherder/alerts/AlertHeader.jsx index abac149982c..ad9bcd004d4 100644 --- a/ui/perfherder/alerts/AlertHeader.jsx +++ b/ui/perfherder/alerts/AlertHeader.jsx @@ -85,10 +85,9 @@ const AlertHeader = ({ ); return issueTrackerUrl + alertSummary.bug_number; }; - const handleRevertRevision = async () => { - setnewRevisionTo(alertSummary.original_revision); - setnewRevisionFrom(alertSummary.original_prev_push_revision); - setInEditMode(true); + const handleRevertRevision = (pushORfrom) => () => { + if (pushORfrom === 'push') setnewRevisionTo(alertSummary.original_revision); + else setnewRevisionFrom(alertSummary.original_prev_push_revision); }; const bugNumber = alertSummary.bug_number ? `Bug ${alertSummary.bug_number}` @@ -103,7 +102,7 @@ const AlertHeader = ({ - + - - - PerfCompare comparison - - {user.isStaff && ( @@ -145,16 +129,6 @@ const AlertHeader = ({ )} - {user.isStaff && - (alertSummary.original_revision !== alertSummary.revision || - alertSummary.original_prev_push_revision !== - alertSummary.prev_push_revision) && ( - - - - )} + + + PerfCompare comparison + + {(alertSummary.original_revision !== alertSummary.revision || + alertSummary.original_prev_push_revision !== + alertSummary.prev_push_revision) && ( + Revisions have been modified. + )} + {performanceTags.length > 0 && ( @@ -242,10 +236,10 @@ const AlertHeader = ({ {inEditMode && (
- - From: + + Current From: - + {`${alertSummary.prev_push_revision.slice(0, 12)}`}{' '} @@ -257,23 +251,29 @@ const AlertHeader = ({ value={newRevisionFrom} placeholder="Enter desired revision" onChange={handleRevisionChange('from')} - onKeyDown={(event) => { - if (event.key === 'Enter') { - event.preventDefault(); - saveRevision(); - } - if (event.key === 'Escape') cancelEditMode(); - }} autoFocus /> + + + - - To: + + Current To: - + {formattedSummaryRevision} @@ -282,17 +282,22 @@ const AlertHeader = ({ value={newRevisionTo} placeholder="Enter desired revision" onChange={handleRevisionChange('push')} - onKeyDown={(event) => { - if (event.key === 'Enter') { - event.preventDefault(); - saveRevision(); - } - if (event.key === 'Escape') cancelEditMode(); - }} autoFocus /> + + + @@ -300,6 +305,7 @@ const AlertHeader = ({ color="primary" className="ml-1" size="xs" + disabled={newRevisionTo === '' && newRevisionFrom === ''} onClick={saveRevision} > Save From 5826f44c497e28fc11f7d236e663886f692b0244 Mon Sep 17 00:00:00 2001 From: Florin Bilt Date: Thu, 10 Apr 2025 15:37:45 +0300 Subject: [PATCH 6/7] fix --- .../api/test_performance_alertsummary_api.py | 4 ++-- ui/perfherder/alerts/AlertHeader.jsx | 14 ++++++++------ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/webapp/api/test_performance_alertsummary_api.py b/tests/webapp/api/test_performance_alertsummary_api.py index 5aa19f68cdb..82a571e078b 100644 --- a/tests/webapp/api/test_performance_alertsummary_api.py +++ b/tests/webapp/api/test_performance_alertsummary_api.py @@ -256,7 +256,7 @@ def test_performance_alert_summary_change_from_revision( assert resp.status_code == 200 assert PerformanceAlertSummary.objects.get(id=1).prev_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/", {"prev_push_revision": "no-push-revision"}, @@ -287,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/ui/perfherder/alerts/AlertHeader.jsx b/ui/perfherder/alerts/AlertHeader.jsx index ad9bcd004d4..ad4420d6cfe 100644 --- a/ui/perfherder/alerts/AlertHeader.jsx +++ b/ui/perfherder/alerts/AlertHeader.jsx @@ -42,8 +42,9 @@ const AlertHeader = ({ setnewRevisionFrom(''); setInEditMode(true); }; - const handleRevisionChange = (pushORfrom) => (event) => { - if (pushORfrom === 'push') setnewRevisionTo(event.target.value); + const handleRevisionChange = (revisionType) => (event) => { + // revisionType can only to be "to" or "from" + if (revisionType === 'to') setnewRevisionTo(event.target.value); else setnewRevisionFrom(event.target.value); }; const saveRevision = async () => { @@ -85,8 +86,9 @@ const AlertHeader = ({ ); return issueTrackerUrl + alertSummary.bug_number; }; - const handleRevertRevision = (pushORfrom) => () => { - if (pushORfrom === 'push') setnewRevisionTo(alertSummary.original_revision); + const handleRevertRevision = (revisionType) => () => { + // revisionType can only to be "to" or "from" + if (revisionType === 'to') setnewRevisionTo(alertSummary.original_revision); else setnewRevisionFrom(alertSummary.original_prev_push_revision); }; const bugNumber = alertSummary.bug_number @@ -281,7 +283,7 @@ const AlertHeader = ({ @@ -293,7 +295,7 @@ const AlertHeader = ({ disabled={ alertSummary.original_revision === alertSummary.revision } - onClick={handleRevertRevision('push')} + onClick={handleRevertRevision('to')} > Reset Revision From aa0b99b49268c09ceca64d9be50d500993915653 Mon Sep 17 00:00:00 2001 From: Florin Bilt Date: Mon, 14 Apr 2025 18:22:45 +0300 Subject: [PATCH 7/7] fix --- ui/perfherder/alerts/AlertHeader.jsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ui/perfherder/alerts/AlertHeader.jsx b/ui/perfherder/alerts/AlertHeader.jsx index ad4420d6cfe..39b1240739d 100644 --- a/ui/perfherder/alerts/AlertHeader.jsx +++ b/ui/perfherder/alerts/AlertHeader.jsx @@ -36,6 +36,7 @@ const AlertHeader = ({ const [newRevisionFrom, setnewRevisionFrom] = useState( alertSummary.prev_push_revision, ); + const revisionToType = 'to'; const handleEditMode = () => { setnewRevisionTo(''); @@ -43,8 +44,8 @@ const AlertHeader = ({ setInEditMode(true); }; const handleRevisionChange = (revisionType) => (event) => { - // revisionType can only to be "to" or "from" - if (revisionType === 'to') setnewRevisionTo(event.target.value); + // revisionType can only be "to" or "from" + if (revisionType === revisionToType) setnewRevisionTo(event.target.value); else setnewRevisionFrom(event.target.value); }; const saveRevision = async () => { @@ -87,8 +88,9 @@ const AlertHeader = ({ return issueTrackerUrl + alertSummary.bug_number; }; const handleRevertRevision = (revisionType) => () => { - // revisionType can only to be "to" or "from" - if (revisionType === 'to') setnewRevisionTo(alertSummary.original_revision); + // 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