[GitHub] xrmx commented on issue #4780: [i18n] Add French translation

[GitBox]

xrmx commented on issue #4780: [i18n] Add French translation
URL: 
https://github.com/apache/incubator-superset/pull/4780#issuecomment-380360213
 
 
   @emirot merci! :)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] myarnav commented on issue #4612: In the “SQL Lab”, the “Results” section shows “Pending” constantly

[GitBox]

myarnav commented on issue #4612: In the “SQL Lab”, the “Results” section shows 
“Pending” constantly
URL: 
https://github.com/apache/incubator-superset/issues/4612#issuecomment-380419500
 
 
   Hi, I have setup async in SQL Lab, and setup celery with redis. My 
superset_config.py file is on python path and included below. 
   
   > ROW_LIMIT = 5000
   > SUPERSET_WORKERS = 4
   > SUPERSET_WEBSERVER_PORT = 8000
   > SECRET_KEY = '\2\mthisismyscretkey\1\2\a\b\y\h'
   > SQLALCHEMY_DATABASE_URI = 'sqlite:home/ubuntu/.superset/superset.db'
   > CSRF_ENABLED = True
   > MAPBOX_API_KEY = ''
   > CACHE_DEFAULT_TIMEOUT = 86400
   > CACHE_CONFIG = {
   > 'CACHE_TYPE': 'RedisCache',
   > 'CACHE_DEFAULT_TIMEOUT': 86400,
   > 'CACHE_KEYPREFIX': 'superset',
   > 'CACHE_REDIS_HOST': 'localhost',
   > 'CACHE_REDIS_PORT': 6379,
   > 'CACHE_REDIS_DB': 1,
   > 'CACHE_REDIS_URL': 'redis://localhost:6379/1'
   
   As suggested in installation instructions, I have setup celery workers and 
running "superset worker & to start superset before I run "superset runserver 
&". When I start the superset workers. I am seeing the following error message :
   
   > --- ** * -- Linux-4.4.0-1052-aws-x86_64-with-Ubuntu-16.04-xenial 
2018-04-11 10:31:55
   > 
   > ** -- [config]
   > ** -- .> app: main:0x7f1652b31c50
   > -- .> transport: amqp://guest:@localhost:5672//
   > ** -- .> results: disabled://
   > --- --- .> concurrency: 32 (prefork)
   > -- *  .> task events: OFF (enable -E to monitor tasks in this worker)
   > -- [queues]
   > .> celery exchange=celery(direct) key=celery
   > 
   > [2018-04-11 10:31:58,576: ERROR/MainProcess] consumer: Cannot connect to 
amqp://guest:@127.0.0.1:5672//: [Errno 111] Connection refused.
   > Trying again in 32.00 seconds... Cannot connect to 
amqp://guest:@127.0.0.1:5672//: [Errno 111] Connection refused.
   > 
   
   I would deeply appreciate if some one is able to help. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] myarnav commented on issue #4612: In the “SQL Lab”, the “Results” section shows “Pending” constantly

[GitBox]

myarnav commented on issue #4612: In the “SQL Lab”, the “Results” section shows 
“Pending” constantly
URL: 
https://github.com/apache/incubator-superset/issues/4612#issuecomment-380419500
 
 
   Hi, I have setup async in SQL Lab, and setup celery with redis. My 
superset_config.py file is on python path and included below. 
   `superset specific config
   -
   ROW_LIMIT = 5000
   SUPERSET_WORKERS = 4
   
   You will be serving site on port 8000 from gunicorn which sits in front of 
flask, and then send to nginx
   SUPERSET_WEBSERVER_PORT = 8000
   
   -
   -
   Flask App Builder configuration
   -
   Your App secret key
   SECRET_KEY = '\2\mthisismyscretkey\1\2\a\b\y\h'
   
   The SQLAlchemy connection string to your database backend
   This connection defines the path to the database that stores your
   superset metadata (slices, connections, tables, dashboards, ...).
   Note that the connection information to connect to the datasources
   you want to explore are managed directly in the web UI
   SQLALCHEMY_DATABASE_URI = 'sqlite:home/ubuntu/.superset/superset.db'
   
   Flask-WTF flag for CSRF
   CSRF_ENABLED = True
   
   Set this API key to enable Mapbox visualizations
   MAPBOX_API_KEY = ''
   
   CACHE_DEFAULT_TIMEOUT = 86400
   CACHE_CONFIG = {
   'CACHE_TYPE': 'RedisCache',
   'CACHE_DEFAULT_TIMEOUT': 86400,
   'CACHE_KEYPREFIX': 'superset',
   'CACHE_REDIS_HOST': 'localhost',
   'CACHE_REDIS_PORT': 6379,
   'CACHE_REDIS_DB': 1,
   'CACHE_REDIS_URL': 'redis://localhost:6379/1'
   }`
   
   
   As suggested in installation instructions, I have setup celery workers and 
running "superset worker & to start superset before I run "superset runserver 
&". When I start the superset workers. I am seeing the following error message :
   
   `--- ** * -- Linux-4.4.0-1052-aws-x86_64-with-Ubuntu-16.04-xenial 2018-04-11 
10:31:55
   
   ** -- [config]
   ** -- .> app: main:0x7f1652b31c50
   -- .> transport: amqp://guest:@localhost:5672//
   ** -- .> results: disabled://
   --- --- .> concurrency: 32 (prefork)
   -- *  .> task events: OFF (enable -E to monitor tasks in this worker)
   -- [queues]
   .> celery exchange=celery(direct) key=celery
   
   [2018-04-11 10:31:58,576: ERROR/MainProcess] consumer: Cannot connect to 
amqp://guest:@127.0.0.1:5672//: [Errno 111] Connection refused.
   Trying again in 32.00 seconds... Cannot connect to 
amqp://guest:@127.0.0.1:5672//: [Errno 111] Connection refused.
   
   `


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] myarnav commented on issue #4612: In the “SQL Lab”, the “Results” section shows “Pending” constantly

[GitBox]

myarnav commented on issue #4612: In the “SQL Lab”, the “Results” section shows 
“Pending” constantly
URL: 
https://github.com/apache/incubator-superset/issues/4612#issuecomment-380419500
 
 
   Hi, I have setup async in SQL Lab, and setup celery with redis. My 
superset_config.py file is on python path and included below. 
   
   > ROW_LIMIT = 5000
   > SUPERSET_WORKERS = 4
   > SUPERSET_WEBSERVER_PORT = 8000
   > SECRET_KEY = '\2\mthisismyscretkey\1\2\a\b\y\h'
   > SQLALCHEMY_DATABASE_URI = 'sqlite:home/ubuntu/.superset/superset.db'
   > CSRF_ENABLED = True
   > MAPBOX_API_KEY = ''
   > CACHE_DEFAULT_TIMEOUT = 86400
   > CACHE_CONFIG = {
   > 'CACHE_TYPE': 'RedisCache',
   > 'CACHE_DEFAULT_TIMEOUT': 86400,
   > 'CACHE_KEYPREFIX': 'superset',
   > 'CACHE_REDIS_HOST': 'localhost',
   > 'CACHE_REDIS_PORT': 6379,
   > 'CACHE_REDIS_DB': 1,
   > 'CACHE_REDIS_URL': 'redis://localhost:6379/1'
   
   As suggested in installation instructions, I have setup celery workers and 
running "superset worker & to start superset before I run "superset runserver 
&". When I start the superset workers. I am seeing the following error message :
   
   > --- ** * -- Linux-4.4.0-1052-aws-x86_64-with-Ubuntu-16.04-xenial 
2018-04-11 10:31:55
   > 
   > ** -- [config]
   > ** -- .> app: main:0x7f1652b31c50
   > -- .> transport: amqp://guest:@localhost:5672//
   > ** -- .> results: disabled://
   > --- --- .> concurrency: 32 (prefork)
   > -- *  .> task events: OFF (enable -E to monitor tasks in this worker)
   > -- [queues]
   > .> celery exchange=celery(direct) key=celery
   > 
   > [2018-04-11 10:31:58,576: ERROR/MainProcess] consumer: Cannot connect to 
amqp://guest:@127.0.0.1:5672//: [Errno 111] Connection refused.
   > Trying again in 32.00 seconds... Cannot connect to 
amqp://guest:@127.0.0.1:5672//: [Errno 111] Connection refused.
   > 
   
   `


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] deepak-goel commented on issue #4785: Not able to get the debug logs or python stacktrace on errors even after LOG_LEVEL is set to 'DEBUG'

[GitBox]

deepak-goel commented on issue #4785: Not able to get the debug logs or python 
stacktrace on errors even after LOG_LEVEL is set to 'DEBUG'
URL: 
https://github.com/apache/incubator-superset/issues/4785#issuecomment-380430067
 
 
   I got this working. I added the following 3 parameters 
   
   ```AUTH_USER_REGISTRATION = True```
   ```AUTH_ROLE_PUBLIC = 'Public'```
   ```AUTH_USER_REGISTRATION_ROLE = 'Public'```
   
   Superset config specifically needs the user registration as True for any 
user that logs in the first time. Closing this issue now.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] deepak-goel closed issue #4785: Not able to get the debug logs or python stacktrace on errors even after LOG_LEVEL is set to 'DEBUG'

[GitBox]

deepak-goel closed issue #4785: Not able to get the debug logs or python 
stacktrace on errors even after LOG_LEVEL is set to 'DEBUG'
URL: https://github.com/apache/incubator-superset/issues/4785
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] SpyderRivera commented on issue #4525: Superset issue #4512: multiple metrics, group by, opacity, legends for histogram

[GitBox]

SpyderRivera commented on issue #4525: Superset issue #4512: multiple metrics, 
group by, opacity, legends for histogram
URL: 
https://github.com/apache/incubator-superset/pull/4525#issuecomment-380460949
 
 
   
![bitmoji](https://render.bitstrips.com/v2/cpanel/ee47f314-e678-4ac2-b3dc-5fa399e731c8-09178b39-9d8a-4dc6-94b9-2b76fa3848ae-v1.png?transparent=1=1=246)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4712: Added order_by() to SqlaTable to ensure alphabetically ordered filter options values.

[GitBox]

codecov-io commented on issue #4712: Added order_by() to SqlaTable to ensure 
alphabetically ordered filter options values.
URL: 
https://github.com/apache/incubator-superset/pull/4712#issuecomment-380476608
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=h1)
 Report
   > Merging 
[#4712](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/68dec245423a64808b6edb45ce2bedbbaf1a000d?src=pr=desc)
 will **increase** coverage by `0.41%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4712/graphs/tree.svg?height=150=650=KsB0fHcx6l=pr)](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=tree)
   
   ```diff
   @@Coverage Diff @@
   ##   master#4712  +/-   ##
   ==
   + Coverage   71.87%   72.29%   +0.41% 
   ==
 Files 204  208   +4 
 Lines   1532315522 +199 
 Branches 1177 1202  +25 
   ==
   + Hits1101411222 +208 
   + Misses   4306 4297   -9 
 Partials33
   ```
   
   
   | [Impacted 
Files](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=tree)
 | Coverage Δ | |
   |---|---|---|
   | 
[superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==)
 | `77.08% <ø> (-0.21%)` | :arrow_down: |
   | 
[superset/assets/javascripts/modules/sandbox.js](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL21vZHVsZXMvc2FuZGJveC5qcw==)
 | `93.75% <0%> (-6.25%)` | :arrow_down: |
   | 
[...explore/components/controls/SelectAsyncControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RBc3luY0NvbnRyb2wuanN4)
 | `63.15% <0%> (-1.55%)` | :arrow_down: |
   | 
[...s/javascripts/explore/components/ControlHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sSGVhZGVyLmpzeA==)
 | `73.52% <0%> (-1.48%)` | :arrow_down: |
   | 
[...set/assets/javascripts/explore/stores/controls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvc3RvcmVzL2NvbnRyb2xzLmpzeA==)
 | `38.02% <0%> (-1.24%)` | :arrow_down: |
   | 
[superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvdml6LnB5)
 | `78.41% <0%> (-1.21%)` | :arrow_down: |
   | 
[superset/dataframe.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5)
 | `97.7% <0%> (-1.05%)` | :arrow_down: |
   | 
[superset/cli.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvY2xpLnB5)
 | `46.83% <0%> (-0.91%)` | :arrow_down: |
   | 
[...javascripts/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIuanN4)
 | `76.05% <0%> (-0.42%)` | :arrow_down: |
   | ... and [29 
more](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree-more)
 | |
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=footer).
 Last update 
[68dec24...bdb540e](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] john-bodley commented on issue #4612: In the “SQL Lab”, the “Results” section shows “Pending” constantly

[GitBox]

john-bodley commented on issue #4612: In the “SQL Lab”, the “Results” section 
shows “Pending” constantly
URL: 
https://github.com/apache/incubator-superset/issues/4612#issuecomment-380522122
 
 
   @myarnav I few things to check:
   1. Is your Redis service running?
   2. It seems that you haven't defined the 
[CeleryConfig](https://github.com/apache/incubator-superset/blob/master/superset/config.py#L268)
 object. The `BROKER_URL` and `CELERY_RESULT_BACKEND` should reference your 
Redis database, i.e., `redis://localhost:6379/1`.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4806: [tests] cleaning up test configuration

[GitBox]

codecov-io commented on issue #4806: [tests] cleaning up test configuration
URL: 
https://github.com/apache/incubator-superset/pull/4806#issuecomment-380523853
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=h1)
 Report
   > Merging 
[#4806](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/adf9ec0bb1943caa16b7da5515741c1f4905c0ea?src=pr=desc)
 will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4806/graphs/tree.svg?src=pr=KsB0fHcx6l=650=150)](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=tree)
   
   ```diff
   @@   Coverage Diff   @@
   ##   master#4806   +/-   ##
   ===
 Coverage   72.29%   72.29%   
   ===
 Files 208  208   
 Lines   1552215522   
 Branches 1202 1202   
   ===
 Hits1122211222   
 Misses   4297 4297   
 Partials33
   ```
   
   
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=footer).
 Last update 
