Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Models aren't easy to extend with custom columns #32

Open
rshackleton opened this issue Aug 30, 2018 · 3 comments
Open

Models aren't easy to extend with custom columns #32

rshackleton opened this issue Aug 30, 2018 · 3 comments

Comments

@rshackleton
Copy link

rshackleton commented Aug 30, 2018

For example, if a new column is added to the CustomerInfo class (i.e. CustomerTitle) then there is no way of using the Kentico.Ecommerce package to access those new columns and expose them through the Customer model.

It could be useful to amend the models to set the "OriginalCustomer" value to be a protected property which can be access by classes inheriting from Customer. This would allow us to create a custom Customer model which includes our extra columns.

This pattern could be applied to the other models in the various packages.

@kentico-zdenekc
Copy link

Richard,
Thank you for your suggestion and ideas.

You are right. Current implementation (in Kentico 10 and 11) is that a model wraps an Info object and has the same default properties. This Info object is unfortunately internal readonly, so it cannot be accessed in the model extension.
We understand the limitation for customization because of this. At the same time it's important to mention that in Kentico 12, we are dropping the default models and related API in Kentico.Ecommerce integration package and also introduce new business API, universally usable in MVC and other development models.
This includes the Customer model removal. Replacement recommendation will be to use CustomerInfo object (and other direct ECommerce objects) or write your own model if needed.

We have discussed the possibilities of changing the internal Info object property protection to make it available in extended models (classes), it's still an open discussion, but because of the upcoming changes in K12 there's some voice against that.

With that being said, the ID of the object is already available, for example (in Customer model):

    public int ID => OriginalCustomer.CustomerID;

This means that it would be possible to add a custom CustomerInfo property in the extended Customer model, e.g.:

internal readonly CustomerInfo MyOriginalCustomer; //(does not have to be internal)

... and initialize it in the constructor. Default model constructors are as follows:

    public Customer()
        : this(new CustomerInfo())
    {
    }

    public Customer(CustomerInfo originalCustomer)
    {
        if (originalCustomer == null)
        {
            throw new ArgumentNullException(nameof(originalCustomer));
        }

        OriginalCustomer = originalCustomer;
    }

Then build the custom properties for the custom fields. Value of a custom field of Info object could be get/set using GetValue/SetValue method.
The custom properties of the extended model class could be implemented e.g. this way:

    public object MyCustomerCustomProperty
    {
        get
        {
            return MyOriginalCustomer.GetValue("MyCustomerCustomFieldName");
        }
        set
        {
            MyOriginalCustomer.SetValue("MyCustomerCustomFieldName", value);
        }
    }

Anyway, due to restriction of the original model internal property access, it would be better to build the custom model using similar approach, but completely independent of the default one. At the same time, this would render the whole integration package a bit useless (as the original Customer model is used there).
If you can go with CustomerInfo object (or other base objects) alongside default models in order to read and work with your custom data, it would seem like an easiest solution until the next version makes the API tasks easier and allow you to work with the base info objects with minimal need for wrapper models.

Would your projects still benefit from the extensibility if we changed the internal info object properties access in a hotfix, i.e. make a public getter? I'm asking this with the information about dropping the models in next version in mind...

Any further comments, questions or feedback are appreciated!
Regards,
Zdenek

@rshackleton
Copy link
Author

Hi Zdenek,

Thanks for the detailed response, appreciate the details re. the Kentico 12 API changes. Based on those I can understand why changing the current package for K11 would be less likely. We've "worked around" the issue in a similar manner as you described. We're using the Info classes directly when we need to access properties and creating a new instance of the model when we need to interact with the business APIs.

I believe it would be very helpful for projects currently in K11 to have access to the info object and to also have access in derived types. It would allow us to remove and simplify a portion of our business logic.

The key would be to make those changes whilst remaining compatible with the business APIs, primarily the APIs around the shopping cart.

Regards,

Rich

@seangwright
Copy link
Member

@kentico-zdenekc what was the original architectural motivation behind encapsulating the Info types behind new model types in the Kentico.Ecommerce package.

I notice this pattern repeated throughout the Kentico.* packages.

Is the goal a more DDD approach where the models have richer behavior instead of being ORM property bags like the *Info types are?

Or was the goal to slowly migrate the *Info types to being internal Kentico-as-a-framework concerns and expose a new set of model Apis that would allow the Kentico team to make *Info Api changes behind the scenes?

I'm asking because like @rshackleton my team often uses custom fields on core Kentico types (UserInfo, CustomerInfo, OrderInfo, ect...) and I want to ensure we are following patterns in alignment with Kentico's architecture goals.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants