blueimpact / kucipong

BSD 3-Clause "New" or "Revised" License
5 stars 2 forks source link

Combine store and store email tables #140

Closed cdepillabout closed 7 years ago

cdepillabout commented 7 years ago

This PR gets rid of the store_email table and just uses the store table.

It also fixes the error from #113 where the Admin user is not able to delete a Store user unless the Store user has already been verified.

If this PR gets merged in to master, then the following SQL commands must be run:

drop table coupon cascade;
drop table store_login_token cascade;
drop table store cascade;
drop table store_email cascade;

Commit 6aeb04e removes the store_email table. This commit changes a lot so it is pretty messy.

Commit cf606df fixes the error with deleting the Store user.

cdepillabout commented 7 years ago

Right now, the Stores, Coupons, Admins, etc are all being logically deleted, not phsyically deleted.

Currently, a Store's key is the email address for the Store user. When a Store is deleted, all that happens is that it's deleted_at column gets updated from null to a date. When trying to re-register a deleted Store, the email address conflicts (since it's the same as the email address for the Store that already exists in the database) and the error occurs.

Here are a couple ways we could fix this:

  1. Change the key of the Store table from the email address to a generic ID (probably an integer or UUID).
    • Since the key is no longer the email address, multiple Stores can be created that have the same email address. In practice, we'd probably only want one non-deleted Store for each email address, but we could ignore deleted Stores.
  2. Instead of logically deleting Stores, we could physically delete them.
    • We'd also have to be sure to delete everything that uses a Store as a foreign key. Right now I think this is just Coupons.
    • We'd have to make sure that our database gets backed-up frequently. We don't want an administration error to accidentally delete customer data.
  3. We could have a StoreHistory table and a CouponHistory table that records every change made to a Store or a Coupon.
    • This is like a combination of (1) and (2). It lets us keep a history while still being able to physically delete things.
  4. Switch to completely different storage paradigm, like event sourcing or datomic.

I think we should go with either (1) or (2). I think either of them would be the easiest to implement and work with.

Personally, I think (3) is generally an anti-pattern. If you want history information stored in the database, I think the best option is using event sourcing (4). However, it would take us too much time to completely rewrite the database layer of Kucipong. It would be interesting to try on a future project though ;-)

@arowM What do you think?

cdepillabout commented 7 years ago

@arowM I've fixed the errors pointed out by the two comments in commit 6c3815c and d7bfe9a. How about we merge this PR in, and figure out what to do about the Store key in a separate issue/PR?

arowM commented 7 years ago

@cdepillabout Good! I'll merge this PR. Could you create an issue to migrate to pattern (1) or (2), and create an separate issue to migrate to (4) with "low priority" label? I'm also personally interested in some sort of "Immutable data structure".