[adf9ec0...1d543d8](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] john-bodley opened a new pull request #4806: [tests] cleaning up test configuration

[GitBox]

john-bodley opened a new pull request #4806: [tests] cleaning up test 
configuration
URL: https://github.com/apache/incubator-superset/pull/4806
 
 
   This PR deletes the obsolete `run_specfic_test.sh` script which is now 
handled via `tox`. Additionally the need for setting the `SUPERSET_CONFIG` 
environment variable in the testing code as this is also handled via `tox` 
[here](https://github.com/apache/incubator-superset/blob/master/tox.ini#L36).


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jeffreythewang commented on issue #4520: Allow users to view dashboards they own

[GitBox]

jeffreythewang commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380523149
 
 
   @john-bodley Yes that is the current logic. This change mainly addresses the 
case where a Dashboard is empty, as the user will not be able to see it since 
they do not have access to any slices in the Dashboard (because there are no 
slices in the Dashboard).
   
   With this change, if the Dashboard contains zero slices, but you are the 
owner of that Dashboard, you will still be able to view it in the Dashboard 
list. This happens when a user wants to start by creating an empty dashboard, 
and then adding slices to it. It's not a common use case, but it happens 
sometimes.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] JeppeSigaard commented on issue #4712: Added localecompare array sort to datahandler, fetchFilterValues

[GitBox]

JeppeSigaard commented on issue #4712: Added localecompare array sort to 
datahandler, fetchFilterValues
URL: 
https://github.com/apache/incubator-superset/pull/4712#issuecomment-380472916
 
 
   Well, in terms of database in use, I just popped open the dev setup. 
Non-alphabetical values can be spotted by setting up a dev environment using 
the test data as per the documentation. 
   A good example would be the response of 
http://localhost:8088/superset/filter/table/2/country_code/, where "DZA" is 
popped in there with the A's.
   
   I rolled back the client side array sort and instead[ added an 
`order_by()`](https://github.com/apache/incubator-superset/pull/4712/files#diff-6506def3966137541a05177a6fb169d7R397)
 as suggested.
   
   This is also a valid solution  


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4712: Added order_by() to SqlaTable to ensure alphabetically ordered filter options values.

[GitBox]

codecov-io commented on issue #4712: Added order_by() to SqlaTable to ensure 
alphabetically ordered filter options values.
URL: 
https://github.com/apache/incubator-superset/pull/4712#issuecomment-380476608
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=h1)
 Report
   > Merging 
[#4712](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/68dec245423a64808b6edb45ce2bedbbaf1a000d?src=pr=desc)
 will **increase** coverage by `0.41%`.
   > The diff coverage is `100%`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4712/graphs/tree.svg?src=pr=KsB0fHcx6l=650=150)](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=tree)
   
   ```diff
   @@Coverage Diff @@
   ##   master#4712  +/-   ##
   ==
   + Coverage   71.87%   72.29%   +0.41% 
   ==
 Files 204  208   +4 
 Lines   1532315522 +199 
 Branches 1177 1202  +25 
   ==
   + Hits1101411222 +208 
   + Misses   4306 4297   -9 
 Partials33
   ```
   
   
   | [Impacted 
Files](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=tree)
 | Coverage Δ | |
   |---|---|---|
   | 
[superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==)
 | `77.08% <ø> (-0.21%)` | :arrow_down: |
   | 
[...ipts/explore/components/controls/FilterControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sLmpzeA==)
 | `97.29% <100%> (ø)` | :arrow_up: |
   | 
[superset/assets/javascripts/modules/sandbox.js](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL21vZHVsZXMvc2FuZGJveC5qcw==)
 | `93.75% <0%> (-6.25%)` | :arrow_down: |
   | 
[...explore/components/controls/SelectAsyncControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RBc3luY0NvbnRyb2wuanN4)
 | `63.15% <0%> (-1.55%)` | :arrow_down: |
   | 
[...s/javascripts/explore/components/ControlHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sSGVhZGVyLmpzeA==)
 | `73.52% <0%> (-1.48%)` | :arrow_down: |
   | 
[...set/assets/javascripts/explore/stores/controls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvc3RvcmVzL2NvbnRyb2xzLmpzeA==)
 | `38.02% <0%> (-1.24%)` | :arrow_down: |
   | 
[superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvdml6LnB5)
 | `78.41% <0%> (-1.21%)` | :arrow_down: |
   | 
[superset/dataframe.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5)
 | `97.7% <0%> (-1.05%)` | :arrow_down: |
   | 
[superset/cli.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvY2xpLnB5)
 | `46.83% <0%> (-0.91%)` | :arrow_down: |
   | 
[...javascripts/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIuanN4)
 | `76.05% <0%> (-0.42%)` | :arrow_down: |
   | ... and [30 
more](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree-more)
 | |
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=footer).
 Last update 
[68dec24...bdb540e](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4712: Added order_by() to SqlaTable to ensure alphabetically ordered filter options values.

[GitBox]

codecov-io commented on issue #4712: Added order_by() to SqlaTable to ensure 
alphabetically ordered filter options values.
URL: 
https://github.com/apache/incubator-superset/pull/4712#issuecomment-380476608
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=h1)
 Report
   > Merging 
[#4712](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/68dec245423a64808b6edb45ce2bedbbaf1a000d?src=pr=desc)
 will **increase** coverage by `0.41%`.
   > The diff coverage is `100%`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4712/graphs/tree.svg?src=pr=650=KsB0fHcx6l=150)](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=tree)
   
   ```diff
   @@Coverage Diff @@
   ##   master#4712  +/-   ##
   ==
   + Coverage   71.87%   72.29%   +0.41% 
   ==
 Files 204  208   +4 
 Lines   1532315522 +199 
 Branches 1177 1202  +25 
   ==
   + Hits1101411222 +208 
   + Misses   4306 4297   -9 
 Partials33
   ```
   
   
   | [Impacted 
Files](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=tree)
 | Coverage Δ | |
   |---|---|---|
   | 
[superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==)
 | `77.08% <ø> (-0.21%)` | :arrow_down: |
   | 
[...ipts/explore/components/controls/FilterControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sLmpzeA==)
 | `97.29% <100%> (ø)` | :arrow_up: |
   | 
[superset/assets/javascripts/modules/sandbox.js](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL21vZHVsZXMvc2FuZGJveC5qcw==)
 | `93.75% <0%> (-6.25%)` | :arrow_down: |
   | 
[...explore/components/controls/SelectAsyncControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RBc3luY0NvbnRyb2wuanN4)
 | `63.15% <0%> (-1.55%)` | :arrow_down: |
   | 
[...s/javascripts/explore/components/ControlHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sSGVhZGVyLmpzeA==)
 | `73.52% <0%> (-1.48%)` | :arrow_down: |
   | 
[...set/assets/javascripts/explore/stores/controls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvc3RvcmVzL2NvbnRyb2xzLmpzeA==)
 | `38.02% <0%> (-1.24%)` | :arrow_down: |
   | 
[superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvdml6LnB5)
 | `78.41% <0%> (-1.21%)` | :arrow_down: |
   | 
[superset/dataframe.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5)
 | `97.7% <0%> (-1.05%)` | :arrow_down: |
   | 
[superset/cli.py](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvY2xpLnB5)
 | `46.83% <0%> (-0.91%)` | :arrow_down: |
   | 
[...javascripts/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIuanN4)
 | `76.05% <0%> (-0.42%)` | :arrow_down: |
   | ... and [30 
more](https://codecov.io/gh/apache/incubator-superset/pull/4712/diff?src=pr=tree-more)
 | |
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=footer).
 Last update 
[68dec24...bdb540e](https://codecov.io/gh/apache/incubator-superset/pull/4712?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] JeppeSigaard commented on issue #4712: Added localecompare array sort to datahandler, fetchFilterValues

[GitBox]

JeppeSigaard commented on issue #4712: Added localecompare array sort to 
datahandler, fetchFilterValues
URL: 
https://github.com/apache/incubator-superset/pull/4712#issuecomment-380472916
 
 
   Well, in terms of database in use, I just popped open the dev setup. 
Non-alphabetical values can be spotted by setting up a dev environment using 
the test data as per the documentation. 
   A good example would be the response of 
http://localhost:8088/superset/filter/table/2/country_code/, where "DZA" is 
popped in there with the A's.
   
   I rolled back the client side array sort and instead added an `order_by()` 
as suggested.
   This is also a valid solution  


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] john-bodley commented on issue #4520: Allow users to view dashboards they own

[GitBox]

john-bodley commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380519069
 
 
   @jeffreythewang isn't the current logic that a user is able to see the 
dashboard if the have access to _at least_ one of the underlying slices (as 
opposed to _all_), per this logic:
   ```
   query = query.filter(
   Dash.id.in_(
   db.session.query(Dash.id)
   .distinct()
   .join(Dash.slices)
   .filter(Slice.id.in_(slice_ids_qry)),
),
   )
   ```
   My understanding is this query returns the distinct dashboard IDs which are 
