Issue 1472: Update check for allowing a field to be required to exclude or include aliquots depending on field derivation scope

api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java

@@ -47,6 +47,7 @@
 import org.labkey.api.exp.property.DomainUtil;
 import org.labkey.api.exp.query.ExpMaterialTable;
 import org.labkey.api.exp.query.ExpSampleTypeTable;
+import org.labkey.api.exp.query.ExpSchema;
 import org.labkey.api.exp.query.SamplesSchema;
 import org.labkey.api.gwt.client.DefaultValueType;
 import org.labkey.api.gwt.client.model.GWTDomain;
@@ -623,13 +624,18 @@ public boolean hasNullValues(Domain domain, DomainProperty prop)
 
         if (getTotalAndNonBlankSql(domain, prop, allRowsSQL, nonBlankRowsSQL))
         {
+            String scope = prop.getDerivationDataScope();
             // Issue 43754: Don't include aliquot rows in the null value check (see ExpMaterialTableImpl.createColumn for IsAliquot)
-            String table = domain.getStorageTableName();
-            SQLFragment nonAliquotRowsSQL = new SQLFragment("SELECT * FROM exp.material WHERE RowId IN (")
-                    .append("SELECT RowId FROM " + getStorageSchemaName() + "." + table)
-                    .append(") AND RootMaterialRowId = RowId");
+            // GH Issue 1472: Include aliquot rows when checking for a property that is editable for both aliquots and samples
+            if (StringUtils.isEmpty(scope) || ExpSchema.DerivationDataScopeType.ParentOnly.name().equalsIgnoreCase(scope))
+            {
+                String table = domain.getStorageTableName();
+                allRowsSQL = new SQLFragment("SELECT * FROM exp.material WHERE RowId IN (")
+                        .append("SELECT RowId FROM ").append(getStorageSchemaName()).append(".").append(table)
+                        .append(") AND RootMaterialRowId = RowId");
+            }
 
-            long totalRows = new SqlSelector(ExperimentService.get().getSchema(), nonAliquotRowsSQL).getRowCount();
+            long totalRows = new SqlSelector(ExperimentService.get().getSchema(), allRowsSQL).getRowCount();
             long nonBlankRows = new SqlSelector(ExperimentService.get().getSchema(), nonBlankRowsSQL).getRowCount();
             return totalRows != nonBlankRows;
         }

experiment/src/org/labkey/experiment/api/property/DomainImpl.java

@@ -83,6 +83,7 @@
 import org.labkey.api.exp.property.DomainPropertyAuditProvider;
 import org.labkey.api.exp.property.DomainTemplate;
 import org.labkey.api.exp.property.DomainUtil;
+import org.labkey.api.exp.query.ExpSchema;
 import org.labkey.api.gwt.client.model.GWTIndex;
 import org.labkey.api.gwt.client.model.GWTPropertyDescriptor;
 import org.labkey.api.query.BatchValidationException;
@@ -123,6 +124,7 @@
 import java.util.concurrent.locks.Lock;
 
 import static org.labkey.api.data.ColumnRenderPropertiesImpl.STORAGE_UNIQUE_ID_SEQUENCE_PREFIX;
+import static org.labkey.api.exp.query.ExpSchema.DerivationDataScopeType.ChildOnly;
 
 public class DomainImpl implements Domain
 {
@@ -756,7 +758,9 @@ else if (impl.isNew())
                             // If this field is newly required, or it's required and we're disabling MV indicators on
                             // it, make sure that all the rows have values for it
                             if ((!impl._pdOld.isRequired() && impl._pd.isRequired()) ||
-                                    (impl._pd.isRequired() && !impl._pd.isMvEnabled() && impl._pdOld.isMvEnabled()))
+                                    (impl._pd.isRequired() && !impl._pd.isMvEnabled() && impl._pdOld.isMvEnabled()) ||
+                                    (impl._pdOld.isRequired() && derivationDataScopeChanged(impl._pdOld.getDerivationDataScope(), impl._pd.getDerivationDataScope()))
+                            )
                             {
                                 checkRequiredStatus.add(impl);
                             }
@@ -874,6 +878,14 @@ else if (null != pdOld)
             {
                 for (DomainProperty prop : checkRequiredStatus)
                 {
+                    // Issue 46733: A field that is editable for aliquots only is never populated for a sample, so it can
+                    // never be required. Reject it before the blank value check below, which would otherwise report every
+                    // sample row as blank and give a misleading reason.
+                    if (ChildOnly.name().equalsIgnoreCase(prop.getDerivationDataScope()))
+                    {
+                        throw new ChangePropertyDescriptorException("The property \"" + prop.getName() + "\" cannot be required when it is editable for aliquots only.");
+                    }
+
                     boolean hasRows = kind.hasNullValues(this, prop);
                     if (hasRows)
                     {
@@ -926,6 +938,13 @@ else if (!isDomainNew)
         }
     }
 
+    private boolean derivationDataScopeChanged(String oldScope, String newScope)
+    {
+        ExpSchema.DerivationDataScopeType oldType = oldScope == null ? ExpSchema.DerivationDataScopeType.ParentOnly : ExpSchema.DerivationDataScopeType.valueOf(oldScope);
+        ExpSchema.DerivationDataScopeType newType = newScope == null ? ExpSchema.DerivationDataScopeType.ParentOnly : ExpSchema.DerivationDataScopeType.valueOf(newScope);
+        return oldType != newType;
+    }
+
     record CalculatedFieldsUpdate(@Nullable List<MetadataColumnJSON> added, @Nullable List<MetadataColumnJSON> removed, @Nullable List<Pair<MetadataColumnJSON, MetadataColumnJSON>> updated)
     {
         public boolean hasChange()