reactabular / treetabular

Tree utilities (MIT)
MIT License
11 stars 6 forks source link

tree.fixOrder does not work for deep hierarchical data #13

Closed vijayst closed 7 years ago

vijayst commented 7 years ago

tree.fixOrder does not work for hierarchical data (more than 2 levels deep).

For (id, parent) combinations below.

100, null
110, 100
120, 100
111, 110
112, 110

following is the arrangement after tree.fixOrder

111, 110
112, 110
100, null
110, 100
120, 100
bebraw commented 7 years ago

This might need some extra logic. PR welcome. 👍

vijayst commented 7 years ago

@bebraw Sure, I will work on the PR sometime today.

vijayst commented 7 years ago

I tried the below test in treetabular - fix-order-test and it works as expected. I need to investigate a bit more why I am getting unexpected results with fixOrder in my app. I am closing the issue.

it('works with nested children 2', () => {
    const rowsWithNestedChildren = [
      {
        id: '111',
        parent: '110'
      },
      {
        id: '112',
        parent: '110'
      },
      {
        id: '100',
        parent: null
      },
      {
        id: '110',
        parent: '100'
      },
      {
        id: '120',
        parent: '100'
      }
    ];

    const expectedOrder = [
      {
        id: '100',
        parent: null
      },
      {
        id: '110',
        parent: '100'
      },
      {
        id: '111',
        parent: '110'
      },
      {
        id: '112',
        parent: '110'
      },
      {
        id: '120',
        parent: '100'
      }
    ];

    const actualOrder = fixOrder()(rowsWithNestedChildren);
    expect(actualOrder).toEqual(expectedOrder);
  });
vijayst commented 7 years ago

@bebraw There is some error with treetabular with sample data that I have. The sample data is quite long - about 35 entries. treetabular fixOrder was giving wrong results. I resolved the problem by writing fixOrder in my app code. However, I am not able to isolate what is wrong.

bebraw commented 7 years ago

Ok. Maybe the logic is missing some handling. Feel free to PR if you can isolate it somehow. :)

vijayst commented 7 years ago

It probably requires your intervention as the bug is subtle. I wrote a recursive function to fix it. When we iterate the grouped children, the parent row is not found in the rows already added but in the grouped children itself. So, I had to recursively add parents till all parents are added to the rows. Probably, not the best way to go about it.

bebraw commented 7 years ago

Can you anonymize the initial data just to make it fail? That would be a start.

vijayst commented 7 years ago

Below is the sample data that I have. D, C, H, V represents a hierarchy. For V321, the parent is H318. For H318, parent is C9. For C9, parent is D15. This gives a wrong output with fix-order-test.

[
    {
        "id":"D0_C0_H0_V321",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D0_C0_H0_V335",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D14_C0_H0_V0",
        "parent":null
    },
    {
        "id":"D0_C0_H0_V333",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D0_C0_H0_V336",
        "parent":"D0_C0_H317_V0"
    },
    {
        "id":"D0_C0_H0_V319",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D0_C0_H317_V0",
        "parent":"D0_C9_H0_V0"
    },
    {
        "id":"D0_C0_H0_V329",
        "parent":"D0_C0_H317_V0"
    },
    {
        "id":"D0_C0_H0_V324",
        "parent":"D0_C0_H317_V0"
    },
    {
        "id":"D0_C0_H0_V320",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D0_C0_H318_V0",
        "parent":"D0_C9_H0_V0"
    },
    {
        "id":"D0_C0_H0_V327",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D0_C0_H0_V322",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D0_C0_H0_V326",
        "parent":"D0_C0_H317_V0"
    },
    {
        "id":"D0_C0_H0_V332",
        "parent":"D0_C0_H317_V0"
    },
    {
        "id":"D0_C0_H0_V323",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D15_C0_H0_V0",
        "parent":null
    },
    {
        "id":"D0_C9_H0_V0",
        "parent":"D15_C0_H0_V0"
    },
    {
        "id":"D16_C0_H0_V0",
        "parent":null
    },
    {
        "id":"D0_C0_H0_V331",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D0_C0_H0_V325",
        "parent":"D0_C0_H317_V0"
    },
    {
        "id":"D0_C0_H0_V334",
        "parent":"D0_C0_H317_V0"
    },
    {
        "id":"D0_C0_H0_V330",
        "parent":"D0_C0_H318_V0"
    },
    {
        "id":"D0_C0_H0_V328",
        "parent":"D0_C0_H317_V0"
    }
]
bebraw commented 7 years ago

Ok. I am going to be a bit tight on time next week (book to release etc.) but maybe after that.

You could try to map the data to something clearer (AAA, BBB, etc.). Perhaps that would reveal some pattern. Reducing the example after that would help too. I guess you need only a few nodes to show it.

I'm certain it's some corner case (nesting?) that the current tests are missing.

pzmudzinski commented 7 years ago

I am facing similar issue, I think issue is here:

      {
        id: '100',
        parent: null
      },
      {
        id: '110',
        parent: '100'
      },
      {
        id: '111',
        parent: '110'
      },
      {
        id: '112',
        parent: '110'
      },
      {
        id: '120',
        parent: '100'
      }
    ];

Last item has parent set to 100, although it doesn't match this condition:

If there's a parent relation, the children must follow their parent right after it

The whole condition to match "right after it" causes a lot of troubles with nested children. What if you have structure like that:

B1, B2 are children of B, and B is children of A. B and C has to be "right after" A, but it's impossible because C cannot go before B1 and B2 as it will break B - B1,B2 relation.

@bebraw any thoughts here?

Here's so sample data from me:

[{
        id: 'Folder/3',
        parent: null,
        showingChildren: true,
        parents: []
    },
    {
        id: 'Folder/9',
        parent: 'Folder/3',
        showingChildren: true,
        parents: ['Folder/3']
    },
    {
        id: 'File/28',
        parent: 'Folder/9',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9'
        ]
    },
    {
        id: 'File/29',
        parent: 'Folder/9',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9'
        ]
    },
    {
        id: 'File/30',
        parent: 'Folder/9',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9'
        ]
    },
    {
        id: 'File/31',
        parent: 'Folder/9',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9'
        ]
    },
    {
        id: 'File/32',
        parent: 'Folder/9',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9'
        ]
    },
    {
        id: 'File/11',
        parent: 'Folder/3',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9',
            'File/32'
        ]
    },
    {
        id: 'File/12',
        parent: 'Folder/3',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9',
            'File/32'
        ]
    },
    {
        id: 'File/13',
        parent: 'Folder/3',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9',
            'File/32'
        ]
    },
    {
        id: 'File/14',
        parent: 'Folder/3',
        showingChildren: undefined,
        parents: ['Folder/3',
            'Folder/9',
            'File/32'
        ]
    },
    {
        id: 'File/21',
        parent: null,
        showingChildren: undefined,
        parents: []
    }
]

where each item is computed like that:

this.state.rows.map( (r, index) => {
        return {
          id: r.id,
          parent: r.parent,
          showingChildren: r.showingChildren,
          parents: tree.getParents({ index, idField: 'id', parentField: 'parent' })(this.state.rows).map( (p) => p.id ),
        };
      })

As you see things start breaking with id: 'File/11' where it thinks it has 3 parents, while actually there's only one - Folder/3which is first element of array.

pzmudzinski commented 7 years ago

Ooops. Sorry. I haven't noticed >3.0 update :-)

vijayst commented 7 years ago

I have reduced this to a test:

it('works with deep nesting', () => {
    const rows = [
      {
        "id": "D1",
        "parent": "C2"
      },
      {
        "id": "A1",
        "parent": null
      },
      {
        "id": "D2",
        "parent": "C2"
      },
      {
        "id": "D3",
        "parent": "C1"
      },
      {
        "id": "D4",
        "parent": "C2"
      },
      {
        "id": "C1",
        "parent": "B1"
      },
      {
        "id": "D5",
        "parent": "C1"
      },
      {
        "id": "D6",
        "parent": "C2"
      },
      {
        "id": "C2",
        "parent": "B1"
      },
      {
        "id": "B1",
        "parent": "A1"
      }
    ];

    const expectedRows = [
      {
        "id": "A1",
        "parent": null
      },
      {
        "id": "B1",
        "parent": "A1"
      },
      {
        "id": "C1",
        "parent": "B1"
      },
      {
        "id": "D3",
        "parent": "C1"
      },
      {
        "id": "D5",
        "parent": "C1"
      },
      {
        "id": "C2",
        "parent": "B1"
      },
      {
        "id": "D1",
        "parent": "C2"
      },
      {
        "id": "D2",
        "parent": "C2"
      },
      {
        "id": "D4",
        "parent": "C2"
      },
      {
        "id": "D6",
        "parent": "C2"
      }
    ]

    expect(fixOrder()(rows)).toEqual(expectedRows);
  });
vijayst commented 7 years ago

@bebraw Fixed in PR #20