[Technical] Refactor relationship between Tours and TaskList

[Atala]

👋

there is a design issue with tours & tasklists, as they are both designed as list of tasks with independent indexes/position. it was very fine at the beginning, because having them both as list of tasks enable very easy prototyping. also very easy to prototype on web side without changing the app code. however now it causes serious bugs and DB inacurracy when there are some flaws in the client or somewhere in the code.

Current state

• we have two endpoints /api/tasklists and /api/tours to manage both entities
• TaskList and Tours are both list of tasks with an intermediate table CollectionItem that tracks the list of tasks

Problem(s)

• on the client side you have to modify both tasks in tours and tasks tasklist at the same time
  - if your code is mistaken (for example recently bug in the optimizer), you can reorder the taskslist without reordering the tour. if you dont update both objects on the client side then you may lose sync between the two, especially when you modify an existing tour
• there is no such concept of "assigned tour", because a tour could be in inaccurate state when some tasks are assigned some other are not.

Solution(s)

I tried to figure out a solution that could synchronize tour order and tasklist order but I could not. also what is complicated is that if you force the change of task orders or fail on the backend side then you have to reflect on the client(s).

So I propose the following :

• two endpoints /api/tasklists and /api/tours to manage both entities (which is good because it is simple)
• a tasklist becomes an ordered list of tasks and tours
• a tour is an ordered list of tasks

This will lead to simplification and I think it is the only way to have good synchronization between all these parts, but also to (big) changes in the clients (web and app) and maybe in the backend code (not 100% sure).

Thoughts?

[Atala]

• in the app we don't need to show tours.
• READ : return list of tasks to be app-compatible?
• have the API retro-compatible
• use a parameter to play with/without a parameter

[Atala]

On my end i was thinking of kind of keep the same interface as before for the TaskList object :

• a new table TaskListItems which have : position, FK to tours, FK to tasks
• TaskList have getItems method that returns either Task or Tour in an array
• TaskList getTasks() returns list of task, if instance is tour it insert the tasks of the tour

I think it will work well with the IDs from ApiPlatform, because thanks to this we know most of the time if the object is a task or a tour (with the use of getItemFromIri)

[alexsegura]

So ok at first I proposed that TaskList implements the Tree / Nested set behavior, but it seems actually overkill, because it would only have 1 level of depth.

So I think you proposal is 👍, i.e a TaskList can contains either tasks or tours. That's simpler, and this is what we need.

So, the ordering of a Tour is always stored in the tour itself, not in the TaskList.

[Atala]

Findings

A tasklist implements (Task)CollectionInterface by extending TaskCollection

These are the functions of (Task)CollectionInterface. It seems that we want to keep all of them implemented by TaskList so keep it as is. We need to modify getTasks so it takes the tasks from a tour if item is a tour.

getDistance();
getDuration();
getPolyline();
getTasks();
setDistance($distance);
setDuration($duration);
setPolyline($polyline);

here are the other functions that are implemented by TaskCollection

function getItems -> serialize the tasklist, should be used to serialize mixed of tours and tasks

function addTask -> very important for tasklist as it synchronizes task.isAssigned and task.assignedTo fields. so should work with the new tour thing as well.
function removeTask -> very important for tasklist as it synchronizes task.isAssigned and task.assignedTo fields. so should work with the new tour thing as well.
-> these changes will trigger the notifications in TaskSubscriber/EntityChangeSetProcessor.php

function setTasks -> should be renamed to setItems and set tours and tasks
function containsTask -> should be implemented to send notifs

function findAt -> seems not used anymore, could be deleted?

utility functions used to display a delivery state, so maybe no use here

function getTasksByType
function computeCancelled
function computeFailed
function computeDone
function computeDoing
function computeState

src/Doctrine/EventSubscriber/TaskSubscriber/EntityChangeSetProcessor.php

• code to send notifs when tasks are changing assignee + send websocket events.
• create the tasklist if not exists in TaskListProvider ? what is the use of this ? ($taskList = $this->taskListProvider->getTaskList($task, $newValue);) what is the the tasklistcache used for ?

Web dispatch

• code to hydrate the tasklists. I think we can remove the "flat version" of the tasklist alright
• code to handle drag'n'drop. it can be simplified a lot

App

Pull : api/me/tasks -> it should still work this way
Push : use centrifugo events task:assigned src/redux/middlewares/CentrifugoMiddleware/actions.js to update the tasks

Centrifugo/websocket

This will broke updateTask which will be called when task:assigned and task:unassigned are updated. because it updates tasklist directly but I think it is already broken with tours anyway. Make it work with tours right now or postpone it. we will need it in the app also.
-> we should use task_list:updated event (also in the web dispatch) to update TaskLists
-> this event is send in TaskCollectionSubscriber so... maybe we create a new listener maybe we continue tying to TaskCollection

TaskListNormalizer -> maybe delete it? it is used to "flattenItems" but maybe it is a legacy bug ?

TaskListCollectionDataProvider -> is used by tricargo at something like /api/stores/1 -> find the spec and see if it fits

Database

Option 1 : keep TaskList based on TaskCollection, just change TaskCollectionItem so it has extra foreign key to Tours (maybe rename stuff)
-> avoid data migration so it is more safe
-> TaskCollectionSubscriber can be kept as is, maybe less unintended bugs (maybe not)

Option 2 : move TaskList out of TaskCollection, create a new intermediate table TaskListItem for position tracking
-> cleaner
-> maybe setup 1-1 between Tour/Task and TaskListItem (prevent double assignment)
-> easier to build upon in the future (specific logic to tasklist)

[Atala]

TaskListItem

    <discriminator-map>
      <discriminator-mapping value="task" class="AppBundle\Entity\Task"/>
      <discriminator-mapping value="tour" class="AppBundle\Entity\Tour"/>
    </discriminator-map>

taskList->getItems() , taskList->setItems()

[Atala]

Actually I was stuck with the "inheritance" approach as Touris already a TaskCollection instance and I couldnt do multi-inheritance mapping both in PHP and doctrine

So i went with the "two foreign keys" approach

also i think it may be profitable for performance, as i will only need to fetch TaskListItemwhen seralizing tasklist, not the whole Task And Tour objects as for TaskCollection will automatically JOIN on TaskCollection and TOur/Delivery (i think because of https://www.doctrine-project.org/projects/doctrine-orm/en/3.1/reference/inheritance-mapping.html#performance-impact)