search

LibrePlan / libreplan

LibrePlan - Open Web Planning
https://www.libreplan.dev
GNU Affero General Public License v3.0
287 stars 170 forks source link

Adding dependency to a container with child with START_ON_FIXED day move the container to a wrong place #1464

Closed kwoot closed 6 years ago

kwoot commented 12 years ago

(Original Bugzilla Bug ID: 1320)

Date: 2011-12-30 10:57:59 From: Manuel Rego Casasnovas \<rego@igalia.com> To: Jacobo Aragunde Perez \<jaragunde@igalia.com> Version: libreplan-1.2 (1.2.x) Last updated: 2012-10-09 07:54:04

(Note: this issue was migrated automatically with bugzilla2github.py tool )

kwoot commented 6 years ago

Bugzilla Comment ID: 3564 Date: 2011-12-30 10:57:59 From: Manuel Rego Casasnovas \<rego@igalia.com>

Steps to reproduce: 1) Create a project with the following structure:

Result: The container is moved one week, but "Task B.1" is still on first day.

Expected result: Container is not moved, and dependency added is marked as violated.

This issue comes from bug #1286 comments.

kwoot commented 6 years ago

Bugzilla Comment ID: 3594 Date: 2012-01-05 12:42:44 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

(In reply to comment #0)

... Expected result: Container is not moved, and dependency added is marked as violated.

Should there be a visual mark specifying that the dependency is violated? I've created one violated dependency using two leaf tasks but the arrow doesn't have a special mark, you only know there's something strange because the task is not moved.

kwoot commented 6 years ago

Bugzilla Comment ID: 3595 Date: 2012-01-05 13:06:24 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

(In reply to comment #1)

(In reply to comment #0)

... Expected result: Container is not moved, and dependency added is marked as violated.

Should there be a visual mark specifying that the dependency is violated?

I see, the end of the arrow should be red. For some reason it didn't happen to me in one case. I'll try to reproduce it and open a different bug.

kwoot commented 6 years ago

Bugzilla Comment ID: 3600 Date: 2012-01-09 07:36:48 From: Manuel Rego Casasnovas \<rego@igalia.com>

(In reply to comment #2)

(In reply to comment #1)

(In reply to comment #0)

... Expected result: Container is not moved, and dependency added is marked as violated.

Should there be a visual mark specifying that the dependency is violated?

I see, the end of the arrow should be red. For some reason it didn't happen to me in one case. I'll try to reproduce it and open a different bug.

Yes, I think the whole arrow should be red, if I remember properly. Ask Loren how will know more about this.

kwoot commented 6 years ago

Bugzilla Comment ID: 3611 Date: 2012-01-11 13:20:43 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

I've been following the flow of the execution and I've arrived to the method calculatePrimaryPointDate(PositionRestrictions) inside GanttDiagramGraph. When this method is called for "Task B.1", it returns the correct date, but when called for its parent ("Task B") it doesn't take into account the START_ON_FIXED_DATE restriction and returns a wrong date.

kwoot commented 6 years ago

Bugzilla Comment ID: 3645 Date: 2012-01-16 13:17:09 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

Digging deeper into the code, I reached GanttDiagramGraph$Recalculation$Dominating.getDependencyConstraintsFor(Point). Here, the method getDependenciesAffectingThisTask() is called. In the case of tasks B.1 and B.2 the dependency from A to B is not returned, therefore a constraint for this dependency is not created, and it won't be taken into account when calculating the new date.

I think I should modify getDependenciesAffectingThisTask() to return the dependencies from parent tasks too. I think it makes sense because any dependency on a container affects its children and it should be taken into account.

kwoot commented 6 years ago

Bugzilla Comment ID: 3652 Date: 2012-01-17 11:01:02 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

(In reply to comment #5)

I think I should modify getDependenciesAffectingThisTask() to return the dependencies from parent tasks too. I think it makes sense because any dependency on a container affects its children and it should be taken into account.

getDependenciesAffectingThisTask() only returns the incoming edges of the task (DirectedGraph.incomingEdgesOf), to do what I want I modified FunctionalityExposedByExtensions.add(...) to create these dependencies in the moment the dependency with the parent is created. Unfortunately that didn't work either. The result can be seen in the attachment. The dependencies from A to B.1 and B.2 are evaluated independently from the dependency from A to B. The result is wrong.

Attached file: gantt-example-container.png File description: Creation of dependencies between one task and all the children

kwoot commented 6 years ago

Bugzilla Comment ID: 3671 Date: 2012-01-23 16:08:36 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

I've just pushed my patch to master:

commit fc4c4ee6c6d15028ea1f898730948819f2dee755 Author: Jacobo Aragunde Pérez jaragunde@igalia.com Date: Mon Jan 23 13:55:45 2012 +0100

Bug #1320: When asking a container for start constraints, return the leftmost
of children's start-in-fixed-date constraints.

Doing it, the dependencies of containers will be ensured to comply with the
most restrictive start dependency of its children.

FEA: ItEr76S04BugFixing

We have to keep it "under surveillance", to see if there's is an impact in performance or if it causes new bugs.

kwoot commented 6 years ago

Bugzilla Comment ID: 3653 Date: 2012-01-17 11:14:46 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

I've been digging a bit deeper and I realized that the root of this bug is in the very core of our model. More exactly, in the way we implement hierarchy relations: a parent task has a start-start dependency (invisible) with all its children, and every children has a end-end dependency with the parent. This is the way the model resizes the container when the children tasks are moved, resized, etc.

So, when we create a end-start dependency between one task (A) and a container (B), we have a situation like this one:

A ---> B ---> B.1 (e-s) (s-s)

When we add a start-on-fixed-day constraint to B.1, the violated dependency is the s-s dependency from B to B.1, not the dependency between A and B. In the case of containers we don't realize because invisible dependencies are involved, but I have done a reproduction with normal dependencies, the result can be seen in the attached screenshot.

To sum up, the ideas I have had so far don't work. Now I'm thinking that, instead of transfer the dependencies from parent to children, I should transfer up the constraints of the children to their parent. I think that could solve the problem although it would need more complex logic.

Attached file: gantt-example.png File description: Visible representation of dependencies between a simple task, a parent and a child

kwoot commented 6 years ago

Bugzilla Comment ID: 3655 Date: 2012-01-17 16:54:20 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

(In reply to comment #7)

... To sum up, the ideas I have had so far don't work. Now I'm thinking that, instead of transfer the dependencies from parent to children, I should transfer up the constraints of the children to their parent. I think that could solve the problem although it would need more complex logic.

It seems that this approach is doing well. Now I have to test it with different types of constraints and in different scenarios.

kwoot commented 6 years ago

Bugzilla Comment ID: 3678 Date: 2012-01-23 19:01:45 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

As a consequence of the fix, a new bug in the schedule system is exposed. When a task is rescheduled, only the tasks related to it are rescheduled. But now it could happen that a sibling task gets affected, but it remains unchanged.

See the image: in the diagram above, there's a violated dependency due to the start-in-fixed-date constraint on task B.2.2. When we remove that constraint, B.2.2 and B.2 are rescheduled to their correct places but B.2.1 remains unchanged, placed in an incorrect position.

Attached file: reschedule-error-with-sibiling.png File description: Graphical description of the error

kwoot commented 6 years ago

Bugzilla Comment ID: 3909 Date: 2012-04-17 07:28:00 From: Manuel Rego Casasnovas \<rego@igalia.com>

(In reply to comment #9)

I've just pushed my patch to master:

commit fc4c4ee6c6d15028ea1f898730948819f2dee755 Author: Jacobo Aragunde Pérez jaragunde@igalia.com Date: Mon Jan 23 13:55:45 2012 +0100

Bug #1320: When asking a container for start constraints, return the

leftmost of children's start-in-fixed-date constraints.

Doing it, the dependencies of containers will be ensured to comply with the
most restrictive start dependency of its children.

FEA: ItEr76S04BugFixing

We have to keep it "under surveillance", to see if there's is an impact in performance or if it causes new bugs.

I'm going to revert this patch for 2 reasons: 1) It's causing a bug #1410 2) It only fixes the issue in forward planning, if you use backwards planning the issue is still present

kwoot commented 6 years ago

Bugzilla Comment ID: 4295 Date: 2012-09-20 16:17:35 From: Jacobo Aragunde Perez \<jaragunde@igalia.com>

Rego has retaken the work on this bug. I have tested his patches and found a situation that we have to test carefully:

1) Create a project with the following structure:

kwoot commented 6 years ago

Bugzilla Comment ID: 4296 Date: 2012-09-21 16:33:26 From: Manuel Rego Casasnovas \<rego@igalia.com>

(In reply to comment #12)

Rego has retaken the work on this bug. I have tested his patches and found a situation that we have to test carefully:

1) Create a project with the following structure:

  • Task A - 40h
  • Task B (container)
    • Task B.1 - 40h
    • Task B.2 - 40h 2) Add dependency from "Task A" to "Task B". 3) Set "Task B.1" as START_ON_FIXED_DATE in the first day of the project 4) Drag "Task B.1" and drop it to a different date

This problem was because of the patches were only fixing the issue when you uses the task properties pop-up. However, when you drag and drop a task, the siblings were not being recalculated.

kwoot commented 6 years ago

Bugzilla Comment ID: 4297 Date: 2012-09-21 16:33:35 From: Manuel Rego Casasnovas \<rego@igalia.com>

commit 125146ebd23493b4fb69f222d44b928c6189b8ef Author: Manuel Rego Casasnovas rego@igalia.com Date: Fri Sep 21 18:30:23 2012 +0200

Bug #1320: Recalculate position of siblings when moving a task

When a task is moved in the Gantt, the constraint changes and it could causes
that some of its siblings should be moved, because of the parent element is
moved too.

FEA: ItEr77S04BugFixing

commit 1bde8c76be6c79d2e74eeb2dcd112922a1fef6fa Author: Manuel Rego Casasnovas rego@igalia.com Date: Wed Sep 19 17:37:32 2012 +0200

Bug #1320: Recalculate position of siblings closing task properties pop-up

It is needed if a constraint is removed in one of the siblings and because of
that the parent element is moved.

FEA: ItEr77S04BugFixing

commit 8a98bf466623cf37da782b898ce91ecb6cc30ef0 Author: Manuel Rego Casasnovas rego@igalia.com Date: Wed Sep 19 16:30:29 2012 +0200

Bug #1320: Fix issue changing methods to get constraints for a task

When the start or end constraints for a task are requested, if it's a container,
it'll return the constraints of the children elements too. Except for the case
of the root task (the project) in order to avoid regression described in bug

FEA: ItEr77S04BugFixing
kwoot commented 6 years ago

Bugzilla Comment ID: 4313 Date: 2012-10-09 07:54:04 From: Manuel Rego Casasnovas \<rego@igalia.com>

Patches were reverted because of they were causing bug #1540.

kwoot commented 6 years ago

commit 04bdd1937bec071f827b98886d22a9c2d84d617e commit 4757dc30c069ba842740ec65a4d303e125dbf5b4 commit 82e18fb77de700d29b5ab99f3e5f79f8b8405c1a commit 125146ebd23493b4fb69f222d44b928c6189b8ef commit 1bde8c76be6c79d2e74eeb2dcd112922a1fef6fa commit 8a98bf466623cf37da782b898ce91ecb6cc30ef0 commit bd0406aa7b88feea9aa2936ef39b3427a3c77779 commit 1ec2ab997801ca72749ad3edf284b7de7a7d682c commit ef057267b968256c3d6a6300a0572e3b303aae54 commit fc4c4ee6c6d15028ea1f898730948819f2dee755