inbound shipment cost price is NaN

[aimee-mcneil-melville]

Fixes #5768

👩🏻‍💻 What does this PR do?

Creates an ArrayUtil function to handle the calculation of unit cost price (average from a 'grouped by' array). Added tests to check the new function.

Also updated SellPrice to CostPrice for the ungrouped result.

The existing function was returning NaN where costPricePerPack and numberOfPacks were both 0. Typescript does not check if NaN can be a result of a function that contains only number types, and allows 0/0 = NaN.

There are already several NaN checks in place (eg in the Currency cell definition). This calculated cost price for the 'grouped by' option is not picked up as NaN from the cell type, so implemented the ArrayUtil to handle it.
This matches the pattern of other columns that use ArrayUtils for calculations

💌 Any notes for the reviewer?

It might be worth having another ArrayUtils calculation for 'Sell Price'
I can replicate this error for grouped sell price in Outbound Shipment -> Detail View

Some other areas also using sell price column calculations, but I was unable to replicate NaN without the grouped toggle being available:

• Outbound Shipment`` -> Detail View->Outbound Line Edit```
• Supplier Returns -> Detail View
• Prescriptions -> Detail View

🧪 Testing

• Create new inbound shipment
• Add items from master list
• Toggle group by item
• View cost price as $0.00 or where there is data, a valid currency format

📃 Documentation

• Part of an epic: documentation will be completed for the feature as a whole
• No documentation required: no user facing changes or a bug fix which isn't a change in behaviour
• These areas should be updated or checked:
  1.
  2.

[github-actions]

Bundle size difference

Comparing this PR to main

Old size	New size	Diff
5.15 MB	5.15 MB	49 B (0.00%)

[mark-prins]

If I can pop in a quick request? the outbound shipment suffers the same fate, as you've noted:

Would be nice to fix that too.

I would also say that there's no need to add a function to ArrayUtils if it is only used in one place.. though yes, it makes the test creation easier 😄

To that end.. I'd make the ArrayUtils function more generic and implement in both inbound and outbound. ticks both boxes

I've made the change in this suggestion branch

[mark-prins]

An alternative would be to fix the CurrencyCell.. as that input is demanding a number and not handling gracefully the bad data. I'd prefer to have - shown rather than $0.00 as the error state but hey, can live with it 🙏

[aimee-mcneil-melville]

Great thank you Mark!
Your suggestion branch looks awesome, and definitely makes sense calculating as an average regardless of if cost or sell price is passed in - I'll pull this in!

In regards to handling the data in Currency Cell, I did look at this but didn't come up with any answers. To me it looked like the NaN is handled prior to the calculation of the average, thus not returning the error or ' ' in this case.

open-msupply/client/packages/common/src/ui/layout/tables/hooks/useColumns/useColumns.tsx

Lines 73 to 82 in d36246d

	case ColumnFormat.Currency: {
	return (value: unknown) => {
	if (Number.isNaN(Number(value))) return '';
	return `${Number(value)}`;
	};
	}
	default: {
	return (value: unknown) => String(value ?? '');
	}

If you have any wisdom on this that'd be great!

[mark-prins]

In regards to handling the data in Currency Cell,

The formatter function is only used by the BasicCell
You could change the CurrencyCell to handle string input values:

export const CurrencyCell = <T extends RecordWithId>({
  column,
  rowData,
  currencyCode,
}: CellProps<T> & { currencyCode?: Currencies }): React.ReactElement<
  CellProps<T>
> => {
  const { c } = useCurrency(currencyCode);
  const value = column.accessor({ rowData });
  if (typeof value === 'string') {
    return <div>{value}</div>;
  }

  const price = Number(value ?? 0);
...

and update ArrayUtils.getAveragePrice to do this:

    return Number.isNaN(averagePrice) ? UNDEFINED_STRING_VALUE : averagePrice;

which gives this:

but I wouldn't! 😄
you'd also have to update the un-grouped display and it's just not worth the dev or QA time!

[aimee-mcneil-melville]

Great thank you for going into that for me!
Is this one ready to merge now?

[aimee-mcneil-melville]

I can't merge until it's been ticked as approved 🙏

[mark-prins]

🚢