UI-0 - (UKS-5229) - remove else condition from removal key tracker

[ibolari42]

The else condition was causing the widget to be removed to not be included in the migrated content. so when the content got passed later on to the removewidget function it was incomplete leading to an erroneous result

[Nouzbe]

UKS-5229.

Summary of Commerzbank issue:

Context

• the original dashboard page contains 2 widgets:
  • a pivot table
  • a page context values

(if I understand correctly), they run:

npx migrate -i "content of ui.json" -o migrated-content.json -s servers.json --remove-widgets "context-values"

Expectation

Only the context values widget is removed.

Result

Both the context values and the pivot table widgets are removed.

Hi @ibolari42,

Is this understanding summarized above correct?
If it is, could you explain the fix? The else aims at only migrating widgets whose keys are not part of keysOfWidgetPluginsToRemove. In Commerzbank's case, I expect it to remove the context values widget but not the pivot table widget.

[ibolari42]

@Nouzbe , Your summary is entirely correct.

The problem with the else statement is that the actual removal of widgets happens here: https://github.com/activeviam/activeui-migration/blob/57da18ec074107bf48573ddaaaabaf32bd32c374/src/migrateDashboard.ts#L190-L208

The else prematurely excludes the widgets from the construction of a dashbaord object which will feed the producer that runs at line 190.

[Nouzbe]

Thank you for the explanation @ibolari42, it makes sense.

I would like to have a test that would have caught the issue.

After checking, we do have a test (admittedly unnecessarily complicated) testing the removal of widgets. I edited it until I found it straight forward to read.

But it does pass on latest, thus seeming to indicate that the described bug does not exist (although I don't doubt that it does).

Could you update this test so that it would have caught this bug?

[ibolari42]

Happy to update the test, i'll take a look.

[ibolari42]

@Nouzbe , Sorry it took me so long to get to this. My prior descriptions were missing an important piece of the puzzle that hopefully the new test clarifies. The problem that this fix solves only really occurs when all but one widget is removed. The tool has moved on a lot since we last looked at this, so it might not be a problem anymore. Essentially because the widget is prematurely removed by the else it affects the getLayoutpath call in the producer here: https://github.com/activeviam/activeui-migration/blob/57da18ec074107bf48573ddaaaabaf32bd32c374/src/migrateDashboard.ts#L190-L208

[ibolari42]

resolved by #119