Merge pull request #878 from MerginMaps/fix-photo-widget-validation

Fix photo widget validation

Author: MarcelGeo

Mergin/test/test_validations.py

@@ -67,7 +67,7 @@ def test_attachment_widget(self):
             "FileWidgetFilter": "",
             "PropertyCollection": {
                 "name": None,
-                "properties": {},
+                "properties": None,
                 "type": "collection",
             },
             "RelativeStorage": QgsFileWidget.RelativeStorage.Absolute,
@@ -141,6 +141,45 @@ def test_attachment_widget(self):
         validator.check_attachment_widget()
         self.assertTrue(len(validator.issues) == 0)
 
+        # right setup, field-based override (Type 2)
+        config["PropertyCollection"]["properties"]["propertyRootPath"] = {
+            "active": True,
+            "field": "photo",
+            "expression": "",
+            "type": 2,
+        }
+        widget_setup = QgsEditorWidgetSetup("ExternalResource", config)
+        self.mem_layer.setEditorWidgetSetup(photo_field_idx, widget_setup)
+        validator.check_attachment_widget()
+        self.assertTrue(len(validator.issues) == 0)
+
+        # right setup, expression-based override with quoted field name (Type 3)
+        config["PropertyCollection"]["properties"]["propertyRootPath"] = {
+            "active": True,
+            "expression": '"photo"',
+            "type": 3,
+        }
+        widget_setup = QgsEditorWidgetSetup("ExternalResource", config)
+        self.mem_layer.setEditorWidgetSetup(photo_field_idx, widget_setup)
+        validator.check_attachment_widget()
+        self.assertTrue(len(validator.issues) == 0)
+
+        # absolute path bypass when storageUrl is active (e.g. Google Drive URLs)
+        config["RelativeStorage"] = QgsFileWidget.RelativeStorage.Absolute
+        config["PropertyCollection"]["properties"] = {
+            "storageUrl": {
+                "active": True,
+                "field": "photo",
+                "type": 2,
+            }
+        }
+        # clear DefaultRoot doesn't trigger the warning
+        config.pop("DefaultRoot", None)
+        widget_setup = QgsEditorWidgetSetup("ExternalResource", config)
+        self.mem_layer.setEditorWidgetSetup(photo_field_idx, widget_setup)
+        validator.check_attachment_widget()
+        self.assertTrue(len(validator.issues) == 0)
+
     def test_embedded_svg(self):
         symbol = QgsMarkerSymbol()
         symbol_layer = QgsSvgMarkerSymbolLayer(os.path.join(test_data_path, "transport_aerodrome.svg"))

Mergin/validation.py

@@ -242,33 +242,52 @@ def check_attachment_widget(self):
         for lid, layer in self.layers.items():
             if lid not in self.editable:
                 continue
-            fields = layer.fields()
-            for i in range(fields.count()):
-                ws = layer.editorWidgetSetup(i)
+            for field in layer.fields():
+                ws = field.editorWidgetSetup()
+
                 if ws and ws.type() == "ExternalResource":
                     cfg = ws.config()
-                    field_name = fields[i].name()
-                    # check for relative paths
-                    if "RelativeStorage" in cfg and cfg["RelativeStorage"] == QgsFileWidget.Absolute:
-                        self.issues.append(SingleLayerWarning(lid, Warning.ATTACHMENT_ABSOLUTE_PATH, field_name))
-                    # check that correct expression is set
-                    try:
-                        is_expression_enabled = cfg["PropertyCollection"]["properties"]["propertyRootPath"]["active"]
-                    except (KeyError, TypeError):
-                        is_expression_enabled = False
-                    if is_expression_enabled:
-                        expression = cfg["PropertyCollection"]["properties"]["propertyRootPath"]["expression"]
-                        if not PROJECT_VARS.search(expression):
+                    field_name = field.name()
+                    prop_collection = cfg.get("PropertyCollection") or {}
+                    properties = prop_collection.get("properties") or {}
+                    root_path_prop = properties.get("propertyRootPath") or {}
+                    storage_url_prop = properties.get("storageUrl") or {}
+
+                    is_root_active = root_path_prop.get("active", False)
+                    is_storage_active = storage_url_prop.get("active", False)
+
+                    # Check for absolute paths
+                    if not (is_root_active or is_storage_active):
+                        if (
+                            cfg.get("RelativeStorage") == QgsFileWidget.Absolute
+                        ):  # Absolute= 0, RelativeProject= 1, RelativeDefaultPath= 2
+                            self.issues.append(SingleLayerWarning(lid, Warning.ATTACHMENT_ABSOLUTE_PATH, field_name))
+
+                    # Check Data Defined Overrides
+                    if is_root_active:
+                        prop_type = root_path_prop.get(
+                            "type"
+                        )  # types are more reliable than keys which can be empty - QGIS decides based on type value
+                        if prop_type == 3:  # ExpressionBasedProperty
+                            expression = root_path_prop.get("expression", "").strip()
+                            is_field_ref = expression.startswith('"') and expression.endswith('"')
+
+                            if not (PROJECT_VARS.search(expression) or is_field_ref):
+                                self.issues.append(
+                                    SingleLayerWarning(lid, Warning.ATTACHMENT_WRONG_EXPRESSION, field_name)
+                                )
+                        elif prop_type == 2:  # FieldBasedProperty
+                            pass
+                        else:  # 1 = StaticProperty or 0 = InvalidProperty
                             self.issues.append(SingleLayerWarning(lid, Warning.ATTACHMENT_WRONG_EXPRESSION, field_name))
-                    # if expression-based path is not set with the data-defined override the app cannot resolve the path to save the photo
-                    else:
+
+                    # Fallback if expression-based path is not set
+                    elif not is_storage_active:
                         if "DefaultRoot" in cfg:
-                            # default root should not be set to the local path
-                            if os.path.isabs(cfg["DefaultRoot"]):
+                            if os.path.isabs(cfg.get("DefaultRoot", "")):
                                 self.issues.append(SingleLayerWarning(lid, Warning.ATTACHMENT_LOCAL_PATH, field_name))
 
-                            # expression must be set with the data-defined override
-                            expr = QgsExpression(cfg["DefaultRoot"])
+                            expr = QgsExpression(cfg.get("DefaultRoot", ""))
                             if expr.isValid():
                                 self.issues.append(
                                     SingleLayerWarning(