diff --git a/api/features_api.py b/api/features_api.py index 1535b9fc1581..aeb369bb5bf3 100644 --- a/api/features_api.py +++ b/api/features_api.py @@ -24,6 +24,7 @@ from framework import permissions from framework import rediscache from framework import users +from internals import attachments from internals.enterprise_helpers import * from internals.core_enums import * from internals.core_models import FeatureEntry, MilestoneSet, Stage @@ -252,6 +253,10 @@ def _patch_update_special_fields( feature.outstanding_notifications = 0 has_updated = True + if 'screenshot_links' in feature_changes: + attachments.delete_orphan_attachments( + feature.key.integer_id(), feature_changes['screenshot_links']) + # Set enterprise first notification milestones. if is_update_first_notification_milestone(feature, feature_changes): feature.first_enterprise_notification_milestone = int(feature_changes['first_enterprise_notification_milestone']) diff --git a/internals/attachments.py b/internals/attachments.py index 9c702c34e04b..470fc9878bd8 100644 --- a/internals/attachments.py +++ b/internals/attachments.py @@ -137,6 +137,16 @@ def mark_attachment_deleted(attachment: Attachment) -> None: attachment.put() +def delete_orphan_attachments(feature_id: int, new_value: str) -> None: + """Soft delete attachments on a feature that are no longer referenced.""" + attachments = Attachment.query( + Attachment.feature_id == feature_id).fetch() + for a in attachments: + uri = 'feature/%r/attachment/%r' % (feature_id, a.key.integer_id()) + if uri not in new_value: + mark_attachment_deleted(a) + + def get_thumbnail(attachment_id: int) -> Thumbnail|None: """Return a Thumbnail, if it exsits.""" thumbnail = Thumbnail.query( diff --git a/internals/attachments_test.py b/internals/attachments_test.py index 6b9b355605aa..b4357e49aa53 100644 --- a/internals/attachments_test.py +++ b/internals/attachments_test.py @@ -131,3 +131,33 @@ def test_get_attachment_url__GAE(self): actual, (f'https://img{digits}-dot-appid.appspot.com/' f'feature/{feature_id}/attachment/{attach_id}')) + + def test_delete_orphan_attachments__none(self): + """When a feature has no attachments, we do nothing without crashing.""" + attachments.delete_orphan_attachments(self.feature_id, '') + + def test_delete_orphan_attachments__maintained(self): + """A feature has an attachment and the links field keeps it: it stays.""" + feature_id = self.feature_id + stored = attachments.store_attachment( + feature_id, b'test content', 'text/plain') + with_link = attachments.get_attachment_url(stored) + + attachments.delete_orphan_attachments(feature_id, with_link) + + all_attach = attachments.Attachment.query().fetch() + self.assertEqual(len(all_attach), 1) + self.assertFalse(all_attach[0].is_deleted) + + def test_delete_orphan_attachments__deleted(self): + """A feature has an attachment and the links field drops it: deleted.""" + feature_id = self.feature_id + stored = attachments.store_attachment( + feature_id, b'test content', 'text/plain') + without_link = '' + + attachments.delete_orphan_attachments(feature_id, without_link) + + all_attach = attachments.Attachment.query().fetch() + self.assertEqual(len(all_attach), 1) + self.assertTrue(all_attach[0].is_deleted) diff --git a/internals/reminders_test.py b/internals/reminders_test.py index 592ae048d06e..1724a04091e5 100644 --- a/internals/reminders_test.py +++ b/internals/reminders_test.py @@ -13,6 +13,7 @@ # limitations under the License. import testing_config # Must be imported before the module under test. +import re import flask import settings from datetime import datetime @@ -629,6 +630,16 @@ def test_get_template_data__resolve_overdue_unassigned(self, mock_now_utc): expected = {'message': expected_message} self.assertEqual(actual, expected) + def assert_equal_ignoring_ids(self, expected, actual): + """Compare two strings, but ignore differences in NDB keys.""" + feature_re = re.compile(r'/feature/\d+') + gate_re = re.compile(r'\?gate=\d+') + expected = feature_re.sub(expected, '/feature/ID') + expected = gate_re.sub(expected, '?gate=ID') + actual = feature_re.sub(actual, '/feature/ID') + actual = gate_re.sub(actual, '?gate=ID') + self.assertMultiLineEqual(expected, actual) + @mock.patch('internals.slo.now_utc') def test_get_template_data__old_reviews(self, mock_now_utc): """More time has passed. We don't keep reminding.""" @@ -660,7 +671,7 @@ def test_build_gate_email_tasks__initial_due(self): self.assertEqual('Review due for: feature one', task['subject']) self.assertEqual(None, task['reply_to']) # TESTDATA.make_golden(task['html'], 'test_build_gate_email_tasks__initial_due.html') - self.assertMultiLineEqual( + self.assert_equal_ignoring_ids( TESTDATA['test_build_gate_email_tasks__initial_due.html'], task['html']) def test_build_gate_email_tasks__initial_overdue(self): @@ -679,7 +690,7 @@ def test_build_gate_email_tasks__initial_overdue(self): self.assertEqual('ESCALATED: Review due for: feature one', task['subject']) self.assertEqual(None, task['reply_to']) # TESTDATA.make_golden(task['html'], 'test_build_gate_email_tasks__initial_overdue.html') - self.assertMultiLineEqual( + self.assert_equal_ignoring_ids( TESTDATA['test_build_gate_email_tasks__initial_overdue.html'], task['html']) def test_build_gate_email_tasks__resolution_due(self): @@ -698,7 +709,7 @@ def test_build_gate_email_tasks__resolution_due(self): self.assertEqual('Review due for: feature one', task['subject']) self.assertEqual(None, task['reply_to']) # TESTDATA.make_golden(task['html'], 'test_build_gate_email_tasks__resolution_due.html') - self.assertMultiLineEqual( + self.assert_equal_ignoring_ids( TESTDATA['test_build_gate_email_tasks__resolution_due.html'], task['html']) def test_build_gate_email_tasks__resolution_overdue(self): @@ -717,5 +728,5 @@ def test_build_gate_email_tasks__resolution_overdue(self): self.assertEqual('ESCALATED: Review due for: feature one', task['subject']) self.assertEqual(None, task['reply_to']) # TESTDATA.make_golden(task['html'], 'test_build_gate_email_tasks__resolution_overdue.html') - self.assertMultiLineEqual( + self.assert_equal_ignoring_ids( TESTDATA['test_build_gate_email_tasks__resolution_overdue.html'], task['html'])