Soft-delete attachments when link is removed.

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'])

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(

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)

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'])