umbraco / Umbraco.Commerce.Issues

18 stars 2 forks source link

Ability to store objects as property values #275

Open callumbwhyte opened 3 years ago

callumbwhyte commented 3 years ago

Is your feature request related to a problem? Please describe.

I find it annoying that properties (e.g. on orders) are always a string.

For example if I want to store an integer or a boolean I have to handle converting between a string representation on every read or write.

bool isTrue = true;

order.SetProperty("alias", isTrue ? "1" : "0");

var isPropertyTrue = order.GetProperty("alias") ? "1" : "0";

Describe the solution you'd like

I would love if properties would work in a similar way to Umbraco's IPublishedContent.Value<T> - where a type can be provided and Vendr will perform the conversion.

Changing SetProperty() to take an object instead of a string shouldn't break anyone's code, and GetProperty() can continue to return a string by default. An additional overload for GetProperty<T>() could be added to convert values to a provided type.

For basic types the built in .NET type converters could be used.

Even better would be checking if the string is valid JSON and attempting to deserialize as the provided type - this could be really handy for storing additional information alongside an order, e.g. the result of calling a credit check API.

I'm absolutely not suggesting Vendr opens up a whole "Converter" model like PropertyValueConverters in Umbraco. But if some element of configurability was desired Umbraco's built in mappers could be utilised as I do here.

Describe alternatives you've considered

Currently we've been using a series of extension methods to do this, but it's becoming increasingly more code to manage as we have to write an extension for every Vendr DTO / interface we need.

The OrderReadOnly and OrderLineReadOnly DTOs implement IHasReadableProperties which is handy, but almost everywhere else the Properties property is implemented directly in the class making its presence difficult to detect. Ensuring all classes with a Properties property implements some kind of common interface would certainly be a step in the right direction.

mattbrailsford commented 3 years ago

I think this should be fine for things that just need storing / retrieving, the only thing off the top of my head I can see potential issues around is things like the property discount rule and thus what might people expect to be able to do with this. Currently it's just a simple equality comparison that is allowed, but if people are storing large serialized objects, would people start to expect to be able to compare values within the object. Or if it's a number, might they want to do range comparisons etc.

I'll have to do an audit to see what entities have properties that are not implementing the IHasReadableProperties as it feels like if they have properties, they should at least be using that.

On the whole though, I would like to bring a level of strongly typing to property access so I'm happy to look at ways to achieve this.

callumbwhyte commented 3 years ago

Valid point about equality comparisons, I hadn't considered that Vendr does this internally in places...

I think it's perfectly fair to expect the built in order property discount rule to just compare the raw value in the DB with the raw value provided in the backoffice. Without a concept like "Converters" Vendr's APIs have no idea what type the property data actually resolves to, and the backoffice simply takes a textstring.

If someone wants more complex comparison logic they're into custom discount rule provider territory.

I suppose the concerns about people wanting to do a range query etc could be targeted with documentation leading down the custom rule provider route?

mattbrailsford commented 3 years ago

@callumbwhyte yup, totally agree 👍

If this is the only drawback I think it would be fine to implement something and address those types of things in another feature later on. But like you say, in the mean time, people can use custom rule providers to provide custom handling.

callumbwhyte commented 3 years ago

Awesome @mattbrailsford!

For reference here's a watered down version of the extensions I've been using currently: https://gist.github.com/callumbwhyte/53113db7b01e8c308a43d0147f8b1cef

The Get/SetValue names are just to differentiate from the inbuilt methods.

Much like with IHasReadableProperties I couldn't find any reliable IHasWritableProperties type interface so have had to target each entity specifically. As a result it seems the current GetProperty and SetProperty methods are in Vendr are implemented directly on the entities rather than derived from an interface or (perhaps better) an extension method.