represented by _any_ (per the `IN condition) of the sanctioned slices. I wonder 
whether this is being confused with 
https://github.com/apache/incubator-superset/issues/4737.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Curl Based REST API For Superset Slice & Dashboard Creation

[ashutosh khira]

Hi,

Please provide Curl Based REST API document for superset’s slice &
dashboard creation.

Thanks
Ashutosh Khira

[GitHub] codecov-io commented on issue #4806: [tests] cleaning up test configuration

[GitBox]

codecov-io commented on issue #4806: [tests] cleaning up test configuration
URL: 
https://github.com/apache/incubator-superset/pull/4806#issuecomment-380523853
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=h1)
 Report
   > Merging 
[#4806](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/adf9ec0bb1943caa16b7da5515741c1f4905c0ea?src=pr=desc)
 will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4806/graphs/tree.svg?width=650=150=KsB0fHcx6l=pr)](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=tree)
   
   ```diff
   @@   Coverage Diff   @@
   ##   master#4806   +/-   ##
   ===
 Coverage   72.29%   72.29%   
   ===
 Files 208  208   
 Lines   1552215522   
 Branches 1202 1202   
   ===
 Hits1122211222   
 Misses   4297 4297   
 Partials33
   ```
   
   
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=footer).
 Last update 
[adf9ec0...1d543d8](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] john-bodley commented on issue #4520: Allow users to view dashboards they own

[GitBox]

john-bodley commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380540209
 
 
   Thanks @jeffreythewang 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch opened a new pull request #4807: Make the bottom margin a bit taller

[GitBox]

mistercrunch opened a new pull request #4807: Make the bottom margin a bit 
taller
URL: https://github.com/apache/incubator-superset/pull/4807
 
 
   https://user-images.githubusercontent.com/487433/38634108-768b04ca-3d76-11e8-8bd2-c6d3c24a8ceb.png;>
   https://user-images.githubusercontent.com/487433/38634109-76a5e90c-3d76-11e8-9322-e5e9f2ce58f8.png;>
   https://user-images.githubusercontent.com/487433/38634110-76bea2c6-3d76-11e8-845f-e6e232c745b7.png;>
   https://user-images.githubusercontent.com/487433/38634111-76d4b2aa-3d76-11e8-9862-08a1639a76f5.png;>
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jeffreythewang commented on issue #4520: Allow users to view dashboards they own

[GitBox]

jeffreythewang commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380537061
 
 
   @john-bodley I think what I meant by that was, if there is a dashboard where 
all of the charts on that dashboard are inaccessible to the user, but the user 
is the owner of the dashboard, then in the current implementation, the user 
will not see that dashboard in the list view.
   
   Also, I'll add a unit test.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jeffreythewang commented on issue #4520: Allow users to view dashboards they own

[GitBox]

jeffreythewang commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380537061
 
 
   @john-bodley I think what I meant by that was, if there is a dashboard where 
all of the charts on that dashboard are inaccessible to the user, but the user 
is the owner of the dashboard, then in the current implementation, the user 
will not see that dashboard in the dashboard list view.
   
   Also, I'll add a unit test.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4651: [explore] proper filtering of NULLs and ''

[GitBox]

codecov-io commented on issue #4651: [explore] proper filtering of NULLs and ''
URL: 
https://github.com/apache/incubator-superset/pull/4651#issuecomment-374685460
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=h1)
 Report
   > Merging 
[#4651](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/adf9ec0bb1943caa16b7da5515741c1f4905c0ea?src=pr=desc)
 will **increase** coverage by `0.02%`.
   > The diff coverage is `76.62%`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4651/graphs/tree.svg?width=650=150=pr=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=tree)
   
   ```diff
   @@Coverage Diff @@
   ##   master#4651  +/-   ##
   ==
   + Coverage   72.29%   72.32%   +0.02% 
   ==
 Files 208  208  
 Lines   1552215571  +49 
 Branches 1202 1209   +7 
   ==
   + Hits1122211261  +39 
   - Misses   4297 4307  +10 
 Partials33
   ```
   
   
   | [Impacted 
Files](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=tree)
 | Coverage Δ | |
   |---|---|---|
   | 
[...et/assets/javascripts/components/OnPasteSelect.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbXBvbmVudHMvT25QYXN0ZVNlbGVjdC5qc3g=)
 | `93.18% <ø> (ø)` | :arrow_up: |
   | 
[superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvdml6LnB5)
 | `78.44% <100%> (+0.02%)` | :arrow_up: |
   | 
[superset/assets/javascripts/common.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbW1vbi5qcw==)
 | `40% <100%> (+40%)` | :arrow_up: |
   | 
[.../assets/javascripts/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==)
 | `100% <100%> (ø)` | :arrow_up: |
   | 
[superset/assets/javascripts/chart/chartAction.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NoYXJ0L2NoYXJ0QWN0aW9uLmpz)
 | `52.52% <33.33%> (-0.6%)` | :arrow_down: |
   | 
[superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==)
 | `76.1% <44.44%> (-0.99%)` | :arrow_down: |
   | 
[superset/assets/javascripts/SqlLab/actions.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL1NxbExhYi9hY3Rpb25zLmpz)
 | `72.44% <50%> (+0.12%)` | :arrow_up: |
   | 
[superset/connectors/druid/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9kcnVpZC9tb2RlbHMucHk=)
 | `81.14% <73.33%> (-0.48%)` | :arrow_down: |
   | 
[...javascripts/explore/components/controls/Filter.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXIuanN4)
 | `78.78% <75%> (-0.58%)` | :arrow_down: |
   | 
[superset/connectors/base/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9iYXNlL21vZGVscy5weQ==)
 | `90.38% <86.95%> (-0.6%)` | :arrow_down: |
   | ... and [2 
more](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree-more)
 | |
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=footer).
 Last update 
[adf9ec0...004aac5](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4651: [explore] proper filtering of NULLs and ''

[GitBox]

codecov-io commented on issue #4651: [explore] proper filtering of NULLs and ''
URL: 
https://github.com/apache/incubator-superset/pull/4651#issuecomment-374685460
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=h1)
 Report
   > Merging 
[#4651](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/adf9ec0bb1943caa16b7da5515741c1f4905c0ea?src=pr=desc)
 will **increase** coverage by `0.02%`.
   > The diff coverage is `76.62%`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4651/graphs/tree.svg?src=pr=KsB0fHcx6l=650=150)](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=tree)
   
   ```diff
   @@Coverage Diff @@
   ##   master#4651  +/-   ##
   ==
   + Coverage   72.29%   72.32%   +0.02% 
   ==
 Files 208  208  
 Lines   1552215571  +49 
 Branches 1202 1209   +7 
   ==
   + Hits1122211261  +39 
   - Misses   4297 4307  +10 
 Partials33
   ```
   
   
   | [Impacted 
Files](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=tree)
 | Coverage Δ | |
   |---|---|---|
   | 
[...et/assets/javascripts/components/OnPasteSelect.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbXBvbmVudHMvT25QYXN0ZVNlbGVjdC5qc3g=)
 | `93.18% <ø> (ø)` | :arrow_up: |
   | 
[superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvdml6LnB5)
 | `78.44% <100%> (+0.02%)` | :arrow_up: |
   | 
[superset/assets/javascripts/common.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbW1vbi5qcw==)
 | `40% <100%> (+40%)` | :arrow_up: |
   | 
[.../assets/javascripts/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==)
 | `100% <100%> (ø)` | :arrow_up: |
   | 
[superset/assets/javascripts/chart/chartAction.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NoYXJ0L2NoYXJ0QWN0aW9uLmpz)
 | `52.52% <33.33%> (-0.6%)` | :arrow_down: |
   | 
[superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==)
 | `76.1% <44.44%> (-0.99%)` | :arrow_down: |
   | 
[superset/assets/javascripts/SqlLab/actions.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL1NxbExhYi9hY3Rpb25zLmpz)
 | `72.44% <50%> (+0.12%)` | :arrow_up: |
   | 
[superset/connectors/druid/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9kcnVpZC9tb2RlbHMucHk=)
 | `81.14% <73.33%> (-0.48%)` | :arrow_down: |
   | 
[...javascripts/explore/components/controls/Filter.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXIuanN4)
 | `78.78% <75%> (-0.58%)` | :arrow_down: |
   | 
[superset/connectors/base/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9iYXNlL21vZGVscy5weQ==)
 | `90.38% <86.95%> (-0.6%)` | :arrow_down: |
   | ... and [2 
more](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree-more)
 | |
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=footer).
 Last update 
[adf9ec0...004aac5](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4651: [WiP] [explore] proper filtering of NULLs and ''

[GitBox]

codecov-io commented on issue #4651: [WiP] [explore] proper filtering of NULLs 
and ''
URL: 
https://github.com/apache/incubator-superset/pull/4651#issuecomment-374685460
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=h1)
 Report
   > Merging 
[#4651](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/adf9ec0bb1943caa16b7da5515741c1f4905c0ea?src=pr=desc)
 will **increase** coverage by `0.02%`.
   > The diff coverage is `76.62%`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4651/graphs/tree.svg?height=150=650=KsB0fHcx6l=pr)](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=tree)
   
   ```diff
   @@Coverage Diff @@
   ##   master#4651  +/-   ##
   ==
   + Coverage   72.29%   72.32%   +0.02% 
   ==
 Files 208  208  
 Lines   1552215571  +49 
 Branches 1202 1209   +7 
   ==
   + Hits1122211261  +39 
   - Misses   4297 4307  +10 
 Partials33
   ```
   
   
   | [Impacted 
Files](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=tree)
 | Coverage Δ | |
   |---|---|---|
   | 
[...et/assets/javascripts/components/OnPasteSelect.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbXBvbmVudHMvT25QYXN0ZVNlbGVjdC5qc3g=)
 | `93.18% <ø> (ø)` | :arrow_up: |
   | 
[.../assets/javascripts/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==)
 | `100% <100%> (ø)` | :arrow_up: |
   | 
[superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvdml6LnB5)
 | `78.44% <100%> (+0.02%)` | :arrow_up: |
   | 
[superset/assets/javascripts/common.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NvbW1vbi5qcw==)
 | `40% <100%> (+40%)` | :arrow_up: |
   | 
[superset/assets/javascripts/chart/chartAction.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2NoYXJ0L2NoYXJ0QWN0aW9uLmpz)
 | `52.52% <33.33%> (-0.6%)` | :arrow_down: |
   | 
[superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==)
 | `76.1% <44.44%> (-0.99%)` | :arrow_down: |
   | 
[superset/assets/javascripts/SqlLab/actions.js](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL1NxbExhYi9hY3Rpb25zLmpz)
 | `72.44% <50%> (+0.12%)` | :arrow_up: |
   | 
[superset/connectors/druid/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9kcnVpZC9tb2RlbHMucHk=)
 | `81.14% <73.33%> (-0.48%)` | :arrow_down: |
   | 
[...javascripts/explore/components/controls/Filter.jsx](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvYXNzZXRzL2phdmFzY3JpcHRzL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXIuanN4)
 | `78.78% <75%> (-0.58%)` | :arrow_down: |
   | 
[superset/connectors/base/models.py](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9iYXNlL21vZGVscy5weQ==)
 | `90.38% <86.95%> (-0.6%)` | :arrow_down: |
   | ... and [2 
more](https://codecov.io/gh/apache/incubator-superset/pull/4651/diff?src=pr=tree-more)
 | |
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=footer).
 Last update 
[adf9ec0...570e108](https://codecov.io/gh/apache/incubator-superset/pull/4651?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jeffreythewang commented on issue #4520: Allow users to view dashboards they own

[GitBox]

jeffreythewang commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380537061
 
 
   Ah I see. Yeah that comment was written before 
https://github.com/apache/incubator-superset/pull/4405 was merged. I've updated 
it now.
   
   I'll add a unit test.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jeffreythewang commented on issue #4520: Allow users to view dashboards they own

[GitBox]

jeffreythewang commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380537061
 
 
   @john-bodley  Ah I see. Yeah that comment was written before I was aware of 
https://github.com/apache/incubator-superset/pull/4405. I've updated it now.
   
   I'll add a unit test.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jeffreythewang commented on issue #4520: Allow users to view dashboards they own

[GitBox]

jeffreythewang commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380537061
 
 
   @john-bodley  Ah I see. Yeah that comment was written before 
https://github.com/apache/incubator-superset/pull/4405 was merged. I've updated 
it now.
   
   I'll add a unit test.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] john-bodley commented on issue #4520: Allow users to view dashboards they own

[GitBox]

john-bodley commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380530013
 
 
   @jeffreythewang I agree with your logic regarding empty dashboards which 
this PR resolves, but I think your original comment
   > For the case where they are the owner of a dashboard, but their dashboard 
has charts they do not have access to (which can only happen if an additional 
owner adds an inaccessible chart to the dashboard)
   
   is not correct according to the current logic. Maybe it would be good to 
updated the PR title and description to better reflect the change. Also I 
wonder whether a unit test would be valuable here. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] timifasubaa commented on issue #4804: [upload csv to hive] fix call to unicodereader.next()

[GitBox]

timifasubaa commented on issue #4804: [upload csv to hive] fix call to 
unicodereader.next()
URL: 
https://github.com/apache/incubator-superset/pull/4804#issuecomment-380561299
 
 
   @john-bodley yes. Thanks for making it clearer. I updated the PR description
   I tested on python2 and 3 and it works fine.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4806: [tests] cleaning up test configuration

[GitBox]

codecov-io commented on issue #4806: [tests] cleaning up test configuration
URL: 
https://github.com/apache/incubator-superset/pull/4806#issuecomment-380523853
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=h1)
 Report
   > Merging 
[#4806](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/adf9ec0bb1943caa16b7da5515741c1f4905c0ea?src=pr=desc)
 will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4806/graphs/tree.svg?width=650=pr=KsB0fHcx6l=150)](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=tree)
   
   ```diff
   @@   Coverage Diff   @@
   ##   master#4806   +/-   ##
   ===
 Coverage   72.29%   72.29%   
   ===
 Files 208  208   
 Lines   1552215522   
 Branches 1202 1202   
   ===
 Hits1122211222   
 Misses   4297 4297   
 Partials33
   ```
   
   
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=footer).
 Last update 
[adf9ec0...1d543d8](https://codecov.io/gh/apache/incubator-superset/pull/4806?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] john-bodley commented on issue #4520: Allow users to view dashboards they own

[GitBox]

john-bodley commented on issue #4520: Allow users to view dashboards they own
URL: 
https://github.com/apache/incubator-superset/pull/4520#issuecomment-380540555
 
 
   Note this filter is also used when you try to add a slice to an existing 
dashboard and thus if you create an empty dashboard you'll now be able to add a 
slice to it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] timifasubaa commented on issue #4659: Adding tests for the time table viz

[GitBox]

timifasubaa commented on issue #4659: Adding tests for the time table viz
URL: 
https://github.com/apache/incubator-superset/pull/4659#issuecomment-380561863
 
 
   @mistercrunch @john-bodley  Why haven't we merged this?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] john-bodley commented on issue #4804: [upload csv to hive] fix call to unicodereader.next()

[GitBox]

john-bodley commented on issue #4804: [upload csv to hive] fix call to 
unicodereader.next()
URL: 
https://github.com/apache/incubator-superset/pull/4804#issuecomment-380565990
 
 
   LGTM


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] john-bodley commented on issue #4804: [upload csv to hive] fix call to unicodereader.next()

[GitBox]

john-bodley commented on issue #4804: [upload csv to hive] fix call to 
unicodereader.next()
URL: 
https://github.com/apache/incubator-superset/pull/4804#issuecomment-380565990
 
 
   LGTM


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4807: Make the bottom margin a bit taller

[GitBox]

codecov-io commented on issue #4807: Make the bottom margin a bit taller
URL: 
https://github.com/apache/incubator-superset/pull/4807#issuecomment-380544999
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=h1)
 Report
   > Merging 
[#4807](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/adf9ec0bb1943caa16b7da5515741c1f4905c0ea?src=pr=desc)
 will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4807/graphs/tree.svg?width=650=150=KsB0fHcx6l=pr)](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=tree)
   
   ```diff
   @@   Coverage Diff   @@
   ##   master#4807   +/-   ##
   ===
 Coverage   72.29%   72.29%   
   ===
 Files 208  208   
 Lines   1552215522   
 Branches 1202 1202   
   ===
 Hits1122211222   
 Misses   4297 4297   
 Partials33
   ```
   
   
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=footer).
 Last update 
[adf9ec0...8995c37](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4807: Make the bottom margin a bit taller

[GitBox]

codecov-io commented on issue #4807: Make the bottom margin a bit taller
URL: 
https://github.com/apache/incubator-superset/pull/4807#issuecomment-380544999
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=h1)
 Report
   > Merging 
[#4807](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/adf9ec0bb1943caa16b7da5515741c1f4905c0ea?src=pr=desc)
 will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4807/graphs/tree.svg?width=650=150=KsB0fHcx6l=pr)](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=tree)
   
   ```diff
   @@   Coverage Diff   @@
   ##   master#4807   +/-   ##
   ===
 Coverage   72.29%   72.29%   
   ===
 Files 208  208   
 Lines   1552215522   
 Branches 1202 1202   
   ===
 Hits1122211222   
 Misses   4297 4297   
 Partials33
   ```
   
   
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=footer).
 Last update 
[adf9ec0...8995c37](https://codecov.io/gh/apache/incubator-superset/pull/4807?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] timifasubaa commented on issue #4804: [upload csv to hive] fix call to unicodereader.next()

[GitBox]

timifasubaa commented on issue #4804: [upload csv to hive] fix call to 
unicodereader.next()
URL: 
https://github.com/apache/incubator-superset/pull/4804#issuecomment-380561299
 
 
   @john-bodley yes. Thanks for clarifying. I updated the PR description. 
   I tested on python2 and 3 and it works fine.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] graceguo-supercat commented on issue #4791: apply new Dashboard builder to dashboard

[GitBox]

graceguo-supercat commented on issue #4791: apply new Dashboard builder to 
dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#issuecomment-380604576
 
 
   thanks for your comment. here is my thoughts:
   
   1. I don't like the idea of delete v1, it will destroy code history and make 
huge duplicates, huge merge conflicts, and very hard to track the changing or 
fixing of existed features. 
   
   Dashboard v2 is a new project, but most of v2's functionalities are same as 
v1, like filters, chart render, refresh and immune etc. New Dashboard will 
improve the layout and organization of Dashboard, which account for a portion 
of whole Dashboard functionality. Adding new feature, and adding new blocks to 
existed work is what wee need. 
   
   2. Naming: 
   
   `charts` are dynamic queries. Each of chart is basically a query based on 
saved params + dashboard added filter. It's attributes are mostly are dynamic, 
like request params, response data, start time, last updated time etc.
   
   `allSlices` mapped to `slices` table in backend. It has static data for 
saved queries (after save query as slice), like name, owner, datasource name, 
timestamp, param list, etc. These data are new to dashboard, to help user 
review, filter, sort all existed slices, then add new slices into dashboard. 
Different than chart data is fetched one by one, available slices should fetch 
once and fetch all slices data together.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] graceguo-supercat commented on issue #4791: apply new Dashboard builder to dashboard

[GitBox]

graceguo-supercat commented on issue #4791: apply new Dashboard builder to 
dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#issuecomment-380604576
 
 
   thanks for your comment. here is my thoughts:
   
   1. I don't like the idea of delete v1, it will destroy code history and make 
huge duplicates, huge merge conflicts, and very hard to track the changing or 
fixing of existed features. 
   
   Dashboard v2 is a new project, but most of v2's functionalities are same as 
v1, like filters, chart render, refresh and immune etc. We will add new 
functions/components to improve the layout and organization of Dashboard, not 
switch to branch new project.
   
   2. Naming: I don't have good idea about redux state keys. Please feel free 
to recommend.
   
   `charts` are dynamic queries. Each of chart is basically a query based on 
saved params + dashboard added filter. It's attributes are mostly are dynamic, 
like request params, response data, start time, last updated time etc.
   
   `allSlices` mapped to `slices` table in backend. It has static data for 
those saved queries (after save query as slice), like name, owner, datasource 
name, timestamp, param list, etc. These data are used to help user review, 
filter, sort all existed slices, then they can add new slices into dashboard. 
Different than chart data that is fetched one by one, available slices data 
should be fetched all together and only once.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] graceguo-supercat commented on issue #4791: apply new Dashboard builder to dashboard

[GitBox]

graceguo-supercat commented on issue #4791: apply new Dashboard builder to 
dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#issuecomment-380604576
 
 
   thanks for your comment. here are my thoughts:
   
   1. I don't like the idea of delete v1, it will destroy code history and make 
huge duplicates, huge merge conflicts, and very hard to track the changing or 
fixing of existed features. 
   
   Dashboard v2 is a new project, but most of v2's functionalities are same as 
v1, like filters, chart render, refresh and immune etc. We will add new 
functions/components to improve the layout and organization of Dashboard, not 
switch to branch new project.
   
   2. Naming: I don't have good idea about redux state keys. Please feel free 
to recommend.
   
   `charts` are dynamic queries. Each of chart is basically a query based on 
saved params + dashboard added filter. It's attributes are mostly are dynamic, 
like request params, response data, start time, last updated time etc.
   
   `allSlices` mapped to `slices` table in backend. It has static data for 
those saved queries (after save query as slice), like name, owner, datasource 
name, timestamp, param list, etc. These data are used to help user review, 
filter, sort all existed slices, then they can add new slices into dashboard. 
Different than chart data that is fetched one by one, available slices data 
should be fetched all together and only once.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] hughhhh closed pull request #4809: Filter recently viewed to just have explore and dashboard types

[GitBox]

hug closed pull request #4809: Filter recently viewed to just have explore 
and dashboard types
URL: https://github.com/apache/incubator-superset/pull/4809
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/superset/assets/javascripts/profile/components/RecentActivity.jsx 
b/superset/assets/javascripts/profile/components/RecentActivity.jsx
index 7099a08649..14cf7a8ddc 100644
--- a/superset/assets/javascripts/profile/components/RecentActivity.jsx
+++ b/superset/assets/javascripts/profile/components/RecentActivity.jsx
@@ -12,12 +12,14 @@ export default class RecentActivity extends 
React.PureComponent {
   render() {
 const rowLimit = 50;
 const mutator = function (data) {
-  return data.map(row => ({
-name: {row.item_title},
-type: row.action,
-time: moment.utc(row.time).fromNow(),
-_time: row.time,
-  }));
+  return data
+.filter(row => row.action === 'dashboard' || row.action === 'explore')
+.map(row => ({
+  name: {row.item_title},
+  type: row.action,
+  time: moment.utc(row.time).fromNow(),
+  _time: row.time,
+}));
 };
 return (
   


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch closed pull request #4803: [explore] set working default for MetricsControl

[GitBox]

mistercrunch closed pull request #4803: [explore] set working default for 
MetricsControl
URL: https://github.com/apache/incubator-superset/pull/4803
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/superset/assets/javascripts/explore/stores/controls.jsx 
b/superset/assets/javascripts/explore/stores/controls.jsx
index d2efd53fe9..e8a1d3f75b 100644
--- a/superset/assets/javascripts/explore/stores/controls.jsx
+++ b/superset/assets/javascripts/explore/stores/controls.jsx
@@ -175,14 +175,17 @@ export const controls = {
 label: t('Metrics'),
 validators: [v.nonEmpty],
 default: (c) => {
-  const metric = mainMetric(c.options);
+  const metric = mainMetric(c.savedMetrics);
   return metric ? [metric] : null;
 },
-mapStateToProps: state => ({
-  columns: state.datasource ? state.datasource.columns : [],
-  savedMetrics: state.datasource ? state.datasource.metrics : [],
-  datasourceType: state.datasource && state.datasource.type,
-}),
+mapStateToProps: (state) => {
+  const datasource = state.datasource;
+  return {
+columns: datasource ? datasource.columns : [],
+savedMetrics: datasource ? datasource.metrics : [],
+datasourceType: datasource && datasource.type,
+  };
+},
 description: t('One or many metrics to display'),
   },
 
@@ -264,7 +267,7 @@ export const controls = {
 label: t('Metric'),
 clearable: false,
 validators: [v.nonEmpty],
-default: c => mainMetric(c.options),
+default: props => mainMetric(props.savedMetrics),
 mapStateToProps: state => ({
   columns: state.datasource ? state.datasource.columns : [],
   savedMetrics: state.datasource ? state.datasource.metrics : [],
diff --git a/superset/assets/javascripts/modules/utils.js 
b/superset/assets/javascripts/modules/utils.js
index 61807837c2..016444a9f7 100644
--- a/superset/assets/javascripts/modules/utils.js
+++ b/superset/assets/javascripts/modules/utils.js
@@ -260,17 +260,17 @@ export function getParam(name) {
   return results === null ? '' : decodeURIComponent(results[1].replace(/\+/g, 
' '));
 }
 
-export function mainMetric(metricOptions) {
+export function mainMetric(savedMetrics) {
   // Using 'count' as default metric if it exists, otherwise using whatever 
one shows up first
   let metric;
-  if (metricOptions && metricOptions.length > 0) {
-metricOptions.forEach((m) => {
+  if (savedMetrics && savedMetrics.length > 0) {
+savedMetrics.forEach((m) => {
   if (m.metric_name === 'count') {
 metric = 'count';
   }
 });
 if (!metric) {
-  metric = metricOptions[0].metric_name;
+  metric = savedMetrics[0].metric_name;
 }
   }
   return metric;


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch closed pull request #4801: [bugfix] dedup groupby columns in Deck visualizations

[GitBox]

mistercrunch closed pull request #4801: [bugfix] dedup groupby columns in Deck 
visualizations
URL: https://github.com/apache/incubator-superset/pull/4801
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/superset/viz.py b/superset/viz.py
index 87dd1a9ebe..dbfcdcc9b7 100644
--- a/superset/viz.py
+++ b/superset/viz.py
@@ -2019,6 +2019,7 @@ def query_obj(self):
 if fd.get('js_columns'):
 gb += fd.get('js_columns')
 metrics = self.get_metrics()
+gb = list(set(gb))
 if metrics:
 d['groupby'] = gb
 d['metrics'] = self.get_metrics()


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch closed pull request #4807: Make the bottom margin a bit taller

[GitBox]

mistercrunch closed pull request #4807: Make the bottom margin a bit taller
URL: https://github.com/apache/incubator-superset/pull/4807
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/superset/assets/visualizations/nvd3_vis.js 
b/superset/assets/visualizations/nvd3_vis.js
index eb5b70187e..284c4e0575 100644
--- a/superset/assets/visualizations/nvd3_vis.js
+++ b/superset/assets/visualizations/nvd3_vis.js
@@ -493,7 +493,7 @@ export default function nvd3Vis(slice, payload) {
   //  (x axis labels are rotated 45 degrees so we use height),
   // - adjust margins based on these measures and render again
   const margins = chart.margin();
-  margins.bottom = 20;
+  margins.bottom = 28;
   if (fd.x_axis_showminmax) {
 // If x bounds are shown, we need a right margin
 margins.right = Math.max(20, maxXAxisLabelHeight / 2) + marginPad;
@@ -502,7 +502,7 @@ export default function nvd3Vis(slice, payload) {
 margins.bottom = maxXAxisLabelHeight + marginPad;
 margins.right = maxXAxisLabelHeight + marginPad;
   } else if (staggerLabels) {
-margins.bottom = 30;
+margins.bottom = 40;
   }
 
   if (vizType === 'dual_line') {


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch commented on issue #4793: [line] do not show 0 or nulls in rich tooltip

[GitBox]

mistercrunch commented on issue #4793: [line] do not show 0 or nulls in rich 
tooltip
URL: 
https://github.com/apache/incubator-superset/pull/4793#issuecomment-380612770
 
 
   @vylc  `Show Zeroes in Tooltip` boolean? 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on issue #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on issue #4791: apply new Dashboard builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#issuecomment-380619142
 
 
   @graceguo-supercat 
   
   1) the code history etc is a valid concern. I think we should still delete 
v1, but move files with `git mv ...` so that it preserves history etc. 
otherwise we won't be sure we've deleted all unused code esp css.
   
   2) thanks for this explanation. next step is to clean it up / align on what 
it should be going forward! I think this would be an improvement:
   
   ```
   datasources
   chartMetadata
   chartData
   dashboardLayout
   dashboardMetadata
   title
   standalone_mode
   slug
   metadata
   id
   edit_perm  
   save_perm
   sliceIds => (delete this?)
   dashboardState
   filters
   userId
   editMode
   hasUnsavedChanges
   showBuilderPane
   common
   toastMessages
   ```
   
   **Explicit changes**
   - `allSlices` => `chartMetadata` (could consider `componentMetadata`, in the 
future we will have more than just "slices"?)
   - `charts` => `chartData`
   - `state.dashboard.dashboard` => separate it out to 
`state.dashboardMetadata`, it shouldn't be mixed with current UI state like 
filters/edit mode. `dashboard.dashboard` is not readable.
   - `state.dashboard.[rest]` => rename this state tree `dashboardState`

   I also don't want this point to be lost in the comments:
   We shouldn't set the id for `chartData` to `slice_[id]`, it should just be 
`id` and match `chartMetaData`. Similarly we should remove the idea of 
`chartKey` which you use, just use id/we shouldn't over complicate with 
different words for the same thing.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on issue #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on issue #4791: apply new Dashboard builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#issuecomment-380619142
 
 
   @graceguo-supercat 
   
   1) the code history etc is a valid concern. I think we should still delete 
v1, but move files with `git mv ...` so that it preserves history etc. 
otherwise we won't be sure we've deleted all unused code esp css.
   
   2) thanks for this explanation. next step is to clean it up / align on what 
it should be going forward! I think this would be an improvement:
   
   ```
   datasources
   toastMessages
   chartMetadata
   chartData
   dashboardLayout
   dashboardMetadata
   title
   standalone_mode
   slug
   metadata
   id
   edit_perm  
   save_perm
   sliceIds => (delete this?)
   dashboardState
   filters
   userId
   editMode
   hasUnsavedChanges
   showBuilderPane
   common
   ```
   
   **Explicit changes**
   - `allSlices` => `chartMetadata` (could consider `componentMetadata`, in the 
future we will have more than just "slices"?)
   - `charts` => `chartData`
   - `state.dashboard.dashboard` => separate it out to 
`state.dashboardMetadata`, it shouldn't be mixed with current UI state like 
filters/edit mode. `dashboard.dashboard` is not readable.
   - `state.dashboard.[rest]` => rename this state tree `dashboardState`

   I also don't want this point to be lost in the comments:
   We shouldn't set the id for `chartData` to `slice_[id]`, it should just be 
`id` and match `chartMetaData`. Similarly we should remove the idea of 
`chartKey` which you use, just use id/we shouldn't over complicate with 
different words for the same thing.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] codecov-io commented on issue #4793: [line] do not show 0 or nulls in rich tooltip

[GitBox]

codecov-io commented on issue #4793: [line] do not show 0 or nulls in rich 
tooltip
URL: 
https://github.com/apache/incubator-superset/pull/4793#issuecomment-379988834
 
 
   # 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4793?src=pr=h1)
 Report
   > Merging 
[#4793](https://codecov.io/gh/apache/incubator-superset/pull/4793?src=pr=desc)
 into 
[master](https://codecov.io/gh/apache/incubator-superset/commit/dadc0574b84ccf047987766ddc2a4c7efff638eb?src=pr=desc)
 will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree 
graph](https://codecov.io/gh/apache/incubator-superset/pull/4793/graphs/tree.svg?src=pr=650=KsB0fHcx6l=150)](https://codecov.io/gh/apache/incubator-superset/pull/4793?src=pr=tree)
   
   ```diff
   @@   Coverage Diff   @@
   ##   master#4793   +/-   ##
   ===
 Coverage   72.38%   72.38%   
   ===
 Files 208  208   
 Lines   1552615526   
 Branches 1203 1203   
   ===
 Hits1123811238   
 Misses   4285 4285   
 Partials33
   ```
   
   
   
   --
   
   [Continue to review full report at 
Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4793?src=pr=continue).
   > **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute  (impact)`, `ø = not affected`, `? = missing data`
   > Powered by 
[Codecov](https://codecov.io/gh/apache/incubator-superset/pull/4793?src=pr=footer).
 Last update 
[dadc057...9365500](https://codecov.io/gh/apache/incubator-superset/pull/4793?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] graceguo-supercat commented on issue #4791: apply new Dashboard builder to dashboard

[GitBox]

graceguo-supercat commented on issue #4791: apply new Dashboard builder to 
dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#issuecomment-380604576
 
 
   thanks for your comment. here is my thoughts:
   
   1. I don't like the idea of delete v1, it will destroy code history and make 
huge duplicates, huge merge conflicts, and very hard to track the changing or 
fixing of existed features. 
   
   Dashboard v2 is a new project, but most of v2's functionalities are same as 
v1, like filters, chart render, refresh and immune etc. We will add new 
functions/components to improve the layout and organization of Dashboard, not 
switch to branch new project.
   
   2. Naming: 
   
   `charts` are dynamic queries. Each of chart is basically a query based on 
saved params + dashboard added filter. It's attributes are mostly are dynamic, 
like request params, response data, start time, last updated time etc.
   
   `allSlices` mapped to `slices` table in backend. It has static data for 
those saved queries (after save query as slice), like name, owner, datasource 
name, timestamp, param list, etc. These data are used to help user review, 
filter, sort all existed slices, then they can add new slices into dashboard. 
Different than chart data that is fetched one by one, available slices data 
should be fetched all together and only once.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] hughhhh commented on issue #4809: Filter recently viewed to just have explore and dashboard types

[GitBox]

hug commented on issue #4809: Filter recently viewed to just have explore 
and dashboard types
URL: 
https://github.com/apache/incubator-superset/pull/4809#issuecomment-380609167
 
 
   Dupe #4808 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch commented on issue #4801: [bugfix] dedup groupby columns in Deck visualizations

[GitBox]

mistercrunch commented on issue #4801: [bugfix] dedup groupby columns in Deck 
visualizations
URL: 
https://github.com/apache/incubator-superset/pull/4801#issuecomment-380613068
 
 
   Didn't want to risk it, the set and list api are not the exact same, not 
sure if we handle the conversion on `json.dump` for instance. Didn't want to 
risk it...


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch commented on issue #4807: Make the bottom margin a bit taller

[GitBox]

mistercrunch commented on issue #4807: Make the bottom margin a bit taller
URL: 
https://github.com/apache/incubator-superset/pull/4807#issuecomment-380613354
 
 
   Yeah I had validated the changes mostly in the explore view which has it's 
own padding, so I missed how close to the edge is was.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] GabeLoins commented on a change in pull request #4736: [Explore] Adding custom expressions to adhoc metrics

[GitBox]

GabeLoins commented on a change in pull request #4736: [Explore] Adding custom 
expressions to adhoc metrics
URL: 
https://github.com/apache/incubator-superset/pull/4736#discussion_r180903978
 
 

 ##
 File path: 
superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx
 ##
 @@ -40,6 +59,22 @@ export default class AdhocMetricEditPopover extends 
React.Component {
   clearable: true,
   selectWrap: VirtualizedSelect,
 };
+if (langTools) {
+  const words = sqlWords.concat(this.props.columns.map(column => (
+{ name: column.column_name, value: column.column_name, score: 50, 
meta: 'column' }
+  )));
+  const completer = {
+getCompletions: (aceEditor, session, pos, prefix, callback) => {
+  callback(null, words);
+},
+  };
+  langTools.setCompleters([completer]);
+}
+document.addEventListener('mouseup', this.onMouseUp);
+  }
+
+  componentWillUnmount() {
+document.removeEventListener('mouseup', this.onMouseUp);
 
 Review comment:
   oooh good call


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] betodealmeida commented on a change in pull request #4798: [DeckGL] Added fixtures and Deck test

