search

vendrcontrib / vendr-wishlists

[WIP] Wishlists for Vendr, the eCommerce solution for Umbraco v8+
MIT License
0 stars 1 forks source link

[Coding Style] Prefer explicit DB schema definitions over automatic DTOs #5

Closed mattbrailsford closed 3 years ago

mattbrailsford commented 3 years ago

At the moment, the create table migrations use DTOs to create the database tables but it is considered better practice to explicitly declare your tables like we have in Vendr Reviews https://github.com/vendrcontrib/vendr-reviews/blob/dev/src/Vendr.Contrib.Reviews/Migrations/V_1_0_0/CreateReviewTable.cs

The reason for this is that migrations need to be immutable but DTO's can change and you can get into a scenario where this could break an upgrade. ie, take the following possible migration updates

  1. Create table from DTO
  2. Migration modifies all values for column X on DTO table
  3. Column X is no longer needed so is deleted

This would cause an issue for fresh installs where they run through all of the migration steps as you'll likely remove the deleted column from the DTO and so on fresh installs this won't create the column for migration 2 to run which modifies that column.

bjarnef commented 3 years ago

Yes, I think this is similar to what @NikRimington asked for in this issue: https://github.com/vendrcontrib/vendr-wishlist/issues/1#issuecomment-711168948

I haven't really updated this with the latest changes we made in Vendr Reviews package, so we can probably re-use much of the recent changes in this project as well 😁😎

bjarnef commented 3 years ago

@mattbrailsford I have updated this in https://github.com/vendrcontrib/vendr-wishlist/commit/81cdeb52790496abc46c6696045d9d413de2cd5c and I think it was the same @NikRimington asked for in https://github.com/vendrcontrib/vendr-wishlist/issues/1

Not sure how much we need in this table yet. Should it be possible to re-use the order functionality in Vendr core and just have a reference via orderGuid? Do we want to set a customerReference here although is exists on order (for members logged in) - maybe this has some benefits when you want to get wishlists for a specific customer?

NikRimington commented 3 years ago

I'm inclined to say we do need the table as it can store wishlist specific things such as a name for the wishlist rather than trying to use fields on an order to do the same thing. It also means you can get all wishlists for a customer quickly, else you have to filter all the orders to get only the wishlist ones if you "just" used the orders table. In a big DB that has lots of orders built up over time, I imagine this being quite slow.

mattbrailsford commented 3 years ago

I agree with @NikRimington here 👍🏻

And orderId column name 😉

bjarnef commented 3 years ago

I agree it would be great the have a separate vendrWishlist table - just not sure how many columns we need in this table, but it have created a basic table in the migration. The name column should probably be optional (unless we set a default value).

We can also add an orderId column (guid) to reference the created order. Should we have a customerReference as well or use if from the vendrOrder table? It is probably easier and faster to query wishlists for a specific customer, when having the customerReference in the wishlist table, but we might need to sync the property in wishlist and order table when a wishlist is created and if it e.g. would be possible to create wishlists as anonymous customer and transfer this to a customer/member after signup (in some ecommerce systems this is possible).

bjarnef commented 3 years ago

I have updated the code, so we now have the following columns: https://github.com/vendrcontrib/vendr-wishlists/blob/main/src/Vendr.Contrib.Wishlists/Migrations/V_1_0_0/CreateWishlistTable.cs#L26-L32

mattbrailsford commented 3 years ago

Yea, maybe the customerReference isn’t necessary. We can always do a join and have a result column property for easy access in code.

bjarnef commented 3 years ago

Okay, if it would be possible to have wishlists fro anonymous customers, can we get these where customerReference on order by default is null? Do we have an identifier for customer in cookie or session? In this scenario in might be fine that the customer only can have a single wishlist and without specifying wishlist name. I have just seen some examples where it is possible to create wishlist as anonymous customer, so I wonder if we have this scenario covered and if it will be easy enough to handle? 🤓

NikRimington commented 3 years ago

If we support anonymous wishlists, we probably want to force a name in that scenario. Anonymous wishlists should probably only be retrievable by putting in the id or some sort of identifier/code. What do we think?

bjarnef commented 3 years ago

I am not familiar with how wishlist for anonymous customers is implemented in various ecommerce systems, but here is an example of a Shopify app which seems to allow this for guest users: https://apps.shopify.com/wishlist-hero?surface_detail=store-design-wishlists&surface_inter_position=1&surface_intra_position=5&surface_type=category

bjarnef commented 3 years ago

I think this one was similar to the issue in https://github.com/vendrcontrib/vendr-wishlists/issues/1 and the migration has been updated, but feel free to raise a new issue if sø De need to modify the migration further 👍😎