repair malformed media columns

Author: rajpatel24

src/formpack/utils/bugfix.py

@@ -5,13 +5,15 @@
 from ..constants import UNTRANSLATED
 
 
-def repair_file_column_content_in_place(content) -> bool:
+def repair_media_column_content_in_place(content) -> bool:
     """
     #321 introduced a bug where the `file` column gets renamed to `media::file`
     and treated as a translatable column (see #322). This function repairs that
     damage.
 
-    This function is intended to be run by KPI, which should it to correct
+    It also handles other `media::<type>` columns similarly affected by the same bug.
+
+    This function is intended to be run by KPI, which should use it to correct
     `Asset` and `AssetVersion` content.
 
     Returns `True` if any change was made
@@ -27,47 +29,96 @@ def repair_file_column_content_in_place(content) -> bool:
         # Do not proceed if content is incomplete
         return False
 
-    try:
-        updates['translated'].remove('media::file')
-    except ValueError:
-        # The invalid column `media::file` inside `translated` is a hallmark of
-        # the problem this method is intended to fix. Do not proceed if it was
-        # not found
+    media_tokens = [
+        t for t in updates['translated'] if isinstance(t, str) and t.startswith('media::')
+    ]
+    if not media_tokens:
+        # No media::<type> in translated, nothing to do
         return False
 
+    # Remove the media tokens from translated
+    for token in media_tokens:
+        try:
+            updates['translated'].remove(token)
+        except ValueError:
+            pass
+
     max_label_list_length = 0
     any_row_fixed = False
     for row in updates['survey']:
         max_label_list_length = max(
             max_label_list_length, len(row.get('label', []))
         )
-        bad_file_col = row.get('media::file')
-        if not bad_file_col:
-            continue
-        if not isinstance(bad_file_col, list):
-            # All problems of our own making (#322) will result in
-            # `media::file` being a list (or array when JSON)
-            continue
-        for val in bad_file_col:
-            if val is not None:
-                row['file'] = val
-                del row['media::file']
+
+        # For each media token, handle its value:
+        # - If value is a list, pick first non-None and migrate it.
+        # - If scalar, migrate it.
+        for media_tok in media_tokens:
+            bad_val = row.get(media_tok)
+            if bad_val is None:
+                continue
+
+            # If it's a list (the broken shape), prefer the first non-None element
+            if isinstance(bad_val, list):
+                non_nulls = [v for v in bad_val if v is not None]
+                if not non_nulls:
+                    try:
+                        del row[media_tok]
+                    except KeyError:
+                        pass
+                    any_row_fixed = True
+                    continue
+
+                chosen = non_nulls[0]
+                short_key = media_tok.split('::', 1)[1]
+                row[short_key] = chosen
+                try:
+                    del row[media_tok]
+                except KeyError:
+                    pass
                 any_row_fixed = True
-                break
+                continue
+
+            # Scalar value: migrate it directly
+            short_key = media_tok.split('::', 1)[1]
+            row[short_key] = bad_val
+            try:
+                del row[media_tok]
+            except KeyError:
+                pass
+            any_row_fixed = True
+
     if not any_row_fixed:
         return False
 
     # Multi-language forms need an additional fix to remove a superfluous null
-    # translation added by the bogus `media::file` column
+    # translation added by the bogus `media::*` column(s)
     if len(updates['translations']) > max_label_list_length:
         labels_translations_mismatch = True
         if len(updates['translations']) == max_label_list_length + 1:
             try:
-                updates['translations'].remove(UNTRANSLATED)
+                if UNTRANSLATED is not None:
+                    updates['translations'].remove(UNTRANSLATED)
+                    labels_translations_mismatch = False
+                else:
+                    # If sentinel not present, remove a media tail name
+                    tails = [m.split('::', 1)[1] for m in media_tokens]
+                    removed = False
+                    for t in tails:
+                        try:
+                            updates['translations'].remove(t)
+                            removed = True
+                            break
+                        except ValueError:
+                            continue
+                    if removed:
+                        labels_translations_mismatch = False
+                    else:
+                        updates['translations'].pop()
+                        labels_translations_mismatch = False
             except ValueError:
                 pass
-            else:
-                labels_translations_mismatch = False  # Fixed it!
+
         if labels_translations_mismatch:
             # This form has uncorrected problems. Bail out instead of modifying
             # it at all

tests/test_bugfix.py

@@ -1,6 +1,8 @@
 # coding: utf-8
+from copy import deepcopy
 
-from formpack.utils.bugfix import repair_file_column_content_in_place
+from formpack.constants import UNTRANSLATED
+from formpack.utils.bugfix import repair_media_column_content_in_place
 
 
 def test_repair_file_column():
@@ -26,7 +28,36 @@ def test_repair_file_column():
         'translated': ['label', 'media::file'],
         'translations': ['Ukrainian', 'English', None],
     }
-    assert repair_file_column_content_in_place(content)
+    assert repair_media_column_content_in_place(content)
     assert content['survey'][1]['file'] == 'oblast.csv'
     assert content['translated'] == ['label']
     assert content['translations'] == ['Ukrainian', 'English']
+
+
+def test_repair_media_image_none_plus_value_and_translations():
+    content = {
+        'schema': '1',
+        'settings': {},
+        'survey': [
+            {
+                'label': ['Código de participante'],
+                'name': 'repro_media',
+                'type': 'text',
+                'media::image': [None, 'repro_test.png'],
+                '$xpath': 'repro_media',
+            }
+        ],
+        'translated': ['label', 'media::image'],
+        'translations': [UNTRANSLATED, 'big-image'],
+    }
+    content_copy = deepcopy(content)
+
+    assert repair_media_column_content_in_place(content_copy) is True
+    assert 'image' in content_copy['survey'][0]
+    assert content_copy['survey'][0]['image'] == 'repro_test.png'
+
+    # 'media::image' token must be removed from translated
+    assert 'media::image' not in content_copy['translated']
+    assert 'label' in content_copy['translated']
+    resulting_translations = content_copy['translations']
+    assert resulting_translations in (['big-image'], [UNTRANSLATED], [None])