[GitBox]

betodealmeida commented on a change in pull request #4798: [DeckGL] Added 
fixtures and Deck test
URL: 
https://github.com/apache/incubator-superset/pull/4798#discussion_r180908278
 
 

 ##
 File path: tests/utils.py
 ##
 @@ -0,0 +1,14 @@
+# -*- coding: utf-8 -*-
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+from __future__ import unicode_literals
+
+import json
+
+FIXTURES_DIR = 'tests/fixtures'
 
 Review comment:
   This is a bit fragile, and might fail depending on from which directory you 
run the unit tests. There's a more robust way of handling this:
   
   1. Make sure the json files are in `MANIFEST.in`.
   2. Make sure `setup.py` has the line `include_package_data=True` (it has).
   3. You can now read the json content from using 
`pkg_resources.resource_filename` **no matter where it was installed**.
   
   For 1 & 2, see [this 
reference](http://python-packaging.readthedocs.io/en/latest/non-code-files.html).
 For 3, see [this 
reference](http://peak.telecommunity.com/DevCenter/PythonEggs#accessing-package-resources).


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] graceguo-supercat commented on issue #4791: apply new Dashboard builder to dashboard

[GitBox]

graceguo-supercat commented on issue #4791: apply new Dashboard builder to 
dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#issuecomment-380604576
 
 
   thanks for your comment. here are my thoughts:
   
   1. I don't like the idea of delete v1, it will destroy code history and make 
huge duplicates, huge merge conflicts, and very hard to track the changing or 
fixing of existed features. 
   
   Dashboard v2 will provide a lot of new features new components, but many of 
existed functionalities will be carried over, like filters, chart render, 
refresh and immune etc. We will add new functions/components to improve the 
layout and organization of Dashboard, not switch to branch new project.
   
   2. Naming: I don't have good idea about redux state keys. Please feel free 
to recommend.
   
   `charts` are dynamic queries. Each of chart is basically a query based on 
saved params + dashboard added filter. Most of its attributes are dynamic, like 
request params, response data, start time, last updated time etc.
   
   `allSlices` mapped to `slices` table in backend. They are static data for 
those saved queries (after save query as slice), like name, owner, datasource 
name, timestamp, param list, etc. These data are used to help user review, 
filter, sort all existed slices, then they can add new slices into dashboard. 
Different than chart data that is fetched one by one, available slices data 
should be fetched all together and only once.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on issue #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on issue #4791: apply new Dashboard builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#issuecomment-380619142
 
 
   @graceguo-supercat 
   
   1) the code history etc is a valid concern. I think we should still delete 
