Implement a React-based table editor #5186

mistercrunch · 2018-06-13T05:21:43Z

TODO:

test/tune with Druid
test coverage

Adding recent screenshots for design review

mistercrunch · 2018-06-15T23:01:39Z

Note slight difference between Columns and and Calculated Columns:

data type is only editable for calculated metrics
column name only editable for calculated metrics
no way to add a Column other than syncing
SQL Expression only visible for Calculated Column

All of these difference make it feel like it was the right thing to do to split them apart.

codecov-io · 2018-07-24T00:11:48Z

Codecov Report

Merging #5186 into master will increase coverage by 0.22%.
The diff coverage is 76.1%.

@@            Coverage Diff             @@
##           master    #5186      +/-   ##
==========================================
+ Coverage   63.25%   63.48%   +0.22%     
==========================================
  Files         351      358       +7     
  Lines       22258    22741     +483     
  Branches     2470     2530      +60     
==========================================
+ Hits        14079    14436     +357     
- Misses       8164     8290     +126     
  Partials       15       15

Impacted Files	Coverage Δ
superset/data/__init__.py	`100% <ø> (ø)`	⬆️
...explore/components/controls/SelectAsyncControl.jsx	`56% <0%> (-14%)`	⬇️
...rset/assets/src/explore/reducers/exploreReducer.js	`36.17% <0%> (ø)`	⬆️
superset/assets/src/components/TableLoader.jsx	`48.57% <100%> (ø)`
superset/assets/src/explore/controls.jsx	`47.36% <100%> (+0.39%)`	⬆️
...perset/assets/src/profile/components/Favorites.jsx	`100% <100%> (ø)`	⬆️
...rc/explore/components/controls/TextAreaControl.jsx	`97.29% <100%> (+0.07%)`	⬆️
...t/assets/src/profile/components/CreatedContent.jsx	`100% <100%> (ø)`	⬆️
superset/connectors/sqla/models.py	`78.92% <100%> (+0.41%)`	⬆️
.../src/explore/components/ControlPanelsContainer.jsx	`88.15% <100%> (ø)`	⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51bd17d...323edaf. Read the comment docs.

mistercrunch · 2018-07-24T22:52:28Z

superset/assets/src/CRUD/utils.js

