Merge pull request #5288 from msupply-foundation/5250-gaps-improve-va…

…lidation-on-packagingitem-variants

5250 gaps improve validation on packagingitem variants

client/packages/common/src/intl/locales/en/common.json

@@ -155,7 +155,7 @@
   "description.fc-sell-price": "Foreign currency sell price per pack",
   "description.forecast-quantity": "Forecast Quantity to Reach Target",
   "description.initial-stock-on-hand": "Stock on hand on the first day of the program period",
-  "description.available-stock": "The customer's initial stock on hand + incoming stock +/- inventory adjustments - outgoing stock", 
+  "description.available-stock": "The customer's initial stock on hand + incoming stock +/- inventory adjustments - outgoing stock",
   "description.invoice-number": "Shipment number",
   "description.last-reading-datetime": "Date and time of the last reading",
   "description.max-min-temperature": "Maximum or minimum temperature reading",
@@ -237,6 +237,8 @@
   "error.failed-to-save-item-variant": "Failed to save item variant",
   "error.failed-to-save-service-charges": "Failed to save service charges",
   "error.failed-to-save-vaccination": "Failed to save vaccination!",
+  "error.failed-to-save-item-variant": "Failed to save item variant",
+  "error.duplicate-item-variant-name": "Item variant with the same name already exists for this item",
   "error.failed-to-save-vaccine-course": "Failed to save vaccine course",
   "error.fetch-notifications": "Unable to display cold chain notifications",
   "error.field-must-be-specified": "{{field}} must be specified",
@@ -1642,4 +1644,4 @@
   "warning.caps-lock": "Warning: Caps lock is on",
   "warning.field-not-parsed": "{{field}} not parsed",
   "warning.nothing-to-supply": "Nothing left to supply!"
-}
+}

client/packages/common/src/types/schema.ts

@@ -7490,7 +7490,7 @@ export enum UniqueValueKey {
   Serial = 'serial'
 }
 
