Skip to content
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

Calculate the distance for a task into a tasklist even when no vehicle is set #4805

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Atala
Copy link
Member

@Atala Atala commented Dec 17, 2024

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do you plan to use distance and CO2 emissions?

I'm not sure that putting them into a Task entity is a good idea. It seems to me that distanceFromPrevious, at least, belongs more to a TaskCollectionItem, as it depends on the position of a Task in a list and should be managed together.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about that as well.

At the time I put it on Task directly, as we are for real interested only on the last value: a task may be assigned several days in a row, but actually only the last assignment if several make sense (when we accomplish the task for real). so in reality there is only one value we are interested into.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are for real interested only on the last value

Then it seems a waste of time to recalculate all these values all the time. Maybe subscribe to a task:done event and calculate them only then? (I'd also rename the fields in that case to something in the past like traveled_distance, co2_consumed)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I put it there because we already call the geocoder at this point, so we have the distance data without an extra call.
however this is true that we update all these tasks objects so it might result in more SQL calls than before. but i am not sure about that, I can check.
maybe it makes more sense architecture-wise to move the logic to task:done handler, you ll prefer that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it’s better if they are set after task:done event, makes the logic behind these fields a bit more clear

@Atala
Copy link
Member Author

Atala commented Dec 26, 2024

Issue here coopcycle/coopcycle#167

@Atala Atala force-pushed the calculate-distance-on-each-task branch 3 times, most recently from ad3c358 to c63e2e2 Compare January 15, 2025 08:31
@Atala Atala force-pushed the calculate-distance-on-each-task branch from c63e2e2 to 43ffa3f Compare January 15, 2025 09:12
@Atala Atala requested a review from vladimir-8 January 16, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants