CGSmith-LLC / shipwise-api

Shipwise API
Other
0 stars 0 forks source link

Orders history #139

Closed bohdan-vorona closed 1 year ago

bohdan-vorona commented 1 year ago
  1. Previous DB table is changed.
  2. Orders History is removed from our API.
  3. Each event is a separate class in common\models\events.
  4. Events are mounted via traits to reduce the amount of used code in our AR models.
  5. Each event can be enabled or disabled via a flag in a specific *Event.php class.
  6. OrderHistory class - used scenarios for each event.
  7. It will be easy to add new events in the future.
  8. See the examples below - there are keys in the arrays = attributes in the DB. But it's possible to replace them with models' label names.

Events:

events

Traits:

traits

Enable/Disable:

en-dis-event

Added events:

ONE IMPORTANT MOMENT:

I started implementing the logic via one event like one "Order is created" with data of Address and Items together. But then I had to split it into:

Why:

  1. We use transactions. And, for instance, when a record of Order is created, it has an empty address_id.
  2. Potentially I could tackle it somehow but it would create "very bad" code when I would need insert my events in controllers/transactions/etc. At the moment events are used only in AR models. Only the event OrderViewed is used in the controller OrderController -> actionView.

notes-4 notes-3 notes-2 notes-1

cgsmith commented 1 year ago

Reviewing today. I agree that the address makes it tricky.

I will create an issue to move the address to the order table which is where it should be IMHO. I'll create the issue and as a team can discuss.

cgsmith commented 1 year ago

I found a strange issue when an address is updated but an item is missing.

Let me know if this picture makes sense. Other tests appear to work fine. I think it could be related to some orders not having items in my test DB?

testing order history

bohdan-vorona commented 1 year ago

@cgsmith

I found a strange issue when an address is updated but an item is missing.

Thanks, I will try to reproduce the issue and fix it.

bohdan-vorona commented 1 year ago

I found a strange issue when an address is updated but an item is missing.

Let me know if this picture makes sense. Other tests appear to work fine. I think it could be related to some orders not having items in my test DB?

@cgsmith It seems that you have one Address model for several Order models. I thought we have a 1-to-1 relation, isn't it? Can you please recheck this fact on your side?

order-address

cgsmith commented 1 year ago

Yes you're correct. I remember now. It used to be one to many but is now one to one. This is technical debt and the address should be 1 to 1.

bohdan-vorona commented 1 year ago

@cgsmith Thanks. I will add order by ID DESC anyway.

cgsmith commented 1 year ago

@cgsmith Thanks. I will add order by ID DESC anyway.

Good workaround. Please note it as a comment on why it is id desc

bohdan-vorona commented 1 year ago

Please note it as a comment on why it is id desc

@cgsmith

Pardon maybe I didn't understand you correctly) Let's clarify)

So, the reason that there was an Address for several different Orders. During updating the Address, I'm searching for a specific Order by the field address_id. At the moment I use this code:

$order = Order::find()
            ->where(['address_id' => $this->owner->id])
            ->orderBy(['id' => SORT_DESC])
            ->one();

It means that I consider we have a 1-to-1 relation.

A) If we're going to use a 1-to-1, it would be better to keep the current code with one(). B) Or, I can replace one() with all() but in this case, all orders with this address_id will have "Address is changed".

cgsmith commented 1 year ago

I just meant to have a comment in the code why we are doing the id => desc - that way in the future we don't remove it by accident.

bohdan-vorona commented 1 year ago

Address for several different Orders

@cgsmith Got it now) Added)