Page MenuHomePhabricator

Number of columns for X and Y values for a graph keep increasing every time I open the dialog for empty <graph> node
Open, LowPublic1 Story Points

Description

Steps to reproduce:

  1. Open VE
  2. Go to Insert>Graph
  3. Go to Data section
  4. Delete all the values for x,y
  5. Click on "Insert"
  6. When an empty graph appears click on Edit again to open the dialog

Now observe that in the Data section there are now two extra columns for x and y. If you close and reopen again, there will be two more....

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 21 2016, 10:40 PM
Ryasmeen renamed this task from Number of columns for X and Y values for a graph keeps increasing every time I open the dialog for empty <graph> node to Number of columns for X and Y values for a graph keep increasing every time I open the dialog for empty <graph> node.Nov 21 2016, 10:40 PM
Jdforrester-WMF triaged this task as High priority.Nov 22 2016, 8:10 PM
Jdforrester-WMF moved this task from Backlog to VisualEditor on the Graphs board.
Jdforrester-WMF set the point value for this task to 1.
Jdforrester-WMF moved this task from To Triage to TR0: Interrupt on the VisualEditor board.
Jdforrester-WMF added a subscriber: ferdbold.

Nice catch, I'll look into this ASaP

Is this still broken?

Deskana lowered the priority of this task from High to Low.Aug 24 2017, 10:37 AM
Deskana moved this task from TR0: Interrupt to Freezer on the VisualEditor board.

Change 437233 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] [Graph] Add non voting QUnit job

https://gerrit.wikimedia.org/r/437233

Change 437233 merged by jenkins-bot:
[integration/config@master] [Graph] Add non voting QUnit job

https://gerrit.wikimedia.org/r/437233

hashar added a subscriber: hashar.Jun 4 2018, 1:20 PM

I tried to add the QUnit tests in CI, the ve.ui.TableWidget tests fail when inserting a column. The first is one can not add an empty column, the test suite has:

modules/ve-graph/tests/ext.graph.visualEditor.test.js
592   widgetA.insertColumn();

Which tries to normalize the insertion of data to an empty array but the code fails later because data is not initialized:

modules/ve-graph/widgets/ve.dm.TableWidgetModel.js
ve.dm.TableWidgetModel.prototype.insertColumn = function ( data, index, key, label ) {
    ...
    // normalized to an empty array
    insertData = ( Array.isArray( data ) ) ? data : [];  
    ...
    // original 'data' is passed which is 'undefined'
    this.emit( 'insertColumn', data, index, key, label );

I changed the test to use:

widgetA.insertColumn( [ 'foo', 'bar' ] );

The assertion Row and column are added successfully then fails and show the column has been inserted twice.

From the git log, @ferdbold implemented the TableWidget but it is not entirely functional yet.

hashar added a comment.Jun 4 2018, 1:33 PM

CI now runs the QUnit tests, though the job is non voting. An example run https://integration.wikimedia.org/ci/job/mwext-qunit-jessie-non-voting/1/console

  ext.graph.visualEditor
    ✔ ve.dm.MWGraphNode
    ✔ ve.ce.MWGraphNode
    ✖ ve.ce.MWGraphNode.static
    ✔ ve.dm.MWGraphModel
    ✔ ve.dm.MWGraphModel.static
    ✖ ve.ui.TableWidget


SUMMARY:
✔ 732 tests completed
✖ 2 tests failed

FAILED TESTS:
  ext.graph.visualEditor
    ✖ ve.ce.MWGraphNode.static
      Chrome 57.0.2987 (Linux 0.0.0)
    Test took longer than 60000ms; test timed out.
        at node_modules/qunit/qunit/qunit.js:2053:7
    

    ✖ ve.ui.TableWidget
      Chrome 57.0.2987 (Linux 0.0.0)
    TypeError: Cannot read property '0' of undefined
        at VeUiTableWidget.ve.ui.TableWidget.onInsertColumn 
        at VeDmTableWidgetModel.oo.EventEmitter.emit 
        at VeDmTableWidgetModel.ve.dm.TableWidgetModel.insertColumn
        at VeUiTableWidget.ve.ui.TableWidget.insertColumn

Change 442145 had a related patch set uploaded (by Hashar; owner: Hashar):
[mediawiki/extensions/Graph@master] VE table widget is still a work in progress

https://gerrit.wikimedia.org/r/442145

Change 442145 merged by jenkins-bot:
[mediawiki/extensions/Graph@master] VE table widget is still a work in progress

https://gerrit.wikimedia.org/r/442145

The QUnit test ve.ui.TableWidget is now skipped until the feature is completed.

The ve.ce.MWGraphNode.static test is unrelated and is being fixed via T198229