[IMP] project: implement new project task state by bastvdn · Pull Request #107593 · odoo/odoo

bastvdn · 2022-12-09T09:17:31Z

Specs Mark as Done

Overview

Before this PR the task state was fixed by the kanban_state field which was useful when you use the stage of the project as parts of a pipeline,
but not relevant when users are using stages as bucket lists. (specific examples at the end of the specs)

The goal of this PR is to provide users a way to mark their tasks as done with a simple button press,
while keeping the option to label a task as Approved, Canceled or Requesting changes like in the old kanban_state field.

The kanban_state of a task had no impact whatsoever on other tasks of the pipe, we would like to change that and make the task state have an influence on its dependent tasks.

The state will also have influence over the 'recurrent' tasks (to be implemented in Task #3084945)

If you want a better description of those changes with screenshot and colors check specs of:

Task-3084930

PRs:

See https://github.com/odoo/enterprise/pull/35359
See https://github.com/odoo/upgrade/pull/4367

Interaction with blocking tasks:

the closed values which mark the task as closed or finished:

Done
Canceled

The Open values when the task isn't finished yet:

In progress
Changes Requested
Approved
Waiting (which is not selectable)

Where to change the state of a task:

For kanban and form views: same place as kanban_state (bottom right of kanban card, top right of form view)
For list view: left of list (after task priority)
more details about the state widget in state field widgets part

Interaction with existing fields

is_closed: which was determined by the task.stage_id.fold, now a task is closed when in one of the following stages
- Done
- Canceled
  a closed task is considered as finished, the time of the closing will be stored in the date_last_stage_update field
is_blocked: a task is considered blocked if ANY of its blocking task is in one of the blocking states (more details about this in the following part Interaction with blocking tasks):
- in Progress
- Changes Requested
- Approved
- Waiting

!! important !! is_closed and is_blocked are not mutually exclusive, you can have a task that blocked and is closed at the same time, the reason why will be explained late

date_last_stage_update: this field is updated everytime the task goes into a closing state OR when the task changes stage.
We need to check that the value is updated in each case (using the already available filter)

Interaction with blocking tasks

the state of a task can now be changed by its blocking tasks following the logic:

if ANY of the blocking tasks is NOT closed (so its state is in one of the open values) the task is considered as blocked

if a task is blocked and NOT closed its state will switch to Waiting
- the Waiting state will display an unclickable hourglass icon on the task kanban/list views, once in the waiting state you can't change the state of the taskfrom the kanban/list views
- a blocked task state can be changed through the form view, so you can override the 'block' by choosing a closed state (only done or canceled)
  - once overriden, the task will change to the closed state the user wants, but the task is still blocked so in case where the user comes back to an open state, the task will automatically switch back to the waiting state (according to the state before the block)
if the blocking task switches to a non-blocking state, the task will not be considered as blocked anymore and its state will switch back to In Progress

Default values

the default value is always in progress

Special cases

when a task is moved from a stage to another one

if the state was in one of the open states (approved, changes requested, in progress ) the state goes back to In Progress
if not, the state stays the same

when a task is moved from a project to another one

the state goes back to In Progress
when a task is duplicated
if the state was in one of the open states (approved, changes requested, in progress ) the state goes back to In Progress
if not, the state stays the same

robodoo · 2022-12-09T09:18:19Z

xavierbol · 2023-01-10T08:27:59Z

+    'state',
+    'state_approval_mode'


if those fields are writable in project sharing then they are also readable. 🙂
That is, no need to add those fields in the readable fields. 🙂

odoo/addons/project/models/project.py

Lines 1347 to 1349 in b9f0963

@property

def SELF_READABLE_FIELDS(self):

return PROJECT_TASK_READABLE_FIELDS | self.SELF_WRITABLE_FIELDS

adr-odoo · 2023-01-23T10:03:47Z

Feel free to ping me when this PR is ready.

MissingNoShiny

Hello, congratulations for your work! 🤩
I left some comments 🙂

MissingNoShiny · 2023-02-21T08:29:19Z

Suggested change

task.is_closed = True if task.state in CLOSED_STATES else False

task.is_closed = task.state in CLOSED_STATES

😇

MissingNoShiny · 2023-02-21T08:29:52Z

If these are not used anymore, don't forget to remove them

MissingNoShiny · 2023-02-21T09:19:51Z

This is a bit too many if else, how about something like this?

# We build a dictionary that maps every state value to its key state_dict = dict(map(reversed, self.env['project.task']._fields['state']._description_selection(request.env))) search_domain.append([('state', 'ilike', state_dict.get(search, search))])

MissingNoShiny · 2023-02-21T09:29:47Z

Don't forget to remove this

MissingNoShiny · 2023-02-21T09:44:05Z

If I understand correctly, a state can only be either blocking or closed? Could we not just use CLOSED_STATES then? That way, if we add a blocking state but forget to add it in BLOCKING_STATES, it won't be a problem.

To avoid repeating information, you could even make CLOSED_STATES a dictionary, and use its items in the state field, like this:

CLOSED_STATES = { 'done': 'Done', 'canceled': 'Canceled', }

state = fields.Selection([ ('in_progress', 'In Progress'), ('changes_requested', 'Changes Requested'), ('approved', 'Approved'), *CLOSED_STATES.items(), ('waiting_normal', 'Waiting'), ], string='Status', copy=False, default='in_progress', required=True, compute='_compute_state', readonly=False, store=True, recursive=True, task_dependency_tracking=True, tracking=True)

The in operator on dictionaries checks the keys, so all your task.state in CLOSED_STATES would keep the same behavior

MissingNoShiny · 2023-02-21T10:21:54Z

Don't forget to remove this

MissingNoShiny · 2023-02-21T10:32:17Z

Don't forget to remove this

MissingNoShiny · 2023-02-21T10:44:17Z

Don't forget to remove this if it's not used anymore

MissingNoShiny · 2023-02-21T10:47:35Z

This means that the task will have the technical name of the state on hover (for example, "changes_requested"). I don't think that's what we want

bastvdn · 2023-02-22T10:10:15Z

Thanks a lot for the review :) very insightful
I fixed the majority of minor problems you pointed
Some of the problems will need a little bit more time but will be done short-term: (hopefully done by the 24 Feb)