-export type UniqueValueViolation = InsertAssetCatalogueItemErrorInterface & InsertAssetErrorInterface & InsertAssetLogErrorInterface & InsertAssetLogReasonErrorInterface & InsertDemographicIndicatorErrorInterface & InsertDemographicProjectionErrorInterface & InsertLocationErrorInterface & UpdateAssetErrorInterface & UpdateDemographicIndicatorErrorInterface & UpdateDemographicProjectionErrorInterface & UpdateLocationErrorInterface & UpdateSensorErrorInterface & {
+export type UniqueValueViolation = InsertAssetCatalogueItemErrorInterface & InsertAssetErrorInterface & InsertAssetLogErrorInterface & InsertAssetLogReasonErrorInterface & InsertDemographicIndicatorErrorInterface & InsertDemographicProjectionErrorInterface & InsertLocationErrorInterface & UpdateAssetErrorInterface & UpdateDemographicIndicatorErrorInterface & UpdateDemographicProjectionErrorInterface & UpdateLocationErrorInterface & UpdateSensorErrorInterface & UpsertItemVariantErrorInterface & {
   __typename: 'UniqueValueViolation';
   description: Scalars['String']['output'];
   field: UniqueValueKey;

client/packages/common/src/ui/layout/tables/components/Cells/NumberInputCell/NumberInputCell.tsx

@@ -27,11 +27,13 @@ export const NumberInputCell = <T extends RecordWithId>({
   TextInputProps,
   width,
   endAdornment,
+  error,
 }: CellProps<T> &
   NumericInputProps & {
     id?: string;
     TextInputProps?: StandardTextFieldProps;
     endAdornment?: string;
+    error?: boolean;
   }): React.ReactElement<CellProps<T>> => {
   const [buffer, setBuffer] = useBufferState(column.accessor({ rowData }));
   const updater = useDebounceCallback(column.setter, [column.setter], 250);
@@ -64,6 +66,7 @@ export const NumberInputCell = <T extends RecordWithId>({
       value={buffer as number | undefined}
       width={width}
       endAdornment={endAdornment}
+      error={error}
     />
   );
 };

client/packages/system/src/Item/DetailView/Tabs/ItemVariants/ItemPackagingVariantsTable.tsx

@@ -40,13 +40,13 @@ export const ItemPackagingVariantsTable = ({
     },
     {
       key: 'packSize',
-      Cell: update ? PackSizeInputCell : TooltipTextCell,
+      Cell: update ? NonZeroInputCell : TooltipTextCell,
       label: 'label.pack-size',
       setter: updatePackaging,
     },
     {
       key: 'volumePerUnit',
-      Cell: update ? VolumeInputCell : TooltipTextCell,
+      Cell: update ? NonZeroInputCell : TooltipTextCell,
       label: 'label.volume-per-unit',
       setter: updatePackaging,
     },
@@ -65,11 +65,11 @@ export const ItemPackagingVariantsTable = ({
 };
 
 // Input cells can't be defined inline, otherwise they lose focus on re-render
-const VolumeInputCell = (props: CellProps<PackagingVariantFragment>) => (
-  <NumberInputCell decimalLimit={10} {...props} />
-);
-
-// Input cells can't be defined inline, otherwise they lose focus on re-render
-const PackSizeInputCell = (props: CellProps<PackagingVariantFragment>) => (
-  <NumberInputCell decimalLimit={10} {...props} />
+const NonZeroInputCell = (props: CellProps<PackagingVariantFragment>) => (
+  <NumberInputCell
+    step={1}
+    decimalLimit={10}
+    error={props.column.accessor({ rowData: props.rowData }) === 0}
+    {...props}
+  />
 );

client/packages/system/src/Item/DetailView/Tabs/ItemVariants/ItemVariantModal.tsx

@@ -35,7 +35,7 @@ export const ItemVariantModal = ({
   const t = useTranslation();
   const { Modal } = useDialog({ isOpen: true, onClose, disableBackdrop: true });
   const height = useKeyboardHeightAdjustment(500);
-  const { success } = useNotification();
+  const { success, error } = useNotification();
 
   const { draft, isComplete, updateDraft, updatePackagingVariant, save } =
     useItemVariant({
@@ -52,9 +52,26 @@ export const ItemVariantModal = ({
           disabled={!isComplete}
           variant="ok"
           onClick={() => {
-            save(draft);
-            success(t('messages.item-variant-saved'))();
-            onClose();
+            save(draft)
+              .then(() => {
+                success(t('messages.item-variant-saved'))();
+                onClose();
+              })
+              .catch(e => {
+                // We create the same error message as we get from the default handler, but prevent duplicates
+                // This avoids the same error message being displayed multiple times, and the only appears once bug...
+                // https://github.com/msupply-foundation/open-msupply/issues/3984
+                if (
+                  e instanceof Error &&
+                  e.message.includes(t('error.duplicate-item-variant-name'))
+                ) {
+                  error(t('error.duplicate-item-variant-name'), {
+                    preventDuplicate: true,
+                  })();
+                  return;
+                }
+                error(t('error.failed-to-save-item-variant'))();
+              });
           }}
         />
       }

client/packages/system/src/Item/api/hooks/useItemVariant/useItemVariant.ts

@@ -101,6 +101,11 @@ const useUpsert = ({ itemId }: { itemId: string }) => {
       if (result.__typename === 'ItemVariantNode') {
         return result;
       }
+      if (result.__typename === 'UpsertItemVariantError') {
+        if (result.error.__typename === 'UniqueValueViolation') {
+          throw new Error(t('error.duplicate-item-variant-name'));
+        }
+      }
     }
     throw new Error(t('error.failed-to-save-item-variant'));
   };
@@ -114,5 +119,10 @@ const useUpsert = ({ itemId }: { itemId: string }) => {
 };
 
 function getIsComplete(draft: ItemVariantFragment) {
-  return !!draft.name;
+  return (
+    !!draft.name &&
+    draft.packagingVariants.every(
+      pv => pv.packSize !== 0 && pv.volumePerUnit !== 0
+    )
+  );
 }

client/packages/system/src/Item/api/operations.generated.ts

@@ -107,7 +107,7 @@ export type UpsertItemVariantMutationVariables = Types.Exact<{
 }>;
 
 
-export type UpsertItemVariantMutation = { __typename: 'Mutations', centralServer: { __typename: 'CentralServerMutationNode', itemVariant: { __typename: 'ItemVariantMutations', upsertItemVariant: { __typename: 'ItemVariantNode', id: string, name: string, dosesPerUnit?: number | null, manufacturerId?: string | null, coldStorageTypeId?: string | null, manufacturer?: { __typename: 'NameNode', code: string, id: string, isCustomer: boolean, isSupplier: boolean, isOnHold: boolean, name: string, store?: { __typename: 'StoreNode', id: string, code: string } | null } | null, coldStorageType?: { __typename: 'ColdStorageTypeNode', id: string, name: string, minTemperature: number, maxTemperature: number } | null, packagingVariants: Array<{ __typename: 'PackagingVariantNode', id: string, name: string, packagingLevel: number, packSize?: number | null, volumePerUnit?: number | null }> } | { __typename: 'UpsertItemVariantError' } } } };
+export type UpsertItemVariantMutation = { __typename: 'Mutations', centralServer: { __typename: 'CentralServerMutationNode', itemVariant: { __typename: 'ItemVariantMutations', upsertItemVariant: { __typename: 'ItemVariantNode', id: string, name: string, dosesPerUnit?: number | null, manufacturerId?: string | null, coldStorageTypeId?: string | null, manufacturer?: { __typename: 'NameNode', code: string, id: string, isCustomer: boolean, isSupplier: boolean, isOnHold: boolean, name: string, store?: { __typename: 'StoreNode', id: string, code: string } | null } | null, coldStorageType?: { __typename: 'ColdStorageTypeNode', id: string, name: string, minTemperature: number, maxTemperature: number } | null, packagingVariants: Array<{ __typename: 'PackagingVariantNode', id: string, name: string, packagingLevel: number, packSize?: number | null, volumePerUnit?: number | null }> } | { __typename: 'UpsertItemVariantError', error: { __typename: 'DatabaseError', description: string } | { __typename: 'InternalError', description: string } | { __typename: 'UniqueValueViolation', description: string, field: Types.UniqueValueKey } } } } };
 
 export type DeleteItemVariantMutationVariables = Types.Exact<{
   storeId: Types.Scalars['String']['input'];
@@ -436,6 +436,17 @@ export const UpsertItemVariantDocument = gql`
         ... on ItemVariantNode {
           ...ItemVariant
         }
+        ... on UpsertItemVariantError {
+          __typename
+          error {
+            __typename
+            description
+            ... on UniqueValueViolation {
+              description
+              field
+            }
+          }
+        }
       }
     }
   }

client/packages/system/src/Item/api/operations.graphql

@@ -326,6 +326,17 @@ mutation upsertItemVariant($storeId: String!, $input: UpsertItemVariantInput!) {
         ... on ItemVariantNode {
           ...ItemVariant
         }
+        ... on UpsertItemVariantError {
+          __typename
+          error {
+            __typename
+            description
+            ... on UniqueValueViolation {
+              description
+              field
+            }
+          }
+        }
       }
     }
   }

server/graphql/item_variant/src/mutations/upsert.rs

@@ -1,6 +1,6 @@
 use async_graphql::*;
 use graphql_core::{
-    simple_generic_errors::{DatabaseError, InternalError},
+    simple_generic_errors::{DatabaseError, InternalError, UniqueValueKey, UniqueValueViolation},
     standard_graphql_error::{validate_auth, StandardGraphqlError},
     ContextExt,
 };
@@ -49,6 +49,7 @@ pub enum UpsertItemVariantResponse {
 #[graphql(field(name = "description", ty = "String"))]
 pub enum UpsertItemVariantErrorInterface {
     InternalError(InternalError),
+    DuplicateName(UniqueValueViolation),
     DatabaseError(DatabaseError),
 }
 
@@ -137,13 +138,23 @@ fn map_error(error: ServiceError) -> Result<UpsertItemVariantErrorInterface> {
     let formatted_error = format!("{:#?}", error);
 
     let graphql_error = match error {
+        // Structured errors
+        ServiceError::DuplicateName => {
+            return Ok(UpsertItemVariantErrorInterface::DuplicateName(
+                UniqueValueViolation(UniqueValueKey::Name),
+            ))
+        }
+        // Generic errors
         ServiceError::CreatedRecordNotFound => InternalError(formatted_error),
         ServiceError::ItemDoesNotExist => InternalError(formatted_error),
         ServiceError::PackagingVariantError(upsert_packaging_variant_error) => {
             match upsert_packaging_variant_error {
                 UpsertPackagingVariantError::ItemVariantDoesNotExist => {
                     BadUserInput(formatted_error)
                 }
+                UpsertPackagingVariantError::CantChangeItemVariant => BadUserInput(formatted_error),
+                UpsertPackagingVariantError::LessThanZero(_field) => BadUserInput(formatted_error),
+
                 UpsertPackagingVariantError::DatabaseError(_repository_error) => {
                     InternalError(formatted_error)
                 }
@@ -153,6 +164,9 @@ fn map_error(error: ServiceError) -> Result<UpsertItemVariantErrorInterface> {
             }
         }
         ServiceError::DatabaseError(_repository_error) => InternalError(formatted_error),
+        ServiceError::CantChangeItem => BadUserInput(formatted_error),
+
+        ServiceError::ColdStorageTypeDoesNotExist => BadUserInput(formatted_error),
     };
 
     Err(graphql_error.extend())

server/service/src/item/item_variant/test/mod.rs

@@ -6,7 +6,9 @@ mod query {
     use repository::{EqualFilter, StringFilter};
     use util::uuid::uuid;
 
-    use crate::item::item_variant::{DeleteItemVariant, UpsertItemVariantWithPackaging};
+    use crate::item::item_variant::{
+        DeleteItemVariant, UpsertItemVariantError, UpsertItemVariantWithPackaging,
+    };
     use crate::service_provider::ServiceProvider;
 
     #[actix_rt::test]
@@ -139,16 +141,89 @@ mod query {
 
     #[actix_rt::test]
     async fn validate_item_variant() {
-        // TODO validation tests
+        let (_, _, connection_manager, _) =
+            setup_all("validate_item_variant", MockDataInserts::none().items()).await;
 
-        // Test that the item variant name is set?
+        let service_provider = ServiceProvider::new(connection_manager, "app_data");
+        let context = service_provider.basic_context().unwrap();
+        let service = service_provider.item_service;
+
+        let test_item_a_variant_id = "test_item_variant_id";
 
         // Test that we can't create a record with an item_id that doesn't exist
+        let result = service.upsert_item_variant(
+            &context,
+            UpsertItemVariantWithPackaging {
+                id: test_item_a_variant_id.to_string(),
+                item_id: uuid(),
+                name: "Variant 1".to_string(),
+                ..Default::default()
+            },
+        );
+
+        assert_eq!(
+            result.unwrap_err(),
+            UpsertItemVariantError::ItemDoesNotExist
+        );
 
         // Test that we can't change the item_id on an existing record???
 
-        // Test that we can't create/update a record with an invalid cold_storage_id
+        // Create a new item variant for item_a
+        let _item_a_variant_a = service
+            .upsert_item_variant(
+                &context,
+                UpsertItemVariantWithPackaging {
+                    id: test_item_a_variant_id.to_string(),
+                    item_id: mock_item_a().id,
+                    name: "item_a_variant_a".to_string(),
+                    ..Default::default()
+                },
+            )
+            .unwrap();
+
+        // Try to change the item_id
+        let result = service.upsert_item_variant(
+            &context,
+            UpsertItemVariantWithPackaging {
+                id: test_item_a_variant_id.to_string(),
+                item_id: mock_item_b().id,
+                name: "Variant 1".to_string(),
+                ..Default::default()
+            },
+        );
 
-        // Test name should be unique for an item?
+        assert_eq!(result.unwrap_err(), UpsertItemVariantError::CantChangeItem);
+
+        // Test that we can't create/update a record with an invalid cold_storage_id
+        let result = service.upsert_item_variant(
+            &context,
+            UpsertItemVariantWithPackaging {
+                id: test_item_a_variant_id.to_string(),
+                item_id: mock_item_a().id,
+                name: "Variant 1".to_string(),
+                cold_storage_type_id: Some(uuid()),
+                ..Default::default()
+            },
+        );
+
+        assert_eq!(
+            result.unwrap_err(),
+            UpsertItemVariantError::ColdStorageTypeDoesNotExist
+        );
+
+        // Test: name should be unique for an item
+
+        // Add another item variant for item_a with the same name
+        let result = service.upsert_item_variant(
+            &context,
+            UpsertItemVariantWithPackaging {
+                id: uuid(),
+                item_id: mock_item_a().id,
+                name: "item_a_variant_a".to_string(),
+                ..Default::default()
+            },
+        );
+
+        assert_eq!(result.unwrap_err(), UpsertItemVariantError::DuplicateName);
     }
 }