-
-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Order creation for stores #4778
base: master
Are you sure you want to change the base?
Conversation
…in should see an incident)
…er created by a recurrence rule (Dispatch page)
…\EmbedController::accessControl()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and I am quite sure it didnt create incident for the following case: API call to reate a single delivery (through OAUTH) -> I guess it should?
It doesn't create incident for delivery CSV import, juste create 0€ deliveries but I think it is normal we agreed on this?
there is also a small remark for the EmbedController, as it doesnt not create an Incident but maybe it is a corner case. actually the biggest problem i forgot is that Embed form is not linked to as store, so maybe we can create an issue for this
@@ -38,6 +39,7 @@ | |||
*/ | |||
class EmbedController extends AbstractController | |||
{ | |||
use AccessControlTrait; | |||
use DeliveryTrait; | |||
|
|||
public function __construct( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on L. 188
of this file you still want to return a form error when NoRuleMatchedException
is thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a customer ordering via a form has to pay immediately, I guess it still makes sense to have an error for now
src/Form/DeliveryType.php
Outdated
@@ -149,8 +165,8 @@ public function buildForm(FormBuilderInterface $builder, array $options) | |||
]); | |||
} | |||
|
|||
$isDeliveryOrder = null !== $store && $store->getCreateOrders(); | |||
|
|||
$isDeliveryOrder = null !== $store; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the check is different than $order->isFoodtech() ?
actually i am not very sure this check is useful anymore?
this form is used only for lastmile/localcommerce + we are always creating an order now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the check. There is a way to reach this form for food tech orders too, so I removed Save
button for food tech orders in the last commit, so that nobody edits it by mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it, we have three types of orders:
- food tech ("shop"/restaurant)
- local commerce and last mile ("store")
- B2C via Order form
The difference is that only for "stores" does it make sense to edit a delivery, set arbitrary price, recurrence rules etc. For both food tech and B2C deliveries, it's a one-off order that is already paid so the page is read-only for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes indeed I agree - maybe add this message as a comment ?
ah yes actually you could access past foodtech deliveries from the "deliveries" panel and then access this form?
Fixed: