tastyigniter / ti-ext-api

Manages and generates RESTful APIs for TastyIgniter
MIT License
16 stars 23 forks source link

BUG Create orders isn't working with menu_items #15

Closed LonnyX closed 3 years ago

LonnyX commented 4 years ago

I tried to create an order via the API but it looks like the order_menus isn't created.

Here is an example of data:

{
    "status_id": 4,
    "order_type": "delivery",
    "payment": "cod",
    "menu_items": [
        {
            "name": "Napoli",
            "price": 10,
            "quantity": 1
        },
        {
            "name": "Bolognese",
            "price": 10,
            "quantity": 2
        }
    ]
}

Firstly, sending menu_items in the request body is causing an error.

now even without the menu_items, it looks like the restAfterSave from Order.php isn't called but instead from the RestController.php itself. And this method is empty in the RestController

It looks like a bug

ryanmitchell commented 4 years ago

menu_items isn't in the right format, see here: https://github.com/tastyigniter/TastyIgniter/blob/ed14ed725e3acc94fceec547f279d1fe0bcb3c3a/app/admin/traits/ManagesOrderItems.php#L111

I'll look into the restAfterSave() issue

LonnyX commented 4 years ago

@ryanmitchell Thank you for the restAfterSave, it worked.

But the menu_items is still an issue, look here: https://github.com/tastyigniter/ti-ext-api/blob/master/apiresources/Orders.php#L56

You are requiring menu_items but it's considered as an attribute of Order and it cause this error:

"Array to string conversion","debug":{"line":419,"file":"\/project\/vendor\/laravel\/framework\/src\/Illuminate\/Support\/Str.php",

You can try by yourself, just copy & paste the payload above

ryanmitchell commented 4 years ago

I wont have time to get to this for a while - so if you have time to debug feel free to step through it and add to the PR i've opened.

ryanmitchell commented 4 years ago

@sampoyigi I traced this back to https://github.com/tastyigniter/TastyIgniter/blob/ed14ed725e3acc94fceec547f279d1fe0bcb3c3a/app/admin/traits/FormModelWidget.php#L152

Might be something you want to handle here - if the value cant be converted to string it throws an error.

As for the actual issue, I'm about to push a change that handles it.

LonnyX commented 4 years ago

@ryanmitchell sorry I was waiting to be at home to fix this but you did it before me. But wait it's not over :P there is still a bug in this workflow

ryanmitchell commented 4 years ago

Sorry my bad. I missed part of my commit - shouldn’t be a need for what you have done.

ryanmitchell commented 4 years ago

I've pushed my update now

LonnyX commented 4 years ago

Still not finished, now that we have Order & Menu items, we also need the totals. Without that the Order list on the Admin panel will crash

I can fix this one but I need to know if we offer a complete freedom to the API (so we accept eventual inconsistency) or we can be a little bit more restrictive and calculate this in the controller

ryanmitchell commented 4 years ago

I think we should calculate subtotal and total in the controller. @sampoyigi any thoughts?

LonnyX commented 4 years ago

To avoid repeating logic, I think we should use the OrderManager class. Let's wait for @sampoyigi advices

ryanmitchell commented 4 years ago

Using OrderManager makes sense, but will also affect menu_items as they will need to be added as cart items (meaning CartManager will also likely be a requirement depending on the implementation). It also leads to complexities around discounts etc, and we would probably need to validate the order menu_items before inserting the order (currently its after).

It might be worth bringing the ordermanager/cart manager calculation functionality into the main Orders_model so it can be used in both contexts.

As you said, lets see what Sam thinks.

sampoyigi commented 4 years ago

Sorry I’m late.

To avoid repeating logic, I think we should use the OrderManager class.

@LonnyX It will be hard to not repeat logic, as Ryan pointed out, the approach for creating order on the frontend differs through api.

Using the OrderManager class we can follow one of these;

Approach 1: We use OrderManager class with a new method saveOrderForApi Approach 2: We extend OrderManager within the api extension and replace the logic within saveOrder

I prefer the second approach as it lets us maintain the api’s create order logic within the extension. We could also move some of the saveOrder logic into smaller functions if we find that we are repeating too much.

LonnyX commented 4 years ago

@sampoyigi What i did locally is adding a new argument $fromAPI = false // by default for addOrderMenus() and addOrderMenuOptions() and it works.

But to be honest your second approach is much better

ryanmitchell commented 4 years ago

Great, lets go for approach #2 then. @LonnyX do you want to do an initial commit and we can all contribute from there?

LonnyX commented 4 years ago

Yep, I will try to push tonight :)

LonnyX commented 4 years ago

@ryanmitchell how can we extend OrderManager within this package when we cannot include the cart entension via composer here?

ryanmitchell commented 4 years ago

` use Igniter\Cart\Classes\OrderManager;

class APIOrderManager extends OrderManager { public function saveOrder() {

} } `

ryanmitchell commented 4 years ago

Then (assuming you put it in api/classes) within the file you want to use it

use Igniter\Api\Classes\APIOrderManager;

...

$orderManagers = APIOrderManager::instance();