-
-
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
feature: cash deliveries should have amount of money in the task info for the messenger (API) #4672
Conversation
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 this is intended to be used via the API, the logic to load data should be moved to a data provider (or a state provider in more recent versions of API Platform). The logic returning a custom DTO shouldn't live in the TaskListRepository
, but in the data provider.
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.
The idea is OK, but it shouldn't be linked to #4337, which can be fixed by a few lines of code in TaskNormalizer
. Instead, it should be linked to another issue about fixing performance issues on the dispatch endpoints.
Related issues |
$task->getPrevious()?->getId(), | ||
$task->getNext()?->getId(), | ||
$task->getTags(), | ||
$task->isDoorstep(), |
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.
for your information isDoorstep is soon to be deprecated (i removed it in an other branch)
|
||
$tasksInTheSameDelivery = $deliveryId ? $tasksByDeliveryId[$deliveryId] : []; | ||
|
||
if ($task->isPickup()) { |
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.
cool!
I think i had the reflex to write it in SQL as to go faster but I think it is cool if we move it in a utility function we can test it better and it will act as a spec 👍
i just add a note here not to forget to use the DTO on TaskListUpdated event (live update for riders) |
@@ -295,20 +219,14 @@ Feature: Tasks | |||
"doneBefore":"@[email protected]().startsWith('2018-03-02T12:00:00')", | |||
"comments":"#bob", | |||
"updatedAt":"@[email protected]()", | |||
"isAssigned":true, | |||
"assignedTo":"bob", |
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.
we don't need these in-app ?
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 did not find any usages of them in the Courier part, only the Dispatch
{ | ||
$this->short_code = $shortCode; | ||
$this->name = $name; | ||
//FIXME; why do we have name and type with the same value? |
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.
it is legacy, i don't think it is used in the rider app anymore?
i spotted one usage here coopcycle-app/src/navigation/store/NewDeliveryForm.js in the source
{ | ||
use NormalizerAwareTrait; | ||
|
||
private const ALREADY_CALLED = 'MyTaskMetadataDtoNormalizer_ALREADY_CALLED'; |
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.
what is the reason for these ALREADY_CALLED?
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 checked in the database, and on my local instance all non-foodtech orders have payment method type For now, it will be fixed by displaying only cash payments in the app: coopcycle/coopcycle-app#1904 |
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.
Can we remove the My
prefix please?
in this case what should be the name? |
I used it to match coopcycle-web/src/Action/MyTasks.php Line 13 in 3644a3e
I'll merge it as it is to start beta-testing, but let's discuss the name. I can rename all of them. How about |
No description provided.