write last implementation tests
remove all ref to kanban_state_label and legend_**
finish migration script

Here are some improvements that should be done in more long term

Remove all ref to is_closed and is_blocked
Some improvements for the frontend
- some changes on the progressbar
- fixed width for the tree view widget
- write Javascript tests
Create a tour

What would you say is the more important?

And i have a small question to finish, haven't you seen any problem with the access rights/groups? I did not implement this feature with the access rights in mind at all (it does not seem that important for that small of a feature but still.. may be important)

MissingNoShiny · 2023-02-23T11:59:00Z

Here are some improvements that should be done in more long term

Remove all ref to is_closed and is_blocked

Some improvements for the frontend

some changes on the progressbar

fixed width for the tree view widget

write Javascript tests

Create a tour

What would you say is the more important?

I would say tests/tours can wait a bit if necessary, eliminating visible problems is more important (and other bugs that might arise in testing).
is_closed and is_blocked should be removed in this PR if possible.

And i have a small question to finish, haven't you seen any problem with the access rights/groups? I did not implement this feature with the access rights in mind at all (it does not seem that important for that small of a feature but still.. may be important)

I didn't notice anything problematic besides the portal rights. Hopefully if there are big issues, they will come up during testing 🙂

xavierbol

Great job! Thanks for your work, I left some comments. 🙂

xavierbol · 2023-03-03T14:04:28Z

Suggested change

return

break

? 🤔

Otherwise you will exit the method and not the for loop

If i break there i'll just get out of the last loop and end up in the if task.state == '04_waiting_normal' condition.

xavierbol · 2023-03-03T14:13:24Z

I am wondering if we could not do something like this:

Suggested change

for task in self:

if task.allow_task_dependencies:

for dependent_task in task.depend_on_ids:

# if one of the blocking task is in a blocking state

if dependent_task.state not in CLOSED_STATES:

# here we check that the blocked task is not alread in a closed state (if the task is already done we don't put it in waiting state)

if task.state not in CLOSED_STATES:

task.state = '04_waiting_normal'

return

# if the task as no blocking dependencies and is in waiting_normal, the task goes back to in progress

if task.state == '04_waiting_normal':

task.state = '01_in_progress'

tasks_with_dependencies_feature = self.filtered("allow_dependencies")

dependent_tasks = self.env["project.task"].search([

("dependent_ids", 'in', tasks_with_dependencies.ids),

("state", "not in", CLOSED_STATES)

])

for task in self:

# if one of the blocking task is in a blocking state

if dependent_task.state not in CLOSED_STATES:

# here we check that the blocked task is not already

# in a closed state (if the task is already done we

# don't put it in waiting state)

if task.state not in CLOSED_STATES and task in dependent_tasks:

task.state = '04_waiting_normal'

elif task.state == '04_waiting_normal':

# if the task as no blocking dependencies and is in

# waiting_normal, the task goes back to in progress

task.state = '01_in_progress'

I didn't test the code

xavierbol · 2023-03-03T14:14:43Z

Should not it be in the _compute_state? Since the allow_task_dependencies is a setting in the project model

Mmmh i don't think so,
Here we need to catch the transition of allow_task_dependencies from True to False
-> in this case every task that was in waiting_normal is just resets to in_progress, How can you make this distinction in the compute_state? isn't the function always going to reset the task to in_progress when the allow_task_dependencies is False?

I don't think we have to know that we can just check if the allow_task_dependencies=False then we do the change accordingly, for instance, if I change a little bit your compute method:

@api.depends('depend_on_ids.state', 'allow_task_dependencies') def _compute_state(self): for task in self: dependent_open_tasks = [] if task.allow_task_dependencies: dependent_open_tasks = [dependent_task for dependent_task in task.depend_on_ids if dependent_task.state not in CLOSED_STATES] # if one of the blocking task is in a blocking state if dependent_open_tasks: # here we check that the blocked task is not already in a closed state (if the task is already done we don't put it in waiting state) if task.state not in CLOSED_STATES: task.state = '04_waiting_normal' # if the task as no blocking dependencies and is in waiting_normal, the task goes back to in progress elif task.state == '04_waiting_normal': task.state = '01_in_progress'

when allow_task_dependencies is False we have to do the same things than when we don't have any dependencies.

(FYI, break stops the for loop, I think here we want to go to the next task in self and so you have to use continue instead)

Ok does work indeed, i can remove the onchange allow_task_dependencies

xavierbol · 2023-03-03T14:17:18Z

I am wondering if it should not be in the _compute_state too.

Same thing as for allow task dependencies, here we are just trying to handle a change of stage,
how can you handle that in the _compute function?

you can let this onchange, you are right. 🙂

xavierbol · 2023-03-03T14:17:55Z

Same, it could be in the _compute_state, IMO
What do you think? 🤔

Same thing as for allow_task_dependencies and onchange_stage_id :p

you are right, however, I think the compute is triggered when the project changed because we have allow_task_dependencies (project_id.allow_task_dependencies) in the dependencies, so maybe this onchange is useless. 🤔

the compute is triggered but it won't reset the state unless it was in waiting_normal, we want it to reset under every circumstance (except if it is still blocked by task and the project allow_task_dependencies)

xavierbol · 2023-03-03T14:56:10Z

Suggested change

'state': '03_approved'

'state': '03_approved',

😇

xavierbol · 2023-03-03T14:56:19Z

Suggested change

'state': '1_done'

'state': '1_done',

😇

xavierbol · 2023-03-03T14:56:29Z

Suggested change

'state': '1_done'

'state': '1_done',

😇

xavierbol · 2023-03-03T14:59:49Z

Is it really needed?

I guess it is needed. 🤷🏼‍♂️

xavierbol · 2023-03-03T15:01:05Z

Then we can remove the div? 🤔

we are keeping the date_deadline finally (couldn't remove it because another view was referring to this div)

xavierbol · 2023-03-07T14:48:12Z

Why not doing this:

Suggested change

<xpath expr="//span[@t-esc='formattedValue']" position="replace">

<s t-if="['1_done', '1_canceled'].includes(this.props.record.data.state) and ['kanban', 'list'].includes(this.props.record.activeFields[this.props.name].viewType)" t-esc="formattedValue"/>

<span t-else="" t-esc="formattedValue"/>

</xpath>

<xpath expr="//span[@t-esc='formattedValue']" position="before">

<s t-if="['1_done', '1_canceled'].includes(this.props.record.data.state) and ['kanban', 'list'].includes(this.props.record.activeFields[this.props.name].viewType)" t-esc="formattedValue"/>

</xpath>

<xpath expr="//span[@t-esc='formattedValue']" position="attributes">

<attribute name="t-else">""</attribute>

</xpath>

? 🤔

What is the s element? 🤔
You didn't want to put span instead?

s is for striked value (when the task is done/canceled)

oh right 🙂

Why not doing this:

? 🤔

What is the s element? 🤔 You didn't want to put span instead?

Don't forget to check that when you have time, I think we can avoid doing a replace here. 🙂

Thanks in advance!

Yes sorry :) it's done

xavierbol · 2023-03-07T14:49:13Z

Suggested change

}

else {

} else {

xavierbol

Thanks again for your work, I left one comment and I will push a commit if you agree with the changes then you can squash it in yours. 🙂

xavierbol · 2023-03-07T16:13:51Z

I guess it is needed. 🤷🏼‍♂️

xavierbol

LGTM, thanks again for your work. 🤩 🥳

May I ask you to edit the PR message to avoid having a big DEPRECATED? Thanks in advance! 🙂

robodoo r+

Before this PR the task state was fixed by the kanban_state field which was useful when you use the stage of the project as parts of a pipeline, but not relevant when users are using stages as bucket lists. (specific examples at the end of the specs) The goal of this PR is to provide users a way to mark their tasks as done with a simple button press, while keeping the option to label a task as Approved, Canceled or Requesting changes like in the old kanban_state field. The kanban_state of a task had no impact whatsoever on other tasks of the pipe, we would like to change that and make the task state have an influence on its dependent tasks. The state will also have influence over the 'recurrent' tasks (to be implemented in Task #3084945) If you want a better description of those changes with screenshot and colors check specs of: Task-3084930 PRs: See odoo/enterprise#35359 See odoo/upgrade#4367 ----------------------------------------- Interaction with blocking tasks: the closed values which mark the task as closed or finished: - Done - Canceled The Open values when the task isn't finished yet: - In progress - Changes Requested - Approved - Waiting (which is not selectable) Where to change the state of a task: - For kanban and form views: same place as kanban_state (bottom right of kanban card, top right of form view) - For list view: left of list (after task priority) more details about the state widget in state field widgets part Interaction with existing fields - is_closed: which was determined by the task.stage_id.fold, now a task is closed when in one of the following stages - Done - Canceled a closed task is considered as finished, the time of the closing will be stored in the date_last_stage_update field - is_blocked: a task is considered blocked if ANY of its blocking task is in one of the blocking states (more details about this in the following part Interaction with blocking tasks): - in Progress - Changes Requested - Approved - Waiting !! important !! is_closed and is_blocked are not mutually exclusive, you can have a task that blocked and is closed at the same time, the reason why will be explained late date_last_stage_update: this field is updated everytime the task goes into a closing state OR when the task changes stage. We need to check that the value is updated in each case (using the already available filter) Interaction with blocking tasks the state of a task can now be changed by its blocking tasks following the logic: if ANY of the blocking tasks is NOT closed (so its state is in one of the open values) the task is considered as blocked - if a task is blocked and NOT closed its state will switch to Waiting - the Waiting state will display an unclickable hourglass icon on the task kanban/list views, once in the waiting state you can't change the state of the taskfrom the kanban/list views - a blocked task state can be changed through the form view, so you can override the 'block' by choosing a closed state (only done or canceled) - once overriden, the task will change to the closed state the user wants, but the task is still blocked so in case where the user comes back to an open state, the task will automatically switch back to the waiting state (according to the state before the block) - if the blocking task switches to a non-blocking state, the task will not be considered as blocked anymore and its state will switch back to In Progress Default values the default value is always in progress Special cases when a task is moved from a stage to another one - if the state was in one of the open states (approved, changes requested, in progress ) the state goes back to In Progress - if not, the state stays the same when a task is moved from a project to another one - the state goes back to In Progress when a task is duplicated - if the state was in one of the open states (approved, changes requested, in progress ) the state goes back to In Progress - if not, the state stays the same

xavierbol · 2023-03-08T16:04:47Z

robodoo r+

C3POdoo requested review from a team December 9, 2022 09:19

bastvdn changed the title ~~[IMP] project: implement new kanban state~~ [IMP] project: implement new project task state Dec 9, 2022

C3POdoo added the RD research & development, internal work label Dec 9, 2022

bastvdn force-pushed the master-mark-as-done-2 branch 3 times, most recently from cbf7388 to 5469fc4 Compare December 15, 2022 16:43

bastvdn force-pushed the master-mark-as-done-2 branch 2 times, most recently from 32152ac to 9afd4f4 Compare December 26, 2022 14:35

bastvdn force-pushed the master-mark-as-done-2 branch from edddeaa to 943ef5d Compare January 3, 2023 09:17

xavierbol reviewed Jan 10, 2023

View reviewed changes

xavierbol mentioned this pull request Jan 10, 2023

[REF] project,*: refactor projects/tasks views and reports #106015

Closed

bastvdn force-pushed the master-mark-as-done-2 branch from 943ef5d to 37b6542 Compare January 10, 2023 17:18

brboi requested review from a team, FrancoisGe and adr-odoo and removed request for a team January 11, 2023 10:31

bastvdn marked this pull request as draft January 18, 2023 08:26

bastvdn force-pushed the master-mark-as-done-2 branch 7 times, most recently from 2455620 to 30c6f6e Compare January 20, 2023 14:44

adr-odoo removed their request for review January 23, 2023 10:02

bastvdn force-pushed the master-mark-as-done-2 branch from 30c6f6e to 5486664 Compare January 26, 2023 12:38

bastvdn force-pushed the master-mark-as-done-2 branch 5 times, most recently from d5acc1c to b003140 Compare February 17, 2023 16:16

MissingNoShiny reviewed Feb 21, 2023

View reviewed changes

bastvdn force-pushed the master-mark-as-done-2 branch from 4832124 to 2282668 Compare February 22, 2023 09:59

bastvdn force-pushed the master-mark-as-done-2 branch 5 times, most recently from 7001444 to d494ac0 Compare February 23, 2023 09:23

bastvdn force-pushed the master-mark-as-done-2 branch 7 times, most recently from d6c8ae4 to 6f44962 Compare February 27, 2023 17:57

xavierbol reviewed Mar 3, 2023

View reviewed changes

xavierbol reviewed Mar 7, 2023

View reviewed changes

Comment thread addons/project/views/project_views.xml Outdated

xavierbol Mar 7, 2023

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

I guess it is needed. 🤷🏼‍♂️

xavierbol approved these changes Mar 8, 2023

View reviewed changes

bastvdn mentioned this pull request Apr 14, 2023

[IMP] project: improve various mark as done features #117968

Closed

pedrobaeza mentioned this pull request Nov 11, 2024

[17.0][MIG] hr_timesheet_task_domain: Migration to 17.0 OCA/timesheet#721

Merged

	@property
	def SELF_READABLE_FIELDS(self):
	return PROJECT_TASK_READABLE_FIELDS \| self.SELF_WRITABLE_FIELDS

	task.is_closed = True if task.state in CLOSED_STATES else False
	task.is_closed = task.state in CLOSED_STATES

-        for task in self:
-            if task.allow_task_dependencies:
-                for dependent_task in task.depend_on_ids:
-                    # if one of the blocking task is in a blocking state
-                    if dependent_task.state not in CLOSED_STATES:
-                        # here we check that the blocked task is not alread in a closed state (if the task is already done we don't put it in waiting state)
-                        if task.state not in CLOSED_STATES:
-                            task.state = '04_waiting_normal'
-                        return
-                # if the task as no blocking dependencies and is in waiting_normal, the task goes back to in progress
-                if task.state == '04_waiting_normal':
-                    task.state = '01_in_progress'
+        tasks_with_dependencies_feature = self.filtered("allow_dependencies")
+        dependent_tasks = self.env["project.task"].search([
+            ("dependent_ids", 'in', tasks_with_dependencies.ids),
+            ("state", "not in", CLOSED_STATES)
+        ])
+        for task in self:
+            # if one of the blocking task is in a blocking state
+            if dependent_task.state not in CLOSED_STATES:
+                # here we check that the blocked task is not already
+                # in a closed state (if the task is already done we
+                # don't put it in waiting state)
+                if task.state not in CLOSED_STATES and task in dependent_tasks:
+                    task.state = '04_waiting_normal'
+                elif task.state == '04_waiting_normal':
+                    # if the task as no blocking dependencies and is in
+                    # waiting_normal, the task goes back to in progress
+                    task.state = '01_in_progress'

-        <xpath expr="//span[@t-esc='formattedValue']" position="replace">
-            <s t-if="['1_done', '1_canceled'].includes(this.props.record.data.state) and ['kanban', 'list'].includes(this.props.record.activeFields[this.props.name].viewType)" t-esc="formattedValue"/>
-            <span t-else="" t-esc="formattedValue"/>
-        </xpath>
+        <xpath expr="//span[@t-esc='formattedValue']" position="before">
+            <s t-if="['1_done', '1_canceled'].includes(this.props.record.data.state) and ['kanban', 'list'].includes(this.props.record.activeFields[this.props.name].viewType)" t-esc="formattedValue"/>
+        </xpath>
+        <xpath expr="//span[@t-esc='formattedValue']" position="attributes">
+            <attribute name="t-else">""</attribute>
+        </xpath>

Uh oh!

Conversation

bastvdn commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Specs Mark as Done

Overview

Interaction with blocking tasks:

Where to change the state of a task:

Interaction with existing fields

Interaction with blocking tasks

Default values

Special cases

Uh oh!

robodoo commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adr-odoo commented Jan 23, 2023

Uh oh!

MissingNoShiny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bastvdn commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MissingNoShiny commented Feb 23, 2023

Uh oh!

xavierbol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bastvdn Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierbol Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierbol Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierbol Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bastvdn commented Dec 9, 2022 •

edited

Loading

robodoo commented Dec 9, 2022 •

edited

Loading

bastvdn commented Feb 22, 2023 •

edited

Loading

bastvdn Mar 6, 2023 •

edited

Loading

xavierbol Mar 3, 2023 •

edited

Loading

xavierbol Mar 6, 2023 •

edited

Loading

xavierbol Mar 6, 2023 •

edited

Loading

xavierbol Mar 7, 2023 •

edited

Loading