nopCommerce 4.30 - Bug fixes and improvements

Hi guys,

During the upgrade of our best selling One Page Checkout plugin we noticed that nopCommerce 4.30 handles customer billing and shipping addresses quite differently than nopCommerce 4.20.

In previous versions the logic of obtaining the customer Billing or Shipping addresses relied only on the BillingAddressId/ShippingAddressId properties of the customer and their respective navigation properties.

But in nopCommerce 4.30 there are new methods in the customer service GetCustomerBillingAddress and GetCustomerShippingAddress that get the addresses by checking if they exists in the customer address mappings rather than getting them simply by their IDs as it used to be in previous versions of nopCommerce.

This is quite different than what it used to be in nopCommerce 4.20 where we have never been expected to force the addresses to be part of the address book of the customer.

Actually we used this functionality in our One Page Checkout plugin to create "temp" addresses during the checkout and add them to the address book only when the order is confirmed.

Now we are "forced" to always keep these "temp" addresses in the address book which of course we would like to prevent as we want to keep the address book as clean as possible.

Now if we set a billing address to the customer and this billing address is not part of the address book for that customer we end up with an exception when we call GetCustomerBillingAddress, instead of just returning the address by its ID as it used to be in previous versions of nopCommerce.

What is the reason for this change? Why should the address always be part of the address book? What is the reason for this limitation?

Actually in the regular checkout where you always specify the address first before calculating any payment or shipping options etc. it is OK to put the address in the address book after the address step as you can't change it any longer but in a plugin like our One Page Checkout (and I guess other similar plugins by other vendors) you can make changes to the addresses all the time until you finally Confirm the order.  At the same time we don't want to change any of the existing addresses already saved in the address book. That is why we need the option to have "temp checkout" addresses until the checkout is not finished yet. Only after the checkout is finished we decide if this is a new address or it matches any of the existing ones and then we add it to the address book if it is a new one.

I hope this makes sense and it would be great if you can leave the old behavior and simply get the addresses by their IDs rather than trying to get them from the customer's address book.

Thank you for looking at this!

Boyko

Nop-Templates.com wrote:
I don't think it's a limitation. It's a validation that this address belongs to this customer.

Anyway, you can always override these two methods ("GetCustomerBillingAddress" and "GetCustomerShippingAddress") and load addresses by "Id" like it was done before

a.m. wrote:

Hi Andrei,

Yes, it is kind of a validation but the point is that this validation was not present in the previous versions of nopCommerce.  I am sure there will be customers that might have addresses that don't belong to their address book so after the upgrade they will end up with an error.

Anyway thank you for looking at this!

Thanks,
Boyko

a.m. wrote:

Actually overriding is not an option for us as if for some reason the store owner disables/uninstalls our plugin then this will leave the default checkout to be broken (there will be addresses for customers that are not part of the address book).
In other words we have no option but to keep some "temp" addresses in the address book of the customers but I am sure that our clients will not like this behavior and will start to complain about it. That is why I see this validation as a constraint or limitation especially for plugins that want to modify the checkout process.

Thanks,
Boyko

Nop-Templates.com wrote:

Please check these changes.

RomanovM wrote:

Hi Maxim,

Thank you for implementing it!
It works fine now!

Thanks,
Boyko

Hi guys,

Congratulations for the release of nopCommerce 4.3!

We started testing our products with nopCommerce 4.30 and while testing the upgrades of our theme demos (more than 30 databases) we ended up with several errors that I am sure some nopCommerce users might also experience during their upgrades.

There are a couple of places in the new version of nopCommerce where we see some kind of validations or assumptions that didn't exist in the previous versions of nopCommerce.

Here are some of them:

1. If you have a language resource with the same key (there is no constraint in the database to ensure unique keys) then nopCommerce 4.30 will throw an exception because of this code assuming the resource name is unique:

var lsNamesList = _lsrRepository.Table
                .Where(lsr => lsr.LanguageId == language.Id)
                .ToDictionary(lsr => lsr.ResourceName, lsr => lsr);

2. We already discussed the addresses validation and shared my concerns but we ended up with databases that have customers (probably started the checkout but didn't completed their orders maybe) which addresses are not in their address book and for these customers the store could not be loaded at all.

I am not saying that these validation are a bad thing but since the previous versions allowed more freedom it is absolutely possible that some database will not match the new rules and many store owners will have issues after the upgrade to 4.30.  So I think the upgrade script should also validate the database and fix the issues beforehand.

We will add fixes in our upgrade scripts and ensure that our clients will never experience these issues but I think it will be helpful for nopCommerce in general to ensure that the databases match the new validations during the upgrade and that is why I write to you.

For example here is a script that ensures that no user has an address that is not in their address books.

UPDATE Customer
SET Customer.BillingAddress_Id = NULL, Customer.ShippingAddress_Id = NULL
WHERE (Customer.BillingAddress_Id  IS NOT NULL
AND NOT EXISTS (SELECT 1 
                   FROM CustomerAddresses
                   WHERE Customer.BillingAddress_Id = CustomerAddresses.Address_Id))
OR
(Customer.ShippingAddress_Id  IS NOT NULL
AND NOT EXISTS (SELECT 1 
                   FROM CustomerAddresses
                   WHERE Customer.ShippingAddress_Id = CustomerAddresses.Address_Id))

You can also easily  delete any duplicate language resources during the upgrade to ensure there are no duplicates and that the store will be loaded without errors.

Hope this helps and if you consider adding these the official upgrade script I am sure this will save a lot of problems to many store owners that upgrade to the new version :)