+import React from 'react';
+
+export function recurseReactClone(children, type, propExtender) {
+  return React.Children.map(children, (child) => {


docstring needed

betodealmeida · 2018-08-01T04:56:27Z

A few issues from testing:

Can't remove Owner from a datasource.
Can we add validation to "Cache Timeout", checking that it's an int?
BIGINT columns are being added as dimensions after being synced:
When adding a new calculated column there's an initial error:
Add a tooltip to "D3 Format" in the metrics expando.

betodealmeida

This looks fantastic, @mistercrunch! I have a few comments and I found some minor bugs, but it should be good to go with some small fixes.

betodealmeida · 2018-08-01T00:01:49Z

superset/assets/spec/javascripts/datasource/DatasourceEditor_spec.jsx

+const props = {
+  datasource: mockDatasource['7__table'],
+  addSuccessToast: () => {},
+  GaddDangerToast: () => {},


s/GaddDangerToast/addDangerToast/

betodealmeida · 2018-08-01T15:40:37Z

superset/assets/spec/javascripts/CRUD/CollectionTable_spec.jsx

+    const length = mockDatasource['7__table'].columns.length;
+    expect(wrapper.find('table')).to.have.lengthOf(1);
+    expect(wrapper.find('tbody tr.row')).to.have.lengthOf(length);
+    expect(wrapper.find('tbody tr.exp')).to.have.lengthOf(length);


Nice, reminded me of what we were talking yesterday about writing resilient unit tests.

betodealmeida · 2018-08-01T15:59:42Z

superset/assets/src/CRUD/CollectionTable.jsx

+  borderRadius: 5,
+  padding: 10,
+  background: '#F4F4F4',
+};


Question, do you have any guidelines on adding CSS here vs. having a CollectionTable.css file?

Moving it to css, we don't really have specific guidelines but should have some.

betodealmeida · 2018-08-01T16:06:37Z

superset/assets/src/CRUD/Fieldset.jsx

+        {title &&
+          <legend>{title}</legend>
+        }
+        {recurseReactClone(this.props.children, Field, propExtender)}


This is super cool.

betodealmeida · 2018-08-01T16:13:47Z

superset/assets/src/datasource/DatasourceEditor.jsx

+    const noFilterCalcCols = this.state.calculatedColumns.filter(
+      col => !col.expression && !col.json);
+    errors = errors.concat(noFilterCalcCols.map(
+      col => t('Calculated column [%s] requires an expression', col.column_name)));


This error is popping up as soon as a new calculated column is added, giving the impression something wrong happened. Maybe have it default to a dummy expression instead of being empty? Or even better, have it so that when a new calculated column is added the field is expanded and the SQL Expression input has focus?

betodealmeida · 2018-08-01T16:31:52Z

superset/assets/src/datasource/DatasourceModal.jsx

+          <div>
+            <i className="fa fa-exclamation-triangle" />{' '}
+            {t('The data source configuration exposed here ')}
+            <strong>{t('affects all the charts using this datasource. ')} </strong>


Nit: for translation it's way better to keep the phrase together. Unfortunately there doesn't seem to be a good way of doing it here because of the <strong> tag. The alternatives seem to be using (1) Markdown or (2) allowing unsafe HTML in translation strings.

Also nit, remove the space after }.

<strong> ins't important here, removing.

betodealmeida · 2018-08-01T16:40:49Z

superset/connectors/base/models.py

+                orm_kwargs = {}
+                for k in obj:
+                    if (
+                            k in fkmany_class.update_from_object_fields and


Nit: dedent a level

betodealmeida · 2018-08-01T16:41:08Z

superset/connectors/base/models.py

+        """Update datasource from a data structure
+
+        The UI's table editor crafts a complex data structure that
+        contains most of the datsource's properties as well as


datasource*

betodealmeida · 2018-08-01T16:58:01Z

superset/views/base.py

+        if raise_if_false:
+            raise security_exception
+        return False
+    roles = (r.name for r in get_user_roles())


It works here, but it might be better to use a list comprehension here instead of a generator expression. The check in line 299 will exhaust the generator, and it you try to access roles after it will be empty.

>>> a = (i for i in range(10)) >>> 1 in a True >>> 1 in a False >>> list(a) []

oh wow! I thought I was just doing a tuple comprehension.

betodealmeida · 2018-08-01T16:58:22Z

superset/views/base.py

+    if hasattr(orig_obj, 'created_by'):
+        owners += [orig_obj.created_by]
+
+    owner_names = (o.username for o in owners)


mistercrunch · 2018-08-02T03:13:20Z

@betodealmeida thanks for the review, I addressed all you comments and new rows should appear expanded now

betodealmeida

Nice, can't wait to see this live!

williaster

Looks good overall! Had a couple comments on the React.

williaster · 2018-08-06T22:09:31Z

superset/assets/src/CRUD/CollectionTable.jsx

+import './styles.css';
+
+const propTypes = {
+  collection: PropTypes.array,


can you make any of these more specific? generic array and objects are not very useful (but it's not always possible to make them more specific either 🤷‍♀️ )

also all non-required props should have a default, I don't think that's true here.

williaster · 2018-08-06T22:10:41Z

superset/assets/src/CRUD/CollectionTable.jsx

+  getLabel(col) {
+    const { columnLabels } = this.props;
+    let label = columnLabels[col] ? columnLabels[col] : col;
+    if (label.startsWith('__')) {


can you add a comment as to what this check is doing? (is it just the __expanded item?) or create a helper with a name to help do the same?

williaster · 2018-08-06T22:15:50Z

superset/assets/src/CRUD/CollectionTable.jsx

+      tds.push(
+        <td key="__expand" className="expand">
+          <i
+            className={`fa fa-caret-${isExpanded ? 'down' : 'right'} text-primary pointer`}


I think the convention that @elibrumbaugh has been pushing for is to have the arrow point in the direction that the click will cause. so unexpanded = down, expanded = up

mmh, I don't think that's the standard though https://www.google.com/search?q=caret+expand&source=lnms&tbm=isch&sa=X&ved=0ahUKEwiH6_vo7uvcAhXLxlQKHbOGDwwQ_AUICygC&biw=1679&bih=908#imgrc=wwzY-6mSHpat8M:

williaster · 2018-08-06T22:17:17Z

superset/assets/src/CRUD/CollectionTable.jsx

@@ -0,0 +1,220 @@
+import React from 'react';


I think you could simplify this component significantly by separating the table component from the row component (ie different files instead of this.renderHeaderRow(), etc.)

The file is ~200 lines which is still pretty intelligible to me. Though I agree we should break down the larger files. I know I tend to make large modules :'(

williaster · 2018-08-06T22:23:06Z

superset/assets/src/CRUD/CollectionTable.jsx

+        </td>);
+    }
+    const trs = [<tr className="row" key={record.id}>{tds}</tr>];
+    if (isExpanded) {


the cleanest expandable tables I have seen have

separated the row component from the table itself (Table + ExpandableRow) to decouple and to simplify the components

have implemented the expanded row with a trick so that you can encapsulate the expansion in one component. the trick is to wrap every row (previously tr) in a tbody, which can have 1 or more rows. so you can render something like:

// ExpandableRow.jsx ... return ( <tbody> <tr>{/* tds here */}</tr> {this.props.isExpanded && <tr><td colspan={x}>{this.props.expandedContent}</td></tr> </tbody> );

Will these multiple tbody line up, or do I have to specify hard widths?

williaster · 2018-08-06T22:26:34Z

superset/assets/src/components/EditableTitle.jsx

@@ -10,14 +10,18 @@ const propTypes = {
  onSaveTitle: PropTypes.func,
  noPermitTooltip: PropTypes.string,
  showTooltip: PropTypes.bool,
+  emptyText: PropTypes.node,
+  style: PropTypes.object,


all non-required props should have a default, can you add one for style?

williaster · 2018-08-06T22:27:27Z

superset/assets/src/datasource/DatasourceModal.jsx

+import DatasourceEditor from '../datasource/DatasourceEditor';
+import withToasts from '../messageToasts/enhancers/withToasts';
+
+const $ = window.$ = require('jquery');


do you have to set window.$? this is a pattern that appears in many files and I don't think is necessary.

yes I'm not sure what the intent was originally... copy pasta

williaster · 2018-08-06T22:27:37Z

superset/assets/src/datasource/DatasourceModal.jsx

+
+const propTypes = {
+  onChange: PropTypes.func,
+  datasource: PropTypes.object,


all non-required props should have a default

williaster · 2018-08-06T22:28:59Z

superset/assets/src/datasource/DatasourceModal.jsx

+              {t('Save')}
+            </Button>
+            <Button bsSize="sm" onClick={this.props.onHide}>{t('Cancel')}</Button>
+            <Dialog ref={(el) => { this.dialog = el; }} />


can you create a setDialogRef function for this?

williaster · 2018-08-06T22:29:32Z

superset/assets/src/explore/components/controls/CheckboxControl.jsx

@@ -4,7 +4,7 @@ import ControlHeader from '../ControlHeader';
 import Checkbox from '../../../components/Checkbox';

 const propTypes = {
-  name: PropTypes.string.isRequired,
+  name: PropTypes.string,


can you add a default prop if you aren't requiring this anymore?

williaster · 2018-08-06T22:32:18Z

nooooo just missed it 😭 code quality around props is important.

mistercrunch · 2018-08-07T17:56:08Z

@williaster thanks for the review. Ill take the time to submit another PR to address your comments.

williaster · 2018-08-08T07:09:16Z

thanks @mistercrunch !

john-bodley · 2018-11-05T18:58:46Z

@mistercrunch it seems that this was never implemented for Druid datasources per this.

* A React table editor * addressing comments * Fix SelectAsyncControl error on clear * fix tests * more corrections * Removed <strong>

michellethomas · 2019-02-15T02:14:33Z

superset/views/datasource.py

+                __(
+                    'You are not authorized to modify '
+                    'this data source configuration'),
+                status='401',


hey @mistercrunch in the new datasource editor you need to be an owner to edit a datasource, previously anyone could edit datasources (this is still the case in the crud view). Allowing anyone to edit is useful because many people may want to add columns or metrics to a datasource. Do you think it's necessary to only have owners edit a datasource?

I can change this to make it configurable, but just wanted some more info from you about this change.

Hmmm, how about creating a new permission can_edit_any_datasource ?

That sounds good, I'll take a look.

john-bodley · 2019-04-22T18:43:30Z

superset/connectors/druid/models.py

@@ -520,6 +528,9 @@ def get_perm(self):
            '[{obj.cluster_name}].[{obj.datasource_name}]'
            '(id:{obj.id})').format(obj=self)

+    def update_from_object(self, obj):


@mistercrunch is there any reason why this wasn't/hasn't been implemented for Druid datasources? The UX is fairly poor if people try to update the metadata fro a Druid datasource which seems to be successfully saved yet the datasource hasn't been updated.

mistercrunch force-pushed the table_editor branch 2 times, most recently from d5cdcb0 to 512ba11 Compare June 15, 2018 17:57

mistercrunch force-pushed the table_editor branch from 512ba11 to 0ebc642 Compare July 5, 2018 15:38

mistercrunch force-pushed the table_editor branch 4 times, most recently from 6d87773 to c7b4e99 Compare July 21, 2018 01:11

mistercrunch changed the title ~~[WiP] a React-based table editor~~ Implement a React-based table editor Jul 21, 2018

mistercrunch force-pushed the table_editor branch 7 times, most recently from 4a776ca to cea649a Compare July 23, 2018 22:20

mistercrunch commented Jul 24, 2018

View reviewed changes

betodealmeida reviewed Aug 1, 2018

View reviewed changes

mistercrunch force-pushed the table_editor branch from 36a673a to 546830f Compare August 2, 2018 01:19

betodealmeida approved these changes Aug 3, 2018

View reviewed changes

mistercrunch added 6 commits August 3, 2018 12:52

A React table editor

697f400

addressing comments

6bbba20

Fix SelectAsyncControl error on clear

2b01caa

fix tests

3557024

more corrections

a9da246

Removed <strong>

323edaf

mistercrunch force-pushed the table_editor branch from 15747b0 to 323edaf Compare August 3, 2018 19:52

mistercrunch merged commit 68ba63f into apache:master Aug 6, 2018

williaster reviewed Aug 6, 2018

View reviewed changes

mistercrunch mentioned this pull request Aug 14, 2018

Tackling late-arriving comments from #5186 #5626

Merged

mistercrunch added a commit to mistercrunch/superset that referenced this pull request Aug 14, 2018

Tackling late-arriving comments from apache#5186

2d10bf0

kristw mentioned this pull request Aug 14, 2018

Update CheckboxControl from apache/master airbnb/superset-fork#85

Merged

mistercrunch added a commit that referenced this pull request Aug 14, 2018

Tackling late-arriving comments from #5186 (#5626)

d601ff4

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Tackling late-arriving comments from apache#5186 (apache#5626)

8d37ccd

michellethomas reviewed Feb 15, 2019

View reviewed changes

john-bodley reviewed Apr 22, 2019

View reviewed changes

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Implement a React-based table editor #5186

Implement a React-based table editor #5186

Conversation

mistercrunch commented Jun 13, 2018 • edited Loading

mistercrunch commented Jun 15, 2018

codecov-io commented Jul 24, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

betodealmeida commented Aug 1, 2018

betodealmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Aug 2, 2018 • edited Loading

betodealmeida left a comment

Choose a reason for hiding this comment

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster commented Aug 6, 2018

mistercrunch commented Aug 7, 2018

williaster commented Aug 8, 2018

john-bodley commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Jun 13, 2018 •

edited

Loading

codecov-io commented Jul 24, 2018 •

edited

Loading

mistercrunch Aug 2, 2018 •

edited

Loading

mistercrunch commented Aug 2, 2018 •

edited

Loading