OrchardCore.Commerce icon indicating copy to clipboard operation
OrchardCore.Commerce copied to clipboard

Adding Image Field with PricePart Errors (OCC-75)

Open jeffolmstead opened this issue 5 years ago • 4 comments

I have tried to isolate this down so here goes. If you add an Image Field (could be any field but I used an Image Field with a name of "Image" then go to add a new product (which has the PricePart and ImageField associated with the content type) it will crash as "LegacyAmountConverter" in "MoneyDataType" project is being called before the post back to the database has a chance to store the "Currency":

if (!Currency.IsKnownCurrency(currency.CurrencyIsoCode))
{
     currency = new Currency(nativename, englishname, symbol, iso, dec.GetValueOrDefault(2));
}

"currency" is null so it will crash. I can't figure out why having a Field on it is causing this to fire off BEFORE the driver is updating the data... If I can figure out more, will let you know.

Jira issue

jeffolmstead avatar Feb 14 '20 19:02 jeffolmstead

Here is the smallest test I came up with:

ContentType:

  • Let's call it SimpleProduct

Fields:

  • Add any text field

Parts:

  • Add PricePart

Then go to make a new "Simple Product" and it will render fine. But, it won't save properly (e.g. the PricePart won't save) and it won't render the PricePart going forward.

jeffolmstead avatar Feb 14 '20 20:02 jeffolmstead

The issue is triggered off of OrchardCore.ContentManagement.ContentExtensions.Get method on line 56:

    var result = (ContentElement)elementData.ToObject(contentElementType);

I am assuming this is only called when a field is present.

jeffolmstead avatar Feb 14 '20 20:02 jeffolmstead

I am not sure how to help on this one as I see there is a "LegacyAmountConverter" and an "AmountConverter" Is one of these going away or are both being kept as one targets Newtonsoft.Json? If LegacyAmountConverter is being kept, should check for "null" on currency before checking if Currency.IsKnownCurrency (i.e. change these lines around):

if (!Currency.IsKnownCurrency(currency.CurrencyIsoCode))
{
    currency = new Currency(nativename, englishname, symbol, iso, dec.GetValueOrDefault(2));
}

if (currency is null)
    throw new InvalidOperationException("Invalid amount format. Must include a currency");

to

if (currency is null)
    throw new InvalidOperationException("Invalid amount format. Must include a currency");

if (!Currency.IsKnownCurrency(currency.CurrencyIsoCode))
{
    currency = new Currency(nativename, englishname, symbol, iso, dec.GetValueOrDefault(2));
}

It seems we need to somehow control the timing (e.g. fields are added first before part is saved). Perhaps there is something that can be skipped in the LegacyAmountConverter if no data set yet, just throwing out thoughts. Off to other development, please let me know if there is a way I can help on this issue.

jeffolmstead avatar Feb 14 '20 20:02 jeffolmstead

The one being kept is the "legacy" one, ironically. I'll be removing all the System.Text.Json ones soon. Failed experiment.

bleroy avatar Feb 14 '20 20:02 bleroy

Closing because I can't reproduce it with the instructions mentioned above. We have changed a lot of related code since this issue was opened so it's probably been fixed on the side. Feel free to reopen if you encounter it again.

sarahelsaig avatar Oct 02 '23 16:10 sarahelsaig