-
-
Notifications
You must be signed in to change notification settings - Fork 761
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
project_task_scheduling: Module for automatic task planning #419
project_task_scheduling: Module for automatic task planning #419
Conversation
Hey @ernestotejeda, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
comodel_name='hr.employee', | ||
relation='project_employee_rel', | ||
column1='project_id', | ||
column2='employee_id', |
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.
Thanks for these modules.
Isn't it easier and concise code if you don't specify relation, column1, column2
attributes
The same in another m2m below
Thanks! I think @tarteo is also interested in this one! |
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.
Is it possible to have runbot fixed so that we can start testing it?
<field name="model">project.project</field> | ||
<field name="inherit_id" ref="project.edit_project"/> | ||
<field name="arch" type="xml"> | ||
<notebook> |
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 think you are missing position="inside"
here?
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.
'inside' is the default value, it is not necessary to specify it
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.
Thanks!
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 still need extra time to continue the review and test the module... will let you know
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.
OK, thanks to you. I'm still trying to have runbot fixed
"demo": [ | ||
"demo/project_task.xml", | ||
], | ||
'installable': True, |
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.
Could you put double quotes here, to be consistent with the rest?
|
||
call_args['start_time'] = current_dt.time() | ||
intervals = calendar._get_day_work_intervals(current_dt.date(), | ||
**call_args) |
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.
Could you explicitly pass start_time
here, instead of **call_args
?
The Runbot failure seems not related with the modules:
Probably something related to the CLA? #419 (comment) @OCA/community-maintainers |
I don't think so, @astirpe. Let me check if I'm able to see anything. |
@gurneyalex do you know why runbot is not working here? We get 502 if the build is correct and we press the blue button, or directly the build is not done and the message that appears in the logs is:
|
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.
This module requires proper documentation (functional and technical). As is, it is very difficult to use.
@@ -0,0 +1,5 @@ | |||
This module allow you: |
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.
add blank line
@@ -0,0 +1,36 @@ | |||
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). |
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.
use standard short headers
@@ -0,0 +1,2 @@ | |||
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). |
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.
no headers in init.py files
@@ -0,0 +1 @@ | |||
This module allow you to execute an automated project task scheduling. |
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.
Could you improve the description and add a use case?
default=lambda self: self._default_start(), | ||
required=True, | ||
) | ||
cooling_ratio = fields.Float( |
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.
Could you add a help message?
|
||
if total_hours_dy > self._MAX_HOURS_DELAYED: | ||
raise ValidationError(_( | ||
'Maybe some tasks have a very long "Initially Planned Hours"' |
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.
You provide the explanation but not the actual error
interval = self._get_interval(task.planned_hours, employee, start) | ||
return interval, index | ||
|
||
def _greedy_distribution(self, state, init_index=0): |
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.
Please add docstrings to all methods
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.
Very cool module! I couldn't test it on Runbot but will after it's fixed.
<field name="name">project.task.scheduling.timeline</field> | ||
<field name="model">project.task.scheduling</field> | ||
<field name="arch" type="xml"> | ||
<timeline date_start="datetime_start" |
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.
Would be nice to also print the picture of the employee here.
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, we could use the same timeline definition as the one in project_timeline + rest of the extras. The dependency handling is another thing in the roadmap. I'm not sure if directly depend on all the other modules or develop several modules that plugs on the main one.
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.
Well, when you have 50 or 100 task a picture is not needed. Because you need to see global over view of the planned schedule, so as much compacted is everything, better visualization.
self.ensure_one() | ||
|
||
def task_cmp(a, b): | ||
a_date_deadline = fields.Date.from_string(a.date_deadline) |
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.
This function could be working fine even without converting to datetime (no need of fields.Date.from_string()
)
for it in range(1000): | ||
intervals = calendar._get_day_work_intervals(current_dt.date(), | ||
current_dt.time(), | ||
**call_args) |
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.
Please replace the old style **call_args
with the explicit parameters.
return self._state_obj(tasks_list, tasks_dict, employees_dict, | ||
state.evaluation) | ||
|
||
def _get_interval(self, hours, employee, start, stop=False): |
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.
You cold move this helper method into the hr.employee
model and invoke this method from a singleton employee record.
This way you reduce a bit the code in this wizard and make the method reusable for other cases.
You may consider to move this method to module project_task_assignment
. What do you think?
_name = "project.task.scheduling.wizard" | ||
_description = "Project task scheduling wizard" | ||
|
||
_state_obj = namedtuple('State', ( |
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.
Could you separate the scheduling algorithm from the wizard?
An idea could be to move the _state_obj
structure and the related algorithm methods to a separate abstract model which represents the scheduling algorithm. You could then make the wizard inheriting from that model.
This way you will make the module more easy to be customized or extended.
What do you think?
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 seems good to me, thank you for the idea, i think it's good, I already have plans to work on this, but I have to organize the ideas.
<footer> | ||
<button name="action_accept" type="object" string="Accept" | ||
class="oe_highlight"/> | ||
or |
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.
This or
string is old-fashioned 😆
|
||
_logger.info('*****************************') | ||
|
||
def simulated_annealing(self, init_temp, final_temp, iterations, |
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.
Did you consider to adopt some external library to implement this algorithm?
Did you also consider to adopt any other different algorithms for the allocation, eg.: any algorithm for a knapsack problem optimization? I see that the algorithm you adopted is a high resource consumption one.
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.
You may want to have a look at this:
https://developers.google.com/optimization/scheduling/employee_scheduling
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 have not seen any library that uses this algorithm.
If there are other algorithms for the assignment, but these are also of high levels of resources. It is a NP-hard problem and that algorithms are used that give meta-heuristic solutions, like this one (simulated annealing). There are variants of the knapsack problem that can be dynamic solutions, that is, they solve the problem in polynomial order, but that type of solutions can not be applied here.
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've tested the functionality. It's really cool but some fields are not clear to me e.g. cooling time (is that the time between tasks?). Some help texts would be nice.
When I apply the schedule it uses new introduced fields (date_start_assignation and date_stop_assignation) instead of the existing fields date_start and date_end. What is the reason for it?
<?xml version="1.0" encoding="utf-8"?> | ||
<!--License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).--> | ||
<odoo> | ||
<data> |
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.
@ernestotejeda IMHO remove <data>
</record> | ||
|
||
</data> | ||
</odoo> |
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.
@ernestotejeda Add new blank line
], | ||
"data": [ | ||
"views/project_task_scheduling_menu_views.xml", | ||
"wizards/scheduling_wizard_views.xml", |
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.
@ernestotejeda Add last
@@ -0,0 +1,44 @@ | |||
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | |||
from odoo import api, fields, models | |||
from odoo.tools.translate import _ |
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.
@ernestotejeda IMO Remove here and Add Directly from odoo import api, fields, models, _
from odoo import api, fields, models | ||
from odoo.exceptions import ValidationError | ||
from odoo.tools.float_utils import float_compare | ||
from odoo.tools.translate import _ |
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.
@ernestotejeda IMO same here
@nikul-serpentcs please use "Start a review" option for grouping all your comments together instead of hammering with an email per comment. |
@pedrobaeza Ok, I will use "Start a review" option for grouping all comments together. Thanks for suggestion! |
230115c
to
5578e2d
Compare
Hey @ernestotejeda, Appreciation of efforts, |
@tarteo These new fields to store the start and end times in which they have planned that they will perform the task for an employee. The date_start and date_end fields have other objectives. For example, when changing the user of the task (field 'assigned to'), the date_start field is updated with the current date, I do not want that to happen with the start date of my task planning. |
@ernestotejeda I agree with @tarteo in using standard fields (and thus, |
Well, I've reviewed and I think we can use those fields for planning. So, I'm going to do it |
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.
Thanks! The new cooling_ratio selection is now much more user friendly.
(functional and code)
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.
Here some more code review.
Are you working also on #419 (comment)? Shall we expect some extra developments, or this version is the final one?
class Task(models.Model): | ||
_inherit = 'project.task' | ||
|
||
closed = fields.Boolean( |
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.
Could this field be moved to module project_stage_closed
?
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 think you all right, i have made a Pull Request to add closed field in project_stage_closed. When that PR is merged, then I remove the field of here.
PR: #434
<button name="action_set_assignation" | ||
type="object" | ||
string="Set assignation" | ||
icon="fa-check" |
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.
Clicking on this button I would expect that the icon changes to something different, to notify that the assignation is applied. As it is right now, the user doen't know which task is already set for assignation and which not.
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.
Same discourse for the global Set scheduling
button.
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 have removed this button because you can remove a task assignation proposal from the list view if you don't want to set it as definitive. Then, when you remove all task assignation proposal that you don't need, you can approve the complete proposal (that is: approve all task assignation proposal in this scheduling proposal) by clicking on approve button in the header of the form view.
I have added a state field to proposal in order to distinguish between an approved proposal and a not approved proposal.
Could you review that, please?
|
||
return employees_dict | ||
|
||
def _obj_func(self, state): |
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.
Could you rename this function to something more self-explanatory?
duration = fields.Float( | ||
compute='_compute_end' | ||
) | ||
delayed_tasks = fields.Float( |
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.
Shouldn't this be an integer?
Change label of the button 'Set scheduling' by 'Approve' . Add states to proposals and remove 'Set assignation' button from task assignation proposal
Change the name of a function and improve its docstring
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.
Small Change
def setUp(self): | ||
super(TestProjectTask, self).setUp() | ||
|
||
self.task_3 = self.env['project.task'].browse( |
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.
@ernestotejeda IMO Add one variable and use this obj self.env['project.task']
.
Ex: self.project_task_obj = self.env['project.task']
self.ref("project_task_employee.restricted_task_1")) | ||
|
||
def test_employee_domain_ids(self): | ||
jth = self.env['hr.employee'].browse(self.ref("hr.employee_jth")) |
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.
@ernestotejeda same as here
class ProjectProject(models.Model): | ||
_inherit = 'project.project' | ||
|
||
employee_ids = fields.Many2many( |
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.
Shouldn't we link to res.users
here instead? Since https://github.com/odoo/odoo/blob/7e5d5045bfa7752176dd8f19c546ff6b19841381/addons/project/models/project.py#L439
@@ -0,0 +1,2 @@ | |||
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). | |||
from . import test_project_task |
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.
Please add newline
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.
After the comment line? Why?
@@ -0,0 +1,41 @@ | |||
# Copyright 2018 Tecnativa - Ernesto Tejeda | |||
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | |||
from odoo.tests.common import TransactionCase |
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.
Newline would be great
@alexey-pelykh forget about |
|
Sorry for the silly question. When I add a dependency to a task, like task2 depend on task1, I expect to finish first task1 before starting with task2. Is that correct? I cannot get the schedule to achieve that, probably my fault. |
@aheficent dependencies in this scheduling is not yet covered. |
BTW I miss the dependency to pygment python library in manifest file. |
Hi @pedrobaeza here is missing your review |
This requires a rework over the base module |
Cc @Tecnativa