oqtane / oqtane.framework

CMS & Application Framework for Blazor & .NET MAUI
http://www.oqtane.org
MIT License
1.83k stars 528 forks source link

Entities from Interfaces and DateTime #94

Closed bbakermmc closed 4 years ago

bbakermmc commented 4 years ago

Why not have the classes for audit/soft deletes etc inherit from some base classes instead of implementing the properties in every class?

Also should probably use UTC times or be configurable. I dont remember if EFCore3 now allows to use the SQL datatime value instead which would be preferred. (EX: If you have a default value set in SQL currently EF doesnt honor it with EF.)

EX of classes:


 [JsonObject(NamingStrategyType = typeof(DefaultNamingStrategy))]
    public partial class Address: AuditBase<int>
    {
        #region Keys

        /// <inheritdoc />
        ///<summary>
        /// Identifier for an address.
        ///</summary>
        [Column("AddressId")]
        [Display(Name = "Address Id", Description = "Identifier for an address.", Prompt = "Address Id")]
        public override int Id { get; set; }

        #endregion

        #region Scalars

        ///<summary>
        /// Line 1 of the address.
        ///</summary>
        [Required]
        [StringLength(50, ErrorMessage="Address 1 max length is 50.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Address 1", Description = "Line 1 of the address.", Prompt = "Address 1")]
        public string Address1 { get; set; }

        ///<summary>
        /// Line 2 of the address.
        ///</summary>
        [StringLength(50, ErrorMessage="Address 2 max length is 50.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Address 2", Description = "Line 2 of the address.", Prompt = "Address 2")]
        public string Address2 { get; set; }

        ///<summary>
        /// 3rd line of the address.  Used in European addresses.
        ///</summary>
        [StringLength(50, ErrorMessage="Address 3 max length is 50.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Address 3", Description = "3rd line of the address.  Used in European addresses.", Prompt = "Address 3")]
        public string Address3 { get; set; }

        ///<summary>
        /// City of the location.
        ///</summary>
        [Required]
        [StringLength(35, ErrorMessage="City max length is 35.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="City", Description = "City of the location.", Prompt = "City")]
        public string City { get; set; }

        ///<summary>
        /// State of the address.
        ///</summary>
        [StringLength(2, ErrorMessage="State max length is 2.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="State", Description = "State of the address.", Prompt = "State")]
        public string State { get; set; }

        ///<summary>
        /// Zip 5 of the address.
        ///</summary>
        [StringLength(5, ErrorMessage="Zip max length is 5.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Zip", Description = "Zip 5 of the address.", Prompt = "Zip")]
        public string Zip { get; set; }

        ///<summary>
        /// Zip plus 4 of the address.
        ///</summary>
        [StringLength(4, ErrorMessage="Zip 4 max length is 4.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Zip 4", Description = "Zip plus 4 of the address.", Prompt = "Zip 4")]
        public string Zip4 { get; set; }

        ///<summary>
        /// The postal code of the address. If a zip+4 is available for a US address this will be formatted as xxxxx-xxxx. Otherwise it will contain the 5-digit zip or the Canadian postal code.
        ///</summary>
        [Required]
        [StringLength(15, ErrorMessage="Postal Code max length is 15.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Postal Code", Description = "The postal code of the address. If a zip+4 is available for a US address this will be formatted as xxxxx-xxxx. Otherwise it will contain the 5-digit zip or the Canadian postal code.", Prompt = "Postal Code")]
        public string PostalCode { get; set; }

        ///<summary>
        /// Code of the country.
        ///</summary>
        [StringLength(3, ErrorMessage="Country Code max length is 3.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Country Code", Description = "Code of the country.", Prompt = "Country Code")]
        public string CountryCode { get; set; }

        ///<summary>
        /// Federal Information Processing Standard Code that uniquely identifies counties and county equivalents in ht eUnited States, certain US possession, and certain freely associated
        ///</summary>
        [StringLength(5, ErrorMessage="FIPS Code max length is 5.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="FIPS Code", Description = "Federal Information Processing Standard Code that uniquely identifies counties and county equivalents in ht eUnited States, certain US possession, and certain freely associated", Prompt = "FIPS Code")]
        public string FIPSCode { get; set; }

        ///<summary>
        /// Reutrn code provided by the address hygiene process.
        ///</summary>
        [StringLength(1, ErrorMessage="Address Return Code max length is 1.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Address Return Code", Description = "Reutrn code provided by the address hygiene process.", Prompt = "Address Return Code")]
        public string AddressReturnCode { get; set; }

        ///<summary>
        /// Latitude of the address.
        ///</summary>
        [Display(Name ="Latitude", Description = "Latitude of the address.", Prompt = "Latitude")]
        public decimal? Latitude { get; set; }

        ///<summary>
        /// Longitude based on the address.
        ///</summary>
        [Display(Name ="Longitude", Description = "Longitude based on the address.", Prompt = "Longitude")]
        public decimal? Longitude { get; set; }

        ///<summary>
        /// Indicator as to the level that the longitude and latitude were calculated to.
        ///</summary>
        [StringLength(1, ErrorMessage="Geo Coding Accuracy Code max length is 1.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Geo Coding Accuracy Code", Description = "Indicator as to the level that the longitude and latitude were calculated to.", Prompt = "Geo Coding Accuracy Code")]
        public string GeoCodingAccuracyCode { get; set; }

        ///<summary>
        /// Date that the reco
        ///</summary>
        [Display(Name ="Last Hygiene Date", Description = "Date that the reco", Prompt = "Last Hygiene Date")]
        public DateTime? LastHygieneDate { get; set; }

        ///<summary>
        /// Matchkey used for a quick match of the address.
        ///</summary>
        [Required]
        [StringLength(250, ErrorMessage="Address Match Key max length is 250.")]
        [DisplayFormat(ConvertEmptyStringToNull = true)]
        [Display(Name ="Address Match Key", Description = "Matchkey used for a quick match of the address.", Prompt = "Address Match Key")]
        public string AddressMatchKey { get; set; }

        #endregion

        #region Reverse Navigation

        ///<summary>
        /// Parent Address pointed to by AddressHygieneQueue.AddressId
        ///</summary>
        [JsonIgnore]
        public List<AddressHygieneQueue> AddressAddressHygieneQueue_Reverse { get; set; }

        ///<summary>
        /// Parent Address pointed to by CompanyAddress.AddressId
        ///</summary>
        [JsonIgnore]
        public List<CompanyAddress> AddressCompanyAddress_Reverse { get; set; }

        ///<summary>
        /// Parent Address pointed to by ContactAddress.AddressId
        ///</summary>
        [JsonIgnore]
        public List<ContactAddress> AddressContactAddress_Reverse { get; set; }

        ///<summary>
        /// Parent Address pointed to by ContactChannelInfo.AddressId
        ///</summary>
        [JsonIgnore]
        public List<ContactChannelInfo> AddressContactChannelInfo_Reverse { get; set; }

        ///<summary>
        /// Parent Address pointed to by Subscriber.AddressId
        ///</summary>
        [JsonIgnore]
        public List<Subscriber> AddressSubscriber_Reverse { get; set; }

        #endregion

        public Address()
        {

            OnCreated();

        }
        partial void OnCreated();
    }
[JsonObject(NamingStrategyType = typeof(DefaultNamingStrategy))]
    public class AuditBase<T> : IEntity<T>
    {
        [Column("Inserted", TypeName = "datetime")]
        [Required(ErrorMessage = "Inserted  is required")]
        [Display(Name = "Inserted", Order = 9000)]
        [ReadOnly(true)]
        [DisplayFormat(DataFormatString = "{0:MM/dd/yyyy hh:mm tt}")]
        [DefaultValue("Getdate()")]
        [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
        public virtual DateTime Inserted { get; set; }

        [Column("InsertedBy")]
        [MaxLength(128)]
        [StringLength(128)]
        [Required(ErrorMessage = "Inserted By  is required")]
        [Display(Name = "Inserted By", Order = 9001)]
        public virtual string InsertedBy { get; set; }

        [Column("Updated", TypeName = "datetime")]
        [Required(ErrorMessage = "Updated  is required")]
        [Display(Name = "Updated", Order = 9002)]
        [DisplayFormat(DataFormatString = "{0:MM/dd/yyyy hh:mm tt}")]
        [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
        [DefaultValue("Getdate()")]
        public virtual DateTime Updated { get; set; }

        [Column("UpdatedBy")]
        [MaxLength(128)]
        [StringLength(128)]
        [Required(ErrorMessage = "Updated By  is required")]
        [Display(Name = "Updated By", Order = 9003)]
        public virtual string UpdatedBy { get; set; }

        [Column("EditCount", TypeName = "int")]
        [Required(ErrorMessage = "Edit Count  is required")]
        [Display(Name = "Edit Count", Order = 904)]
        [ConcurrencyCheck]
        public virtual int EditCount { get; set; }

        public bool IsTransient()
        {
            if (EqualityComparer<T>.Default.Equals(Id, default))
            {
                return true;
            }

            //Workaround for EF Core since it sets int/long to min value when attaching to dbcontext
            if (typeof(T) == typeof(int))
            {
                return Convert.ToInt32(Id) <= 0;
            }

            if (typeof(T) == typeof(long))
            {
                return Convert.ToInt64(Id) <= 0;
            }

            return false;
        }

        public virtual T Id { get; set; }
    }
 public interface IEntity : IEntity<int>
    {

    }
sbwalker commented 4 years ago

If you look at the models in the Shared project you will notice there is already an IAuditable interface that models are implementing for tracking CreatedBy, CreatedOn, ModifiedBy, ModifiedOn - and these fields are being updated automatically within the DBContext. There is a pull request which has been submitted to do something similar for soft deletions.

bbakermmc commented 4 years ago

@sbwalker Im aware of that, I'm questing why use an interface rather than a base class so we dont need to duplicate a bunch of fields in possibly thousands of files.

sbwalker commented 4 years ago

The interface approach provides more flexibility over how you want to define your entities - its an opt-in model. If your entities require auditing you can implement IAuditable, if they require soft delete they can implement IDeletable, etc... If we used the base class approach it would require that your entities support all features - which may or may not make sense in all cases. Obviously there would be nothing stopping you from creating your own base class for entities if you prefer this approach for your own entities.

sbwalker commented 4 years ago

However your point about UTC dates is valid. Currently the DBContextBase is using DateTime.Now which is problematic. I will change it to DateTime.UtcNow. Thank you for pointing this out.

fileman commented 4 years ago

... and until this is closed, it's better to duplicate the properties ... in my opinion

fileman commented 4 years ago

However your point about UTC dates is valid. Currently the DBContextBase is using DateTime.Now which is problematic. I will change it to DateTime.UtcNow. Thank you for pointing this out.

why not DateTimeOffset instead?

sbwalker commented 4 years ago

Audit dates should uniquely and unambiguously identify a single point in time - which is what UtcNow provides. Also UtcNow is universally supported across all database platforms, and at some point Oqtane will need to support more than just SQL Server.

bbakermmc commented 4 years ago

@fileman Possibly, we dont use EF as our primary source for SQL, so for us this isnt problematic and honestly isnt it a moot point in the end SQL doesnt care.

@sbwalker Im not a huge fan over the override on the context. Id much prefer it in the Repository so I could override it. Again, not sure why you didn't just implement some generic patterns :) Im just putting my ideas out there for you. We have a current solution with something like over 400 entities for our CRM, generics help out a lot when you need to implement basic functionality across hundred of entities. But then this way in specific entities if we want to override the Add to not set the inserted/insertby automatically we can, this way if we have Audit logging that still flow just fine but we were able to override the values/process.

fileman commented 4 years ago

@bbakermmc isn't problematic for SQL, but have audit properties on top of tables columns is not the best to see... but is only an opinion. I like the idea of generic pattern for Id type, since in some case it could be int, string or GUID, even with Interfaces, something like IAuditable<TKey> as in other frameworks

fileman commented 4 years ago

but in this case it's required DataAnnotations Key attribute in Infertace to decorate Id property, since Oqtane now use naming conventions ... I await the opinion of @sbwalker before working on it

bbakermmc commented 4 years ago

@fileman Correct, but it is easier to then make generic methods and such. Its how we currently do it.

We have a solution based on AspNetZero. And while its fairly decent, they dont provide upgrade paths, so each new version it can be a nightmare to upgrade, plus they spread their code across multiple nugets, and you cant always override what you need to do so it could be a couple of weeks/versions for them to implement the override. But the standard repository/application service/bases work fairly well. They have a new version they are working on ABP.IO but they seem to want to make UIs for multiple frameworks which is where the real issues are. This is why Im looking at Oqtane. Its Blazor focused, and seems to simplify the overall architecture. But from someone who is going to use the framework, there seems to be a lot of duplicated code that could be impemented in to some generic patterns so that when we want to add a new entity we dont need to copy/paste a bunch of template code. Just inherit from a base repo where all the basic CRUD is done and let us override if we need.

EX: Why does every Repo need to define these? Have the repo CTOR set the EntityName like the Permissions, change the Id to a generic Id use the Key attribute. Then every entity we add we inherit and its like boom, off we go.

public Module AddModule(Module Module)
        {
            db.Module.Add(Module);
            db.SaveChanges();
            Permissions.UpdatePermissions(Module.SiteId, "Module", Module.ModuleId, Module.Permissions);
            return Module;
        }

Then from here we could have some generic razor components to maybe implement a grid or form with very little coding. If I could spend a few more min adding attributes to a model to make a form or grid auto generate its very valuable to me, and I think most people, no one wants to make UI pages for CRUD.

If you take a basic Address model [PrimaryKey] AddressId Address1 Address2 [DropDown(Repository = "Country", Method = "GetActiveCountries")] Country [DropDown(Repository = "State", Method = "GetActiveStates")] State

Then maybe there is a generic GetGridValues(int pageSize, int skip, int page) The json would come back with a set of values for the grid, and then on the first request when page = 0 you would also get back two other sets to be used for filtering or drop downs on forms etc. And if I needed more join I could override the GetGridValues add the extra includes.

sbwalker commented 4 years ago

My preference is to not refactor the current repository layer - it is functional and satisfies the business goals intended for the initial MVP. The recommended approach for working with the Oqtane entities from a third party development perspective is to rely on the APIs - not the database. And just to be clear there is nothing stopping anyone from implementing their own repository patterns/practices for developing their own extensions. The idea is for Oqtane to not be overly opinionated so that it allows the flexibility for developers to use their own preferred development methods.

bbakermmc commented 4 years ago

@sbwalker I guess this is where my lack of understand what you are trying to accomplish comes in. I know this is fairly early work so far. But from a person who is looking to your framework maybe a basic guide on how I would implement a basic module. With the understanding that I want to be able to easily upgrade from v1 to v1.1 etc.

Should I make my own DB context since I dont want your opinionated updating of audit columns for my entities? Is IAudit ever going to implement functionality where I can see in the UI where I select and entity and I can see the logs? In that case do I then inherit your interface or do I need to make my own and then register that? There becomes a point where opinions matter.

fileman commented 4 years ago

@bbakermmc I known ABP / AspNetZero, it's a complete and complex application framework for all layer, while Oqtane seem to be more focused to UI, leaving at developer to possibility to choose their preferred pattern. I've reflected on this and even generic IAuditable was not a solution, because it's designed for Oqtane Core Entities, so the type of Key is not a developer choise. As @sbwalker has said, you can extend Oqtane with a module that matches your needs and even share with a community.

sbwalker commented 4 years ago

@bbakermmc you should take a look at the the "blog" module ( https://github.com/oqtane/oqtane.module.blogs ) as it provides an example of how to build a module external to Oqtane which can be installed and utilized. @fileman is correct in his assessment of Oqtane - its goal is to provide a composite UI framework which allows you to dynamically build modular web applications - but it is not opinionated on how you build your modules. The only requirement is that a module's components inherit from ModuleBase - but when it comes to the business logic, services, data access, etc... you are free to build them based on your own preference. There will obviously be some example modules ( such as the Blog module ) which demonstrate a specific approach but its up to you if you want to follow it or not. This loosely coupled, non-opinionated philosophy was how DotNetNuke operated as well.

mikecasas commented 4 years ago

while Oqtane seem to be more focused to UI

So is Oqtanane more of a "Vue" for .NET, and doesn't really focus on the back end?