Thanks,
Boyko

How do I write any migrations that alter tables?  Say I release a 4.3 version and then I need to release another update that adds new features.  I need to be able to add columns to a table.  From what I can tell MigrationManager only supports CREATE TABLE.

Also, what is going to happen when someone is upgrading from 4.2 to 4.3 and my plugin tables already exist?  Their database won't have my MigrationVersionInfo row so it will try to execute and create those tables.

I do like this approach more than .sql scripts

AndyMcKenna wrote:
Speaking from my experience with using FluentMigrator in previous version of Nop and migrating to the newest version, I think this is a short-sighted from Nop team, probably due to the development time for the release. It is great that we can now use FluentMigrator without including it within the plugin but the way Nop black-boxed things, in my opinion, is not a great idea. It should be like .Net Core approach where everything can be extended freely. In this case, the data layer of Nop is too restricted at the moment.

Back to your first question, if you need to alter anything, then you need to inherit the Migration base class of FluentMigrator directly instead of AutoReversingMigration since AutoReversingMigration only supports certain expressions.
An example:

[NopMigration("1", "Alter migration")]
public class AlterTableMigration : Migration
{
    public override void Up()
    {
        Alter.Table("TableName").AddColumn("SomeDate").AsDateTime().NotNullable();
    }
}

For the second one, at the moment, Nop will automatically scan-in any assembly, find the one that inherits Migration class (AutoReversingMigration is also a child of Migration), and then run the migration. This approach is very flawed if your plugin has used FluentMigrator before this release like mine did. Also, every migration from every plugin that uses this new approach is put into one big MigrationVersionInfo table and to me that is very bad. So this means there are two problem: How to avoid re-run of migrations that is already existed and how to avoid putting everything into one migration version table?
The answer for the first one is currently not implemented yet as I already put into a feature request and seems like it will be a long time for it to be introduced. At the moment, to skip one migration, you can only have SkipMigrationOnUpdate and by the name itself, Nop would only skip those with that attribute during plugin update process (this is a new process introduced within the 4.30 version). So if you cannot wait for Nop team to implement this feature, then you can modify the existing code base and add an additional attribute, then in the MigrationManager class, within the ApplyUpMigrations and ApplyDownMigrations, check for the attribute like they are doing with the SkipMigrationOnUpdate and ignore the migrations.
For the second problem, it is better if Nop exposes the migration runner in some manner so one can hooks into it and run the migrations of the plugin within the plugin startup pipeline. That way you can modify the runner settings (i.e change the migration table, ...) as you like. I don't know what the best implementation for that in Nop, but a workaround that I have implemented so far worked wonderful for me.

public static IServiceProvider CreatePluginMigratorService()
{
    var dataSettings = DataSettingsManager.LoadSettings();

    return new ServiceCollection()
        // Add common FluentMigrator services
        .AddFluentMigratorCore()
        .ConfigureRunner(rb => rb
            // Add SQLite support to FluentMigrator
            .AddSqlServer()
            // Set the connection string
            .WithGlobalConnectionString(dataSettings.ConnectionString)
            // Define the assembly containing the migrations
            .WithVersionTable(new CustomMigrationTable())
            .ScanIn(typeof(MyPlugin.AddNewTableMigration).Assembly)
            .For.Migrations()
            .For.VersionTableMetaData())
        // Enable logging to console in the FluentMigrator way
        .AddLogging(lb => lb.AddEventSourceLogger())
        .Configure<RunnerOptions>(opt =>
        {
            // Only run the migration if the tag is empty or equivalent to MyPluginTag
            opt.Tags = new[] { "", "MyPluginTag" };
        })
        // Build the service provider
        .BuildServiceProvider(false);
}

private void ApplyMigration(IApplicationBuilder application)
{
    try
    {
        var migrationServiceProvider = CreatePluginMigratorService();
        using var serviceScope = migrationServiceProvider.CreateScope();
        var runner = serviceScope.ServiceProvider.GetRequiredService<IMigrationRunner>();
        try
        {
            runner.MigrateUp();
        }
        catch (MissingMigrationsException)
        {
            // ignored
        }
    }
    catch (Exception)
    {
        // ignored
    }
}

public void Configure(IApplicationBuilder application)
{
    ApplyMigration(application);
}

If you want to use this workaround, you would need the skip migration above for this to work (apply all your plugin migrations with the custom attribute) since Nop will try to automatically run your migration and FluentMigrator will throw duplicate error.

Thanks for the "inherit from Migration instead of AutoReversingMigration" advice, that looks like it will do exactly what I need.

I think for skipping the migration for 4.20 store owners coming to 4.30 with my plugin, I'll just have to recommend manually inserting into the MigrationVersionInfo table just like the official 4.30 upgrade script is doing:

INSERT [MigrationVersionInfo] ([AppliedOn], [Description], [Version]) VALUES (CAST(N'2019-12-30T09:12:42.0000000' AS DateTime2), N'AddAffiliateAddressFK', 637097594562551771)