VirtoCommerce/vc-module-order

Update order with more than one dynamic property leads to wrong properties (merged values)

corneliusmunz opened this issue · 7 comments

Please provide detailed information about your issue, thank you!

Version info:

  • Browser version: doesn't matter (backend issue)
  • Platform version: v3.80.0
  • Module version: 3.43.0 (current release)

Expected behavior

If i add two dynamic properties to an existent order with no dynamic properties and update the order via the API i would expect to see two dynamic properties with their values.

The same would be expected if i add more than two properties and update the order via the API

Actual behavior

If i add two dynamic properties to an exsitent order and update the order via API i will get ONE dynamic property with all the values of the two dynamic properties MERGED in that ONE dynamic property.

--> Seems to be a bug in the following line of code where a GroupBy statement is done on the PropertyId which is for the new properties both "null". If i change the line to GroupBy Id instead of PropertyId, it works

order.DynamicProperties = DynamicPropertyObjectValues.GroupBy(g => g.PropertyId).Select(x =>

Steps to reproduce

  1. create order from cart without any dynamic properties
  2. Update order with two different dynamic properties
  3. Fetch order and look into the DynamicProperties

Example order response with the wrong result where the values are merged into one property in the response
wrong_result
Example order response with the correct result where two dynamic properties are available in the response
correct_result

@corneliusmunz Could you please show an example query how you create new dynamic properties for orders (update orders)? ("dynamicProperties":[] section)

@dvvkgd Here you can find an example code how we add dynamic properties (not exactly used in our code base but similar). We are using NSWAG for generating clients to access the VIRTO API

  1. Create order from Shopping Cart with
var order = await _orderNswagApiClient.OrderModule_CreateOrderFromCartAsync(shoppingCart.Id);
  1. Update order object
order.CustomerId = "1234";
order.CustomerName = "My company";
order.EmployeeId = "My subject id";
order.OrganizationId = "Customer reference number";
order.Status = "Pending"";
order.LanguageCode = "de-DE";
if (order.DynamicProperties == null)
{
    order.DynamicProperties = new List<DynamicObjectProperty>();
}
var clientIdProperty = new DynamicObjectProperty
{
    Name = "ClientId",
    Description = "ClientId which has created the order",
    ValueType = DynamicPropertyValueType.ShortText,
    Values = new List<DynamicPropertyObjectValue>
    {
        new DynamicPropertyObjectValue
        {
            Value = "This is the client Id which has created the order",
            ValueType = DynamicPropertyValueType.ShortText
        }
    }
};
order.DynamicProperties.Add(clientIdProperty);
if (!string.IsNullOrEmpty(company.LogoUrl))
{
    var logoProperty = new DynamicObjectProperty
    {
        Name = "LogoUrl",
        Description = "Url to installer company logo",
        ValueType = DynamicPropertyValueType.LongText,
        Values = new List<DynamicPropertyObjectValue>
        {
            new DynamicPropertyObjectValue
            {
                Value = "This is the url to the logo of the customer",
                ValueType = DynamicPropertyValueType.LongText
            }
        }
    };
    order.DynamicProperties.Add(logoProperty);
}
  1. Do a PUT on the /orders resouce via the NSWAG generated API
await _orderNswagApiClient.OrderModule_UpdateOrderAsync(order);

@dvvkgd If i look into the DB the values are stored correctly with the PUT/Update of the order resource. Only the fetch oft GET .../orders does merge the values

@corneliusmunz I can suggest the workaround:
to work with dynamic properties, You first needs to create them and then use their ID for 'PropertyId' field. I hope, it helps You.
image
image
image

We will also create a task to handle this scenario and support dynamic properties without PropertyID.

@dvvkgd many thanks for your feedback. I will try out the workaround (or maybe better the way it was supposed to be used) and would be good if you support the scenario in the future without haveing already a PropertyID