v1, but move files with `git mv ...` so that it preserves history etc. 
otherwise we won't be sure we've deleted all unused code esp css.
   
   2) thanks for this explanation. next step is to clean it up / align on what 
it should be going forward! I think this would be an improvement:
   
   ```
   datasources
   chartMetadata
   chartData
   dashboardLayout
   dashboardMetadata
   title
   standalone_mode
   slug
   metadata
   id
   edit_perm  
   save_perm
   sliceIds => (delete this?)
   dashboardState
   filters
   userId
   editMode
   hasUnsavedChanges
   showBuilderPane
   common
   toastMessages
   ```
   
   **Explicit changes**
   - `allSlices` => `chartMetadata` (could consider `componentMetadata`, in the 
future we will have more than just "slices"?)
   - `charts` => `chartData` (could consider `chartQueryData`)
   - `state.dashboard.dashboard` => separate it out to 
`state.dashboardMetadata`, it shouldn't be mixed with current UI state like 
filters/edit mode. `dashboard.dashboard` is not readable.
   - `state.dashboard.[rest]` => rename this state tree `dashboardState`

   I also don't want this point to be lost in the comments:
   We shouldn't set the id for `chartData` to `slice_[id]`, it should just be 
`id` and match `chartMetaData`. Similarly we should remove the idea of 
`chartKey` which you use, just use id/we shouldn't over complicate with 
different words for the same thing.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch closed pull request #4804: [upload csv to hive] fix call to unicodereader.next()

[GitBox]

mistercrunch closed pull request #4804: [upload csv to hive] fix call to 
unicodereader.next()
URL: https://github.com/apache/incubator-superset/pull/4804
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/superset/db_engine_specs.py b/superset/db_engine_specs.py
index 85ff9dbb80..d239f899a8 100644
--- a/superset/db_engine_specs.py
+++ b/superset/db_engine_specs.py
@@ -869,7 +869,7 @@ def create_table_from_csv(form, table):
 """Uploads a csv file and creates a superset datasource in Hive."""
 def get_column_names(filepath):
 with open(filepath, 'rb') as f:
-return unicodecsv.reader(f, encoding='utf-8-sig').next()
+return next(unicodecsv.reader(f, encoding='utf-8-sig'))
 
 table_name = form.name.data
 filename = form.csv_file.data.filename


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180884402
 
 

 ##
 File path: superset/assets/javascripts/dashboard/reducers/index.js
 ##
 @@ -0,0 +1,116 @@
+import { combineReducers } from 'redux';
+import shortid from 'shortid';
+
+import charts, { chart } from '../../chart/chartReducer';
+import dashboardReducer from './dashboard';
+import datasourcesReducer from './datasources';
+import allSlices, { initAllSlices } from './allSlices';
+import layoutReducer from '../v2/reducers/index';
+import messageToasts from '../v2/reducers/messageToasts';
+import { getParam } from '../../modules/utils';
+import layoutConverter from '../util/dashboardLayoutConverter';
+import { applyDefaultFormData } from '../../explore/stores/store';
+import { getColorFromScheme } from '../../modules/colors';
+
+export function getInitialState(bootstrapData) {
+  const { user_id, datasources, common } = bootstrapData;
+  delete common.locale;
+  delete common.language_pack;
+
+  const dashboard = { ...bootstrapData.dashboard_data };
+  let filters = {};
+  try {
+// allow request parameter overwrite dashboard metadata
+filters = JSON.parse(getParam('preselect_filters') || 
dashboard.metadata.default_filters);
+  } catch (e) {
+//
+  }
+
+  // Priming the color palette with user's label-color mapping provided in
+  // the dashboard's JSON metadata
+  if (dashboard.metadata && dashboard.metadata.label_colors) {
+const colorMap = dashboard.metadata.label_colors;
+for (const label in colorMap) {
+  getColorFromScheme(label, null, colorMap[label]);
+}
+  }
+
+  // dashboard layout
+  const position_json = dashboard.position_json;
+  let layout;
+  if (!position_json || !position_json['DASHBOARD_ROOT_ID']) {
+layout = layoutConverter(dashboard);
+  } else {
+layout = position_json;
+  }
+
+  const dashboardLayout = {
+past: [],
+present: layout,
+future: [],
+  };
+  delete dashboard.position_json;
+  delete dashboard.css;
+
+  // charts and slices
+  const initCharts = {};
+  const slices = {};
+  dashboard.slices.forEach((slice) => {
+const chartKey = 'slice_' + slice.slice_id;
+initCharts[chartKey] = { ...chart,
+  chartKey,
 
 Review comment:
   can we just delete this? why not use `slice_id`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] betodealmeida commented on a change in pull request #4800: Improve the calendar heatmap

[GitBox]

betodealmeida commented on a change in pull request #4800: Improve the calendar 
heatmap
URL: 
https://github.com/apache/incubator-superset/pull/4800#discussion_r180903916
 
 

 ##
 File path: superset/assets/javascripts/modules/colors.js
 ##
 @@ -122,6 +122,42 @@ export const spectrums = {
 '#FAFAFA',
 '#66',
   ],
+  greens: [
+'#cc',
+'#78c679',
+'#006837',
+  ],
+  purples: [
+'#f2f0f7',
+'#9e9ac8',
+'#54278f',
+  ],
+  oranges: [
+'#fef0d9',
+'#fc8d59',
+'#b3',
+  ],
+  red_yellow_blue: [
+'#d7191c',
+'#fdae61',
+'#bf',
+'#abd9e9',
+'#2c7bb6',
+  ],
+  brown_white_green: [
+'#a6611a',
+'#dfc27d',
+'#f5f5f5',
+'#80cdc1',
+'#018571',
+  ],
+  purple_white_green: [
 
 Review comment:
   Cool, #TIL.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch closed pull request #4133: BugFix(#3658)

[GitBox]

mistercrunch closed pull request #4133: BugFix(#3658)
URL: https://github.com/apache/incubator-superset/pull/4133
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/superset/connectors/sqla/views.py 
b/superset/connectors/sqla/views.py
index 12f9d6d555..591fcc1475 100644
--- a/superset/connectors/sqla/views.py
+++ b/superset/connectors/sqla/views.py
@@ -238,7 +238,9 @@ def pre_add(self, table):
 get_datasource_exist_error_mgs(table.full_name))
 
 # Fail before adding if the table can't be found
-if not table.database.has_table(table):
+try:
+table.get_sqla_table_object()
+except Exception:
 raise Exception(_(
 'Table [{}] could not be found, '
 'please double check your '


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch commented on issue #4659: Adding tests for the time table viz

[GitBox]

mistercrunch commented on issue #4659: Adding tests for the time table viz
URL: 
https://github.com/apache/incubator-superset/pull/4659#issuecomment-380584663
 
 
   I think for committers it's preferable for them to merge their own PRs as 
they might know things others don't or may want to merge their multiple PR in a 
certain order. @michellethomas do you have `write` access to the repo yet?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180879257
 
 

 ##
 File path: superset/assets/javascripts/dashboard/index.jsx
 ##
 @@ -8,36 +8,17 @@ import { initEnhancer } from '../reduxUtils';
 import { appSetup } from '../common';
 
 Review comment:
   should be moved to `v2/`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180879319
 
 

 ##
 File path: superset/assets/javascripts/dashboard/index.jsx
 ##
 @@ -8,36 +8,17 @@ import { initEnhancer } from '../reduxUtils';
 import { appSetup } from '../common';
 import { initJQueryAjax } from '../modules/utils';
 import DashboardContainer from './components/DashboardContainer';
-// import rootReducer, { getInitialState } from './reducers';
-
-import emptyDashboardLayout from './v2/fixtures/emptyDashboardLayout';
-import rootReducer from './v2/reducers/';
+import rootReducer, { getInitialState } from './reducers/index';
 
 appSetup();
 initJQueryAjax();
 
 const appContainer = document.getElementById('app');
-// const bootstrapData = 
JSON.parse(appContainer.getAttribute('data-bootstrap'));
-// const initState = Object.assign({}, getInitialState(bootstrapData));
-
-const initState = {
-  dashboardLayout: {
-past: [],
-present: emptyDashboardLayout,
-future: [],
-  },
-  editMode: true,
-  messageToasts: [],
-};
+const bootstrapData = JSON.parse(appContainer.getAttribute('data-bootstrap'));
+const initState = Object.assign({}, getInitialState(bootstrapData));
 
 Review comment:
   please use object rest spread


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180872812
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/Dashboard.jsx
 ##
 @@ -87,12 +77,26 @@ class Dashboard extends React.PureComponent {
 
   componentWillReceiveProps(nextProps) {
 if (this.firstLoad &&
-  Object.values(nextProps.slices)
-.every(slice => (['rendered', 'failed', 
'stopped'].indexOf(slice.chartStatus) > -1))
+  Object.values(nextProps.charts)
+.every(chart => (['rendered', 'failed', 
'stopped'].indexOf(chart.chartStatus) > -1))
 ) {
   Logger.end(this.loadingLog);
   this.firstLoad = false;
 }
+
+const currentCharts = this.getChartKeysFromLayout(this.props.layout);
 
 Review comment:
   why is this logic in the Component? can you move this to the relevant 
actions? 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180876462
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/SliceAdder.jsx
 ##
 @@ -46,174 +59,157 @@ class SliceAdder extends React.Component {
 }
   }
 
-  onEnterModal() {
-const uri = 
`/sliceaddview/api/read?_flt_0_created_by=${this.props.userId}`;
-this.slicesRequest = $.ajax({
-  url: uri,
-  type: 'GET',
-  success: (response) => {
-// Prepare slice data for table
-const slices = response.result.map(slice => ({
-  id: slice.id,
-  sliceName: slice.slice_name,
-  vizType: slice.viz_type,
-  datasourceLink: slice.datasource_link,
-  modified: slice.modified,
-}));
-
-this.setState({
-  slices,
-  selectionMap: {},
-  slicesLoaded: true,
-});
-  },
-  error: (error) => {
-this.errored = true;
-this.setState({
-  errorMsg: t('Sorry, there was an error fetching slices to this 
dashboard: ') +
-  this.getAjaxErrorMsg(error),
-});
-  },
-});
+  componentWillReceiveProps(nextProps) {
+if (nextProps.lastUpdated !== this.props.lastUpdated) {
+  this.setState({
+filteredSlices: Object.values(nextProps.slices)
+  .filter(createFilter(this.state.searchTerm, KEYS_TO_FILTERS))
+  .sort(this.sortByComparator(KEYS_TO_SORT[this.state.sortBy].key)),
+  });
+}
   }
 
-  getAjaxErrorMsg(error) {
-const respJSON = error.responseJSON;
-return (respJSON && respJSON.message) ? respJSON.message :
-  error.responseText;
+  sortByComparator(attr) {
+const desc = 'changedOn' === attr ? -1 : 1;
+
+return (a, b) => {
+  if (a[attr] < b[attr]) {
+return -1 * desc;
+  } else if (a[attr] > b[attr]) {
+return 1 * desc;
+  } else {
+return 0;
+  }
+};
   }
 
-  addSlices() {
-const adder = this;
-this.props.addSlicesToDashboard(Object.keys(this.state.selectionMap))
-  // if successful, page will be reloaded.
-  .fail((error) => {
-adder.errored = true;
-adder.setState({
-  errorMsg: t('Sorry, there was an error adding slices to this 
dashboard: ') +
-  this.getAjaxErrorMsg(error),
-});
-  });
+  handleKeyPress(ev) {
+if (ev.key === 'Enter') {
+  ev.preventDefault();
+
+  this.searchUpdated(ev.target.value);
+}
   }
 
-  toggleSlice(slice) {
-const selectionMap = Object.assign({}, this.state.selectionMap);
-selectionMap[slice.id] = !selectionMap[slice.id];
-this.setState({ selectionMap });
+  getFilteredSortedSlices(searchTerm, sortBy) {
+return Object.values(this.props.slices)
+  .filter(createFilter(searchTerm, KEYS_TO_FILTERS))
+  .sort(this.sortByComparator(KEYS_TO_SORT[sortBy].key));
   }
 
-  modifiedDateComparator(a, b, order) {
-if (order === 'desc') {
-  if (a.changed_on > b.changed_on) {
-return -1;
-  } else if (a.changed_on < b.changed_on) {
-return 1;
-  }
-  return 0;
-}
+  searchUpdated(searchTerm) {
+this.setState({
+  searchTerm,
+  filteredSlices: this.getFilteredSortedSlices(searchTerm, 
this.state.sortBy),
+});
+  }
 
-if (a.changed_on < b.changed_on) {
-  return -1;
-} else if (a.changed_on > b.changed_on) {
-  return 1;
-}
-return 0;
+  handleSelect(sortBy) {
+this.setState({
+  sortBy,
+  filteredSlices: this.getFilteredSortedSlices(this.state.searchTerm, 
sortBy),
+})
   }
 
-  render() {
-const hideLoad = this.state.slicesLoaded || this.errored;
-let enableAddSlice = this.state.selectionMap && 
Object.keys(this.state.selectionMap);
-if (enableAddSlice) {
-  enableAddSlice = enableAddSlice.some(function (key) {
-return this.state.selectionMap[key];
-  }, this);
-}
-const modalContent = (
-  
-
-
-  {this.state.errorMsg}
+  rowRenderer({ key, index, style }) {
+const cellData = this.state.filteredSlices[index];
+const duration = cellData.modified ? cellData.modified.replace(/<[^>]*>/g, 
'') : '';
+const isSelected = this.props.selectedSliceIds.has(cellData.slice_id);
+const type = CHART_TYPE;
+const id = NEW_CHART_ID;
+const meta = {
+  chartKey: 'slice_' + cellData.slice_id,
 
 Review comment:
   this is fragile. it'd be better to define a util function that made sure 
this was define only one way always


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git 

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180874993
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/GridLayout.jsx
 ##
 @@ -1,33 +1,29 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import { Responsive, WidthProvider } from 'react-grid-layout';
+import cx from 'classnames';
 
 import GridCell from './GridCell';
-
-require('react-grid-layout/css/styles.css');
-require('react-resizable/css/styles.css');
-
-const ResponsiveReactGridLayout = WidthProvider(Responsive);
+import { slicePropShape } from '../reducers/propShapes';
+import DashboardBuilder from '../v2/containers/DashboardBuilder';
 
 const propTypes = {
   dashboard: PropTypes.object.isRequired,
+  layout: PropTypes.object.isRequired,
   datasources: PropTypes.object,
   charts: PropTypes.object.isRequired,
 
 Review comment:
   should charts have a shape?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180881204
 
 

 ##
 File path: superset/assets/javascripts/dashboard/reducers/dashboard.js
 ##
 @@ -0,0 +1,123 @@
+import d3 from 'd3';
 
 Review comment:
   move to `v2/reducers`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mistercrunch commented on issue #4612: In the “SQL Lab”, the “Results” section shows “Pending” constantly

[GitBox]

mistercrunch commented on issue #4612: In the “SQL Lab”, the “Results” section 
shows “Pending” constantly
URL: 
https://github.com/apache/incubator-superset/issues/4612#issuecomment-380587655
 
 
   Also make sure the port is open, I usually check using the `telnet {host} 
{port}` command


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180869980
 
 

 ##
 File path: superset/assets/javascripts/dashboard/actions/allSlices.js
 ##
 @@ -0,0 +1,66 @@
+/* global notify */
 
 Review comment:
   what does `allSlices` mean? can we give this a more readable name? 
`chartData`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] michellethomas commented on issue #4659: Adding tests for the time table viz

[GitBox]

michellethomas commented on issue #4659: Adding tests for the time table viz
URL: 
https://github.com/apache/incubator-superset/pull/4659#issuecomment-380594393
 
 
   I don't have access, I'm not a committer yet.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4736: [Explore] Adding custom expressions to adhoc metrics

[GitBox]

williaster commented on a change in pull request #4736: [Explore] Adding custom 
expressions to adhoc metrics
URL: 
https://github.com/apache/incubator-superset/pull/4736#discussion_r180897757
 
 

 ##
 File path: 
superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx
 ##
 @@ -40,6 +59,22 @@ export default class AdhocMetricEditPopover extends 
React.Component {
   clearable: true,
   selectWrap: VirtualizedSelect,
 };
+if (langTools) {
+  const words = sqlWords.concat(this.props.columns.map(column => (
+{ name: column.column_name, value: column.column_name, score: 50, 
meta: 'column' }
+  )));
+  const completer = {
+getCompletions: (aceEditor, session, pos, prefix, callback) => {
+  callback(null, words);
+},
+  };
+  langTools.setCompleters([completer]);
+}
+document.addEventListener('mouseup', this.onMouseUp);
+  }
+
+  componentWillUnmount() {
+document.removeEventListener('mouseup', this.onMouseUp);
 
 Review comment:
   unlikely sitch, but could also call 
`document.removeEventListener('mousemove', this.onMouseMove);` to be safe


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4736: [Explore] Adding custom expressions to adhoc metrics

[GitBox]

williaster commented on a change in pull request #4736: [Explore] Adding custom 
expressions to adhoc metrics
URL: 
https://github.com/apache/incubator-superset/pull/4736#discussion_r180897336
 
 

 ##
 File path: 
superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx
 ##
 @@ -48,16 +83,25 @@ export default class AdhocMetricEditPopover extends 
React.Component {
   }
 
   onColumnChange(column) {
-this.setState({ adhocMetric: this.state.adhocMetric.duplicateWith({ column 
}) });
+this.setState({ adhocMetric: this.state.adhocMetric.duplicateWith({
+  column,
+  expressionType: EXPRESSION_TYPES.SIMPLE,
+}) });
   }
 
   onAggregateChange(aggregate) {
 // we construct this object explicitly to overwrite the value in the case 
aggregate is null
-this.setState({
-  adhocMetric: this.state.adhocMetric.duplicateWith({
-aggregate: aggregate && aggregate.aggregate,
-  }),
-});
+this.setState({ adhocMetric: this.state.adhocMetric.duplicateWith({
 
 Review comment:
   nit -- I would newline `adhocMetric`, wish we had better lint rules enabled! 
  


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4736: [Explore] Adding custom expressions to adhoc metrics

[GitBox]

williaster commented on a change in pull request #4736: [Explore] Adding custom 
expressions to adhoc metrics
URL: 
https://github.com/apache/incubator-superset/pull/4736#discussion_r180898894
 
 

 ##
 File path: superset/assets/javascripts/explore/components/AdhocMetricOption.jsx
 ##
 @@ -18,6 +18,11 @@ export default class AdhocMetricOption extends 
React.PureComponent {
   constructor(props) {
 super(props);
 this.closeMetricEditOverlay = this.closeMetricEditOverlay.bind(this);
+this.onPopoverResize = this.onPopoverResize.bind(this);
+  }
+
+  onPopoverResize() {
+   this.forceUpdate();
 
 Review comment:
   nit indent


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4736: [Explore] Adding custom expressions to adhoc metrics

[GitBox]

williaster commented on a change in pull request #4736: [Explore] Adding custom 
expressions to adhoc metrics
URL: 
https://github.com/apache/incubator-superset/pull/4736#discussion_r180899633
 
 

 ##
 File path: superset/assets/spec/javascripts/explore/AdhocMetric_spec.js
 ##
 @@ -82,4 +95,95 @@ describe('AdhocMetric', () => {
 
 expect(adhocMetric2.label).to.equal('label1');
   });
+
+  it('can determines if it is valid', () => {
 
 Review comment:
   nit remove `determines` 's'


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4736: [Explore] Adding custom expressions to adhoc metrics

[GitBox]

williaster commented on a change in pull request #4736: [Explore] Adding custom 
expressions to adhoc metrics
URL: 
https://github.com/apache/incubator-superset/pull/4736#discussion_r180899096
 
 

 ##
 File path: superset/assets/javascripts/explore/components/AdhocMetricOption.jsx
 ##
 @@ -44,6 +50,7 @@ export default class AdhocMetricOption extends 
React.PureComponent {
 disabled
 overlay={overlay}
 rootClose
+shouldUpdatePosition
 
 Review comment:
    thanks again for adding this!


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4736: [Explore] Adding custom expressions to adhoc metrics

[GitBox]

williaster commented on a change in pull request #4736: [Explore] Adding custom 
expressions to adhoc metrics
URL: 
https://github.com/apache/incubator-superset/pull/4736#discussion_r180896732
 
 

 ##
 File path: superset/assets/javascripts/explore/AdhocMetric.js
 ##
 @@ -23,10 +68,29 @@ export default class AdhocMetric {
 
   equals(adhocMetric) {
 return adhocMetric.label === this.label &&
+  adhocMetric.expressionType === this.expressionType &&
+  adhocMetric.sqlExpression === this.sqlExpression &&
   adhocMetric.aggregate === this.aggregate &&
   (
 (adhocMetric.column && adhocMetric.column.column_name) ===
 (this.column && this.column.column_name)
   );
   }
+
+  isValid() {
+if (this.expressionType === EXPRESSION_TYPES.SIMPLE) {
+  return !!(this.column && this.aggregate);
 
 Review comment:
   `Boolean()` also works fyi


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4736: [Explore] Adding custom expressions to adhoc metrics

[GitBox]

williaster commented on a change in pull request #4736: [Explore] Adding custom 
expressions to adhoc metrics
URL: 
https://github.com/apache/incubator-superset/pull/4736#discussion_r180898341
 
 

 ##
 File path: 
superset/assets/javascripts/explore/components/AdhocMetricEditPopover.jsx
 ##
 @@ -101,38 +176,68 @@ export default class AdhocMetricEditPopover extends 
React.Component {
 
 const popoverTitle = (
   
 );
 
-const stateIsValid = this.state.adhocMetric.column && 
this.state.adhocMetric.aggregate;
-const hasUnsavedChanges = 
this.state.adhocMetric.equals(this.props.adhocMetric);
+const stateIsValid = adhocMetric.isValid();
+const hasUnsavedChanges = adhocMetric.equals(propsAdhocMetric);
 
 Review comment:
   is this backwards? or I'm reading it wrong? I guess I'd expect equals to 
return true if they're the same which would mean no unsaved changes?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] hughhhh opened a new pull request #4808: [Welcome] Filter recently viewed to just have explore and dashboard types

[GitBox]

hug opened a new pull request #4808: [Welcome] Filter recently viewed to 
just have explore and dashboard types
URL: https://github.com/apache/incubator-superset/pull/4808
 
 
   Filter recently viewed to only show recent explore and dashboard actions
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180889255
 
 

 ##
 File path: superset/assets/javascripts/dashboard/v2/reducers/index.js
 ##
 @@ -1,17 +1,10 @@
-import { combineReducers } from 'redux';
 import undoable, { distinctState } from 'redux-undo';
 
 import dashboardLayout from './dashboardLayout';
-import editMode from './editMode';
-import messageToasts from './messageToasts';
 
-const undoableLayout = undoable(dashboardLayout, {
+export const undoableLayout = undoable(dashboardLayout, {
 
 Review comment:
   why do you need a default and named export?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180876633
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/SliceAdder.jsx
 ##
 @@ -46,174 +59,157 @@ class SliceAdder extends React.Component {
 }
   }
 
-  onEnterModal() {
-const uri = 
`/sliceaddview/api/read?_flt_0_created_by=${this.props.userId}`;
-this.slicesRequest = $.ajax({
-  url: uri,
-  type: 'GET',
-  success: (response) => {
-// Prepare slice data for table
-const slices = response.result.map(slice => ({
-  id: slice.id,
-  sliceName: slice.slice_name,
-  vizType: slice.viz_type,
-  datasourceLink: slice.datasource_link,
-  modified: slice.modified,
-}));
-
-this.setState({
-  slices,
-  selectionMap: {},
-  slicesLoaded: true,
-});
-  },
-  error: (error) => {
-this.errored = true;
-this.setState({
-  errorMsg: t('Sorry, there was an error fetching slices to this 
dashboard: ') +
-  this.getAjaxErrorMsg(error),
-});
-  },
-});
+  componentWillReceiveProps(nextProps) {
+if (nextProps.lastUpdated !== this.props.lastUpdated) {
+  this.setState({
+filteredSlices: Object.values(nextProps.slices)
+  .filter(createFilter(this.state.searchTerm, KEYS_TO_FILTERS))
+  .sort(this.sortByComparator(KEYS_TO_SORT[this.state.sortBy].key)),
+  });
+}
   }
 
-  getAjaxErrorMsg(error) {
-const respJSON = error.responseJSON;
-return (respJSON && respJSON.message) ? respJSON.message :
-  error.responseText;
+  sortByComparator(attr) {
+const desc = 'changedOn' === attr ? -1 : 1;
+
+return (a, b) => {
+  if (a[attr] < b[attr]) {
+return -1 * desc;
+  } else if (a[attr] > b[attr]) {
+return 1 * desc;
+  } else {
+return 0;
+  }
+};
   }
 
-  addSlices() {
-const adder = this;
-this.props.addSlicesToDashboard(Object.keys(this.state.selectionMap))
-  // if successful, page will be reloaded.
-  .fail((error) => {
-adder.errored = true;
-adder.setState({
-  errorMsg: t('Sorry, there was an error adding slices to this 
dashboard: ') +
-  this.getAjaxErrorMsg(error),
-});
-  });
+  handleKeyPress(ev) {
+if (ev.key === 'Enter') {
+  ev.preventDefault();
+
+  this.searchUpdated(ev.target.value);
+}
   }
 
-  toggleSlice(slice) {
-const selectionMap = Object.assign({}, this.state.selectionMap);
-selectionMap[slice.id] = !selectionMap[slice.id];
-this.setState({ selectionMap });
+  getFilteredSortedSlices(searchTerm, sortBy) {
+return Object.values(this.props.slices)
+  .filter(createFilter(searchTerm, KEYS_TO_FILTERS))
+  .sort(this.sortByComparator(KEYS_TO_SORT[sortBy].key));
   }
 
-  modifiedDateComparator(a, b, order) {
-if (order === 'desc') {
-  if (a.changed_on > b.changed_on) {
-return -1;
-  } else if (a.changed_on < b.changed_on) {
-return 1;
-  }
-  return 0;
-}
+  searchUpdated(searchTerm) {
+this.setState({
+  searchTerm,
+  filteredSlices: this.getFilteredSortedSlices(searchTerm, 
this.state.sortBy),
+});
+  }
 
-if (a.changed_on < b.changed_on) {
-  return -1;
-} else if (a.changed_on > b.changed_on) {
-  return 1;
-}
-return 0;
+  handleSelect(sortBy) {
+this.setState({
+  sortBy,
+  filteredSlices: this.getFilteredSortedSlices(this.state.searchTerm, 
sortBy),
+})
   }
 
-  render() {
-const hideLoad = this.state.slicesLoaded || this.errored;
-let enableAddSlice = this.state.selectionMap && 
Object.keys(this.state.selectionMap);
-if (enableAddSlice) {
-  enableAddSlice = enableAddSlice.some(function (key) {
-return this.state.selectionMap[key];
-  }, this);
-}
-const modalContent = (
-  
-
-
-  {this.state.errorMsg}
+  rowRenderer({ key, index, style }) {
+const cellData = this.state.filteredSlices[index];
+const duration = cellData.modified ? cellData.modified.replace(/<[^>]*>/g, 
'') : '';
+const isSelected = this.props.selectedSliceIds.has(cellData.slice_id);
+const type = CHART_TYPE;
+const id = NEW_CHART_ID;
+const meta = {
+  chartKey: 'slice_' + cellData.slice_id,
+};
+
+return (
+  
+{({ dragSourceRef }) => (
+  
+
+  {cellData.slice_name}
+  
+
+  Modified: 
+  {duration}
+
+
+  Viz type: 
+  {cellData.viz_type}
+
+
+  Data source: 
+  
 
 Review comment:
   why are you setting html? why can this not just be a link?

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180883909
 
 

 ##
 File path: superset/assets/javascripts/dashboard/reducers/index.js
 ##
 @@ -0,0 +1,116 @@
+import { combineReducers } from 'redux';
+import shortid from 'shortid';
+
+import charts, { chart } from '../../chart/chartReducer';
+import dashboardReducer from './dashboard';
+import datasourcesReducer from './datasources';
+import allSlices, { initAllSlices } from './allSlices';
+import layoutReducer from '../v2/reducers/index';
+import messageToasts from '../v2/reducers/messageToasts';
+import { getParam } from '../../modules/utils';
+import layoutConverter from '../util/dashboardLayoutConverter';
+import { applyDefaultFormData } from '../../explore/stores/store';
+import { getColorFromScheme } from '../../modules/colors';
+
+export function getInitialState(bootstrapData) {
 
 Review comment:
   as mentioned above this should be moved to its own file like 
`v2/reducers/getInitialState`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180880502
 
 

 ##
 File path: superset/assets/javascripts/dashboard/reducers/allSlices.js
 ##
 @@ -0,0 +1,74 @@
+import * as actions from '../actions/allSlices';
+import { t } from '../../locales';
+
+export const initAllSlices = {
+  slices: {},
+  isLoading: true,
+  errorMsg: null,
+  lastUpdated: 0,
+};
+
+export default function allSlicesReducer(state = initAllSlices, action) {
+  const actionHandlers = {
+[actions.UPDATE_SLICE_NAME]() {
+  const updatedSlice = {
+...state.slices[action.key],
+slice_name: action.sliceName,
+  };
+  const updatedSlices = {
+...state.slices,
+[action.key]: updatedSlice,
+  };
+  return { ...state, slices: updatedSlices };
+},
+[actions.FETCH_ALL_SLICES_STARTED]() {
+  return {
+...state,
+isLoading: true,
+  }
+},
+[actions.SET_ALL_SLICES]() {
+  let slices = { ...state.slices };
 
 Review comment:
   this doesn't need to be a `let`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180876866
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/SliceAdder.jsx
 ##
 @@ -46,174 +59,157 @@ class SliceAdder extends React.Component {
 }
   }
 
-  onEnterModal() {
-const uri = 
`/sliceaddview/api/read?_flt_0_created_by=${this.props.userId}`;
-this.slicesRequest = $.ajax({
-  url: uri,
-  type: 'GET',
-  success: (response) => {
-// Prepare slice data for table
-const slices = response.result.map(slice => ({
-  id: slice.id,
-  sliceName: slice.slice_name,
-  vizType: slice.viz_type,
-  datasourceLink: slice.datasource_link,
-  modified: slice.modified,
-}));
-
-this.setState({
-  slices,
-  selectionMap: {},
-  slicesLoaded: true,
-});
-  },
-  error: (error) => {
-this.errored = true;
-this.setState({
-  errorMsg: t('Sorry, there was an error fetching slices to this 
dashboard: ') +
-  this.getAjaxErrorMsg(error),
-});
-  },
-});
+  componentWillReceiveProps(nextProps) {
+if (nextProps.lastUpdated !== this.props.lastUpdated) {
+  this.setState({
+filteredSlices: Object.values(nextProps.slices)
+  .filter(createFilter(this.state.searchTerm, KEYS_TO_FILTERS))
+  .sort(this.sortByComparator(KEYS_TO_SORT[this.state.sortBy].key)),
+  });
+}
   }
 
-  getAjaxErrorMsg(error) {
-const respJSON = error.responseJSON;
-return (respJSON && respJSON.message) ? respJSON.message :
-  error.responseText;
+  sortByComparator(attr) {
+const desc = 'changedOn' === attr ? -1 : 1;
+
+return (a, b) => {
+  if (a[attr] < b[attr]) {
+return -1 * desc;
+  } else if (a[attr] > b[attr]) {
+return 1 * desc;
+  } else {
+return 0;
+  }
+};
   }
 
-  addSlices() {
-const adder = this;
-this.props.addSlicesToDashboard(Object.keys(this.state.selectionMap))
-  // if successful, page will be reloaded.
-  .fail((error) => {
-adder.errored = true;
-adder.setState({
-  errorMsg: t('Sorry, there was an error adding slices to this 
dashboard: ') +
-  this.getAjaxErrorMsg(error),
-});
-  });
+  handleKeyPress(ev) {
+if (ev.key === 'Enter') {
+  ev.preventDefault();
+
+  this.searchUpdated(ev.target.value);
+}
   }
 
-  toggleSlice(slice) {
-const selectionMap = Object.assign({}, this.state.selectionMap);
-selectionMap[slice.id] = !selectionMap[slice.id];
-this.setState({ selectionMap });
+  getFilteredSortedSlices(searchTerm, sortBy) {
+return Object.values(this.props.slices)
+  .filter(createFilter(searchTerm, KEYS_TO_FILTERS))
+  .sort(this.sortByComparator(KEYS_TO_SORT[sortBy].key));
   }
 
-  modifiedDateComparator(a, b, order) {
-if (order === 'desc') {
-  if (a.changed_on > b.changed_on) {
-return -1;
-  } else if (a.changed_on < b.changed_on) {
-return 1;
-  }
-  return 0;
-}
+  searchUpdated(searchTerm) {
+this.setState({
+  searchTerm,
+  filteredSlices: this.getFilteredSortedSlices(searchTerm, 
this.state.sortBy),
+});
+  }
 
-if (a.changed_on < b.changed_on) {
-  return -1;
-} else if (a.changed_on > b.changed_on) {
-  return 1;
-}
-return 0;
+  handleSelect(sortBy) {
+this.setState({
+  sortBy,
+  filteredSlices: this.getFilteredSortedSlices(this.state.searchTerm, 
sortBy),
+})
   }
 
-  render() {
-const hideLoad = this.state.slicesLoaded || this.errored;
-let enableAddSlice = this.state.selectionMap && 
Object.keys(this.state.selectionMap);
-if (enableAddSlice) {
-  enableAddSlice = enableAddSlice.some(function (key) {
-return this.state.selectionMap[key];
-  }, this);
-}
-const modalContent = (
-  
-
-
-  {this.state.errorMsg}
+  rowRenderer({ key, index, style }) {
+const cellData = this.state.filteredSlices[index];
+const duration = cellData.modified ? cellData.modified.replace(/<[^>]*>/g, 
'') : '';
+const isSelected = this.props.selectedSliceIds.has(cellData.slice_id);
+const type = CHART_TYPE;
+const id = NEW_CHART_ID;
+const meta = {
+  chartKey: 'slice_' + cellData.slice_id,
+};
+
+return (
+  
+{({ dragSourceRef }) => (
+  
+
+  {cellData.slice_name}
+  
+
+  Modified: 
+  {duration}
+
+
+  Viz type: 
 
 Review comment:
   Please use `visualization` not `viz`


This is an automated message from the Apache Git Service.
To respond to the message, please log on 

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180879991
 
 

 ##
 File path: superset/assets/javascripts/dashboard/reducers/allSlices.js
 ##
 @@ -0,0 +1,74 @@
+import * as actions from '../actions/allSlices';
 
 Review comment:
   please import individual actions, it is
   1) less error prone and
   2) can be linted in the future, `*` breaks linting.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180886368
 
 

 ##
 File path: 
superset/assets/javascripts/dashboard/v2/components/DashboardBuilder.jsx
 ##
 @@ -20,15 +20,18 @@ import {
 } from '../util/constants';
 
 const propTypes = {
+  cells: PropTypes.object.isRequired,
+
   // redux
   dashboardLayout: PropTypes.object.isRequired,
   deleteTopLevelTabs: PropTypes.func.isRequired,
   editMode: PropTypes.bool.isRequired,
+  showBuilderPane: PropTypes.bool,
   handleComponentDrop: PropTypes.func.isRequired,
 };
 
 const defaultProps = {
-  editMode: true,
+  showBuilderPane: false,
 
 Review comment:
   why not just make this required?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180871121
 
 

 ##
 File path: superset/assets/javascripts/chart/chartAction.js
 ##
 @@ -111,6 +111,17 @@ export function updateQueryFormData(value, key) {
   return { type: UPDATE_QUERY_FORM_DATA, value, key };
 }
 
+export const ADD_CHART = 'ADD_CHART';
 
 Review comment:
   what does `addChart` mean?  add to what? if dashboard can you rename to 
`addChartToDashboard`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180883434
 
 

 ##
 File path: superset/assets/javascripts/dashboard/reducers/datasources.js
 ##
 @@ -0,0 +1,17 @@
+import * as actions from '../actions/datasources';
+
+export default function datasourceReducer(datasources = {}, action) {
+  const actionHandlers = {
+[actions.SET_DATASOURCE]() {
+  return action.datasource;
+},
+  };
+
+  if (action.type in actionHandlers) {
+return {
+  ...datasources,
+  [action.key]: actionHandlers[action.type](datasources[action.key], 
action),
 
 Review comment:
   should add payload => `action.payload.key`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180874259
 
 

 ##
 File path: 
superset/assets/javascripts/dashboard/components/DashboardContainer.jsx
 ##
 @@ -1,28 +1,37 @@
 import { bindActionCreators } from 'redux';
 import { connect } from 'react-redux';
 
-import * as dashboardActions from '../actions';
+import * as dashboardActions from '../actions/dashboard';
+import { saveSliceName } from '../actions/allSlices';
 import * as chartActions from '../../chart/chartAction';
-import Dashboard from '../v2/components/Dashboard';
+import Dashboard from './Dashboard';
 
 Review comment:
   this should be in the original directory `'../v2/components/Dashboard'`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180877222
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/SliceAdder.jsx
 ##
 @@ -46,174 +59,157 @@ class SliceAdder extends React.Component {
 }
   }
 
-  onEnterModal() {
-const uri = 
`/sliceaddview/api/read?_flt_0_created_by=${this.props.userId}`;
-this.slicesRequest = $.ajax({
-  url: uri,
-  type: 'GET',
-  success: (response) => {
-// Prepare slice data for table
-const slices = response.result.map(slice => ({
-  id: slice.id,
-  sliceName: slice.slice_name,
-  vizType: slice.viz_type,
-  datasourceLink: slice.datasource_link,
-  modified: slice.modified,
-}));
-
-this.setState({
-  slices,
-  selectionMap: {},
-  slicesLoaded: true,
-});
-  },
-  error: (error) => {
-this.errored = true;
-this.setState({
-  errorMsg: t('Sorry, there was an error fetching slices to this 
dashboard: ') +
-  this.getAjaxErrorMsg(error),
-});
-  },
-});
+  componentWillReceiveProps(nextProps) {
+if (nextProps.lastUpdated !== this.props.lastUpdated) {
+  this.setState({
+filteredSlices: Object.values(nextProps.slices)
+  .filter(createFilter(this.state.searchTerm, KEYS_TO_FILTERS))
+  .sort(this.sortByComparator(KEYS_TO_SORT[this.state.sortBy].key)),
+  });
+}
   }
 
-  getAjaxErrorMsg(error) {
-const respJSON = error.responseJSON;
-return (respJSON && respJSON.message) ? respJSON.message :
-  error.responseText;
+  sortByComparator(attr) {
+const desc = 'changedOn' === attr ? -1 : 1;
+
+return (a, b) => {
+  if (a[attr] < b[attr]) {
+return -1 * desc;
+  } else if (a[attr] > b[attr]) {
+return 1 * desc;
+  } else {
+return 0;
+  }
+};
   }
 
-  addSlices() {
-const adder = this;
-this.props.addSlicesToDashboard(Object.keys(this.state.selectionMap))
-  // if successful, page will be reloaded.
-  .fail((error) => {
-adder.errored = true;
-adder.setState({
-  errorMsg: t('Sorry, there was an error adding slices to this 
dashboard: ') +
-  this.getAjaxErrorMsg(error),
-});
-  });
+  handleKeyPress(ev) {
+if (ev.key === 'Enter') {
+  ev.preventDefault();
+
+  this.searchUpdated(ev.target.value);
+}
   }
 
-  toggleSlice(slice) {
-const selectionMap = Object.assign({}, this.state.selectionMap);
-selectionMap[slice.id] = !selectionMap[slice.id];
-this.setState({ selectionMap });
+  getFilteredSortedSlices(searchTerm, sortBy) {
+return Object.values(this.props.slices)
+  .filter(createFilter(searchTerm, KEYS_TO_FILTERS))
+  .sort(this.sortByComparator(KEYS_TO_SORT[sortBy].key));
   }
 
-  modifiedDateComparator(a, b, order) {
-if (order === 'desc') {
-  if (a.changed_on > b.changed_on) {
-return -1;
-  } else if (a.changed_on < b.changed_on) {
-return 1;
-  }
-  return 0;
-}
+  searchUpdated(searchTerm) {
+this.setState({
+  searchTerm,
+  filteredSlices: this.getFilteredSortedSlices(searchTerm, 
this.state.sortBy),
+});
+  }
 
-if (a.changed_on < b.changed_on) {
-  return -1;
-} else if (a.changed_on > b.changed_on) {
-  return 1;
-}
-return 0;
+  handleSelect(sortBy) {
+this.setState({
+  sortBy,
+  filteredSlices: this.getFilteredSortedSlices(this.state.searchTerm, 
sortBy),
+})
   }
 
-  render() {
-const hideLoad = this.state.slicesLoaded || this.errored;
-let enableAddSlice = this.state.selectionMap && 
Object.keys(this.state.selectionMap);
-if (enableAddSlice) {
-  enableAddSlice = enableAddSlice.some(function (key) {
-return this.state.selectionMap[key];
-  }, this);
-}
-const modalContent = (
-  
-
-
-  {this.state.errorMsg}
+  rowRenderer({ key, index, style }) {
+const cellData = this.state.filteredSlices[index];
+const duration = cellData.modified ? cellData.modified.replace(/<[^>]*>/g, 
'') : '';
+const isSelected = this.props.selectedSliceIds.has(cellData.slice_id);
+const type = CHART_TYPE;
+const id = NEW_CHART_ID;
+const meta = {
+  chartKey: 'slice_' + cellData.slice_id,
+};
+
+return (
+  
+{({ dragSourceRef }) => (
+  
+
+  {cellData.slice_name}
+  
+
+  Modified: 
+  {duration}
+
+
+  Viz type: 
+  {cellData.viz_type}
+
+
+  Data source: 
+  
+
+  
 
-
-  
+)}
+  
+)
+  }
+
+  render() {
+return 

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180878276
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/SliceHeader.jsx
 ##
 @@ -1,17 +1,16 @@
 import React from 'react';
 
 Review comment:
   please move this to `v2/components/`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180875972
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/SliceAdder.jsx
 ##
 @@ -1,43 +1,56 @@
 import React from 'react';
-import $ from 'jquery';
 import PropTypes from 'prop-types';
-import { BootstrapTable, TableHeaderColumn } from 'react-bootstrap-table';
+import cx from 'classnames';
+import { DropdownButton, MenuItem } from 'react-bootstrap';
+import { List } from 'react-virtualized';
+import SearchInput, {createFilter} from 'react-search-input';
 
-import ModalTrigger from '../../components/ModalTrigger';
-import { t } from '../../locales';
-
-require('react-bootstrap-table/css/react-bootstrap-table.css');
+import { slicePropShape } from '../reducers/propShapes';
+import DragDroppable from '../v2/components/dnd/DragDroppable';
+import { CHART_TYPE, NEW_COMPONENT_SOURCE_TYPE } from 
'../v2/util/componentTypes';
+import { NEW_CHART_ID, NEW_COMPONENTS_SOURCE_ID } from '../v2/util/constants';
 
 const propTypes = {
-  dashboard: PropTypes.object.isRequired,
-  triggerNode: PropTypes.node.isRequired,
+  actions: PropTypes.object,
+  isLoading: PropTypes.bool.isRequired,
+  slices: PropTypes.objectOf(slicePropShape).isRequired,
+  lastUpdated: PropTypes.number.isRequired,
+  errorMsg: PropTypes.string,
   userId: PropTypes.string.isRequired,
-  addSlicesToDashboard: PropTypes.func,
+  selectedSliceIds: PropTypes.object,
+  editMode: PropTypes.bool,
+};
+
+const defaultProps = {
+  selectedSliceIds: new Set(),
+  editMode: false,
 };
 
+const KEYS_TO_FILTERS = ['slice_name', 'viz_type', 'datasource_name'];
+const KEYS_TO_SORT = [
+  { key: 'slice_name', display: 'Name' },
+  { key: 'viz_type', display: 'Viz' },
 
 Review comment:
   Can you call this `Visualization`? also `label` is more common than `display`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180879055
 
 

 ##
 File path: 
superset/assets/javascripts/dashboard/components/SliceHeaderControls.jsx
 ##
 @@ -0,0 +1,103 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import cx from 'classnames';
+import moment from 'moment';
+import { DropdownButton } from 'react-bootstrap';
+
+import { ActionMenuItem } from './ActionMenuItem';
+import { t } from '../../locales';
+
+const propTypes = {
+  slice: PropTypes.object.isRequired,
+  isCached: PropTypes.bool,
+  isExpanded: PropTypes.bool,
+  cachedDttm: PropTypes.string,
+  toggleExpandSlice: PropTypes.func,
+  forceRefresh: PropTypes.func,
+  exploreChart: PropTypes.func,
+  exportCSV: PropTypes.func,
+};
+
+const defaultProps = {
+  forceRefresh: () => ({}),
+  toggleExpandSlice: () => ({}),
+  exploreChart: () => ({}),
+  exportCSV: () => ({}),
+};
+
+class SliceHeaderControls extends React.PureComponent {
+  constructor(props) {
+super(props);
+this.exportCSV = this.props.exportCSV.bind(this, 'slice_' + 
this.props.slice.slice_id);
 
 Review comment:
   does this break if `props.slice_id` changes? why not just bind to a 
component method and pass id there?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r18096
 
 

 ##
 File path: 
superset/assets/javascripts/dashboard/v2/containers/DashboardHeader.jsx
 ##
 @@ -1,23 +1,39 @@
 import { ActionCreators as UndoActionCreators } from 'redux-undo';
+import {
+  setEditMode,
+  toggleBuilderPane,
+  fetchFaveStar,
+  saveFaveStar,
+  fetchCharts,
+  startPeriodicRender,
+  updateDashboardTitle,
+  onChange,
+  onSave,
+} from '../../actions/dashboard'
 import { bindActionCreators } from 'redux';
 import { connect } from 'react-redux';
 
-import DashboardHeader from '../components/DashboardHeader';
+import DashboardHeader from '../../components/Header';
 
 Review comment:
   please move the new header to `v2/`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180888627
 
 

 ##
 File path: 
superset/assets/javascripts/dashboard/v2/containers/DashboardGrid.jsx
 ##
 @@ -7,11 +7,18 @@ import {
   resizeComponent,
 } from '../actions/dashboardLayout';
 
+function mapStateToProps({ dashboard }, ownProps) {
+  return {
+editMode: dashboard.editMode,
+cells: ownProps.cells,
 
 Review comment:
   why are you overwriting cells with the same `ownProps`? also see my comments 
about removing cells.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180874614
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/GridCell.jsx
 ##
 @@ -4,8 +4,8 @@ import PropTypes from 'prop-types';
 
 
 Review comment:
   what's a grid cell and why do we need it in v2? can you give it a better 
name if we still need it?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180869813
 
 

 ##
 File path: superset/assets/javascripts/dashboard/actions/allSlices.js
 ##
 @@ -0,0 +1,66 @@
+/* global notify */
+import $ from 'jquery';
+
+export const UPDATE_SLICE_NAME = 'UPDATE_SLICE_NAME';
+export function updateSliceName(key, sliceName) {
+  return { type: UPDATE_SLICE_NAME, key, sliceName };
+}
+
+export function saveSliceName(slice, sliceName) {
+  const oldName = slice.slice_name;
+  return (dispatch) => {
+const sliceParams = {};
+sliceParams.slice_id = slice.slice_id;
+sliceParams.action = 'overwrite';
+sliceParams.slice_name = sliceName;
+
+const url = slice.slice_url + '&' +
+  Object.keys(sliceParams)
+  .map(key => (key + '=' + sliceParams[key]))
+  .join('&');
+const key = 'slice_' + slice.slice_id;
+return $.ajax({
+  url,
+  type: 'POST',
+  success: () => {
+dispatch(updateSliceName(key, sliceName));
+notify.success('This slice name was saved successfully.');
+  },
+  error: () => {
+// if server-side reject the overwrite action,
+// revert to old state
+dispatch(updateSliceName(key, oldName));
+notify.error("You don't have the rights to alter this slice");
 
 Review comment:
   should update to new toasts


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180881331
 
 

 ##
 File path: superset/assets/javascripts/dashboard/reducers/dashboard.js
 ##
 @@ -0,0 +1,123 @@
+import d3 from 'd3';
 
 Review comment:
   we should not import all of d3 for this utility function! please remove it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] williaster commented on a change in pull request #4791: apply new Dashboard builder to dashboard

[GitBox]

williaster commented on a change in pull request #4791: apply new Dashboard 
builder to dashboard
URL: 
https://github.com/apache/incubator-superset/pull/4791#discussion_r180877358
 
 

 ##
 File path: superset/assets/javascripts/dashboard/components/SliceAdder.jsx
 ##
 @@ -1,43 +1,56 @@
 import React from 'react';
 
 Review comment:
   if this is for v2 it should be moved to the `v2/` folder


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services