You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/16 06:55:54 UTC

[GitHub] [superset] simcha90 opened a new pull request #13145: feat(dashboard_rbac): manage roles for dashboard

simcha90 opened a new pull request #13145:
URL: https://github.com/apache/superset/pull/13145


   ### SUMMARY
   This PR add roles management to DashboardProperties modal under Feature flag
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   https://user-images.githubusercontent.com/56388545/108028455-882a0e80-7034-11eb-9e93-3e254f381c86.mov
   
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] simcha90 commented on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
simcha90 commented on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-783191116


   @amitmiran137 fixed, @willbarrett added


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] simcha90 commented on a change in pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
simcha90 commented on a change in pull request #13145:
URL: https://github.com/apache/superset/pull/13145#discussion_r580043536



##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -276,12 +304,109 @@ class PropertiesModal extends React.PureComponent {
           jsonMetadata: result.json_metadata,
           ownerIds: result.owners,
           colorScheme: metadataColorScheme || colorScheme,
+          ...moreResultProps,
         });
         this.props.onHide();
       }, handleErrorResponse);
     }
   }
 
+  getRowsWithoutRoles() {
+    const { values, isDashboardLoaded } = this.state;
+    return (
+      <Row>
+        <Col md={6}>
+          <h3 style={{ marginTop: '1em' }}>{t('Access')}</h3>
+          <FormLabel htmlFor="owners">{t('Owners')}</FormLabel>
+          <AsyncSelect
+            name="owners"
+            isMulti
+            value={values.owners}
+            loadOptions={loadAccessOptions('owners')}
+            defaultOptions // load options on render
+            cacheOptions
+            onChange={this.onOwnersChange}
+            disabled={!isDashboardLoaded}
+            filterOption={null} // options are filtered at the api
+          />
+          <p className="help-block">
+            {t(
+              'Owners is a list of users who can alter the dashboard. Searchable by name or username.',
+            )}
+          </p>
+        </Col>
+        <Col md={6}>
+          <h3 style={{ marginTop: '1em' }}>{t('Colors')}</h3>
+          <ColorSchemeControlWrapper
+            onChange={this.onColorSchemeChange}
+            colorScheme={values.colorScheme}
+          />
+        </Col>
+      </Row>
+    );
+  }
+
+  getRowsWithRoles() {

Review comment:
       I did it first... but except of add `AsyncSelect` it also require changes in layout, because of it I moved to separate functions only code that requires changes of layout




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io commented on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-783321945


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=h1) Report
   > Merging [#13145](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=desc) (26556fc) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `2.56%`.
   > The diff coverage is `66.25%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13145/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13145      +/-   ##
   ==========================================
   + Coverage   53.06%   55.62%   +2.56%     
   ==========================================
     Files         489      474      -15     
     Lines       17314    15762    -1552     
     Branches     4482     4044     -438     
   ==========================================
   - Hits         9187     8768     -419     
   + Misses       8127     6994    -1133     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `55.62% <66.25%> (+2.56%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../src/SqlLab/components/EstimateQueryCostButton.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0VzdGltYXRlUXVlcnlDb3N0QnV0dG9uLmpzeA==) | `20.83% <ø> (ø)` | |
   | [...end/src/SqlLab/components/ExploreResultsButton.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0V4cGxvcmVSZXN1bHRzQnV0dG9uLmpzeA==) | `8.00% <ø> (ø)` | |
   | [...et-frontend/src/SqlLab/components/QueryHistory.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5SGlzdG9yeS5qc3g=) | `50.00% <0.00%> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <0.00%> (-0.09%)` | :arrow_down: |
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (ø)` | |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <ø> (ø)` | |
   | [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `57.89% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `75.67% <0.00%> (-1.04%)` | :arrow_down: |
   | ... and [249 more](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=footer). Last update [f85497e...26556fc](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=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 to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] willbarrett commented on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
willbarrett commented on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-781669127


   I've added a few developers that are more familiar with this code. These add/edit modals become quite complex over time and are frequent sources of bugs in the codebase. Would it be possible to add tests for the new functionality to help reduce the likelihood of future regressions?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-790638473


   @simcha90 This change is causing a test to fail in Node 15 since in this version of Node they changed the way to [handle promise rejections](https://developer.ibm.com/languages/node-js/blogs/nodejs-15-release-blog/). In previous versions of Node, promise rejections are handled as warnings, so the test pass but that doesn't mean that the test is correct. I don't know if we support Node 15 but I'm using it for development for security reasons. Can you check what's causing the promise rejection?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] simcha90 commented on a change in pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
simcha90 commented on a change in pull request #13145:
URL: https://github.com/apache/superset/pull/13145#discussion_r580043536



##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -276,12 +304,109 @@ class PropertiesModal extends React.PureComponent {
           jsonMetadata: result.json_metadata,
           ownerIds: result.owners,
           colorScheme: metadataColorScheme || colorScheme,
+          ...moreResultProps,
         });
         this.props.onHide();
       }, handleErrorResponse);
     }
   }
 
+  getRowsWithoutRoles() {
+    const { values, isDashboardLoaded } = this.state;
+    return (
+      <Row>
+        <Col md={6}>
+          <h3 style={{ marginTop: '1em' }}>{t('Access')}</h3>
+          <FormLabel htmlFor="owners">{t('Owners')}</FormLabel>
+          <AsyncSelect
+            name="owners"
+            isMulti
+            value={values.owners}
+            loadOptions={loadAccessOptions('owners')}
+            defaultOptions // load options on render
+            cacheOptions
+            onChange={this.onOwnersChange}
+            disabled={!isDashboardLoaded}
+            filterOption={null} // options are filtered at the api
+          />
+          <p className="help-block">
+            {t(
+              'Owners is a list of users who can alter the dashboard. Searchable by name or username.',
+            )}
+          </p>
+        </Col>
+        <Col md={6}>
+          <h3 style={{ marginTop: '1em' }}>{t('Colors')}</h3>
+          <ColorSchemeControlWrapper
+            onChange={this.onColorSchemeChange}
+            colorScheme={values.colorScheme}
+          />
+        </Col>
+      </Row>
+    );
+  }
+
+  getRowsWithRoles() {

Review comment:
       I did it in the beginning... but except of add `AsyncSelect` it also require changes in layout, because of it I moved to separate functions only code that requires changes of layout




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-779709284


   @riahk it would be great if you could take a look at this PR!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] amitmiran137 merged pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
amitmiran137 merged pull request #13145:
URL: https://github.com/apache/superset/pull/13145


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-795489696


   > @simcha90 This change is causing the test to fail in Node 15 since in this version of Node they changed the way to [handle promise rejections](https://developer.ibm.com/languages/node-js/blogs/nodejs-15-release-blog/). In previous versions of Node, promise rejections are handled as warnings, so the test pass but that doesn't mean that the test is correct. I don't know if we support Node 15 but I'm using it in development for security reasons. Can you check what's causing the promise rejection?
   > 
   > <img alt="Screen Shot 2021-03-04 at 11 26 26 AM" width="1320" src="https://user-images.githubusercontent.com/70410625/109978301-81f68c00-7cdc-11eb-826c-86a3874da8fa.png">
   
   Fixed in #13548


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] amitmiran137 commented on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
amitmiran137 commented on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-781288594


   @simcha90 I found an issue when trying to remove all roles & save then it failed to save
   
   https://user-images.githubusercontent.com/47772523/108352842-1a2a4680-71f0-11eb-99e3-ad0241e95da8.mov
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-783321945


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=h1) Report
   > Merging [#13145](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=desc) (26556fc) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `5.50%`.
   > The diff coverage is `69.25%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13145/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13145      +/-   ##
   ==========================================
   + Coverage   53.06%   58.56%   +5.50%     
   ==========================================
     Files         489      478      -11     
     Lines       17314    16005    -1309     
     Branches     4482     4127     -355     
   ==========================================
   + Hits         9187     9374     +187     
   + Misses       8127     6631    -1496     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `58.56% <69.25%> (+5.50%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../src/SqlLab/components/EstimateQueryCostButton.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0VzdGltYXRlUXVlcnlDb3N0QnV0dG9uLmpzeA==) | `20.83% <ø> (ø)` | |
   | [...end/src/SqlLab/components/ExploreResultsButton.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0V4cGxvcmVSZXN1bHRzQnV0dG9uLmpzeA==) | `8.00% <ø> (ø)` | |
   | [...et-frontend/src/SqlLab/components/QueryHistory.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5SGlzdG9yeS5qc3g=) | `50.00% <0.00%> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <0.00%> (-0.09%)` | :arrow_down: |
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (ø)` | |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <ø> (ø)` | |
   | [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `57.89% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `75.67% <0.00%> (-1.04%)` | :arrow_down: |
   | ... and [207 more](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=footer). Last update [f85497e...26556fc](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=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 to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] lilykuang commented on a change in pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
lilykuang commented on a change in pull request #13145:
URL: https://github.com/apache/superset/pull/13145#discussion_r578795167



##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -276,12 +304,109 @@ class PropertiesModal extends React.PureComponent {
           jsonMetadata: result.json_metadata,
           ownerIds: result.owners,
           colorScheme: metadataColorScheme || colorScheme,
+          ...moreResultProps,
         });
         this.props.onHide();
       }, handleErrorResponse);
     }
   }
 
+  getRowsWithoutRoles() {
+    const { values, isDashboardLoaded } = this.state;
+    return (
+      <Row>
+        <Col md={6}>
+          <h3 style={{ marginTop: '1em' }}>{t('Access')}</h3>
+          <FormLabel htmlFor="owners">{t('Owners')}</FormLabel>
+          <AsyncSelect
+            name="owners"
+            isMulti
+            value={values.owners}
+            loadOptions={loadAccessOptions('owners')}
+            defaultOptions // load options on render
+            cacheOptions
+            onChange={this.onOwnersChange}
+            disabled={!isDashboardLoaded}
+            filterOption={null} // options are filtered at the api
+          />
+          <p className="help-block">
+            {t(
+              'Owners is a list of users who can alter the dashboard. Searchable by name or username.',
+            )}
+          </p>
+        </Col>
+        <Col md={6}>
+          <h3 style={{ marginTop: '1em' }}>{t('Colors')}</h3>
+          <ColorSchemeControlWrapper
+            onChange={this.onColorSchemeChange}
+            colorScheme={values.colorScheme}
+          />
+        </Col>
+      </Row>
+    );
+  }
+
+  getRowsWithRoles() {

Review comment:
       `getRowsWithoutRoles ` and `getRowsWithRoles ` are very similar.  Maybe the two functions could combine into one and use `DASHBOARD_RBAC` feature flag to determine  `AsyncSelect` with role show or hidden.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina edited a comment on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-790638473


   @simcha90 This change is causing a test to fail in Node 15 since in this version of Node they changed the way to [handle promise rejections](https://developer.ibm.com/languages/node-js/blogs/nodejs-15-release-blog/). In previous versions of Node, promise rejections are handled as warnings, so the test pass but that doesn't mean that the test is correct. I don't know if we support Node 15 but I'm using it in development for security reasons. Can you check what's causing the promise rejection?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] lilykuang commented on a change in pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
lilykuang commented on a change in pull request #13145:
URL: https://github.com/apache/superset/pull/13145#discussion_r578795167



##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -276,12 +304,109 @@ class PropertiesModal extends React.PureComponent {
           jsonMetadata: result.json_metadata,
           ownerIds: result.owners,
           colorScheme: metadataColorScheme || colorScheme,
+          ...moreResultProps,
         });
         this.props.onHide();
       }, handleErrorResponse);
     }
   }
 
+  getRowsWithoutRoles() {
+    const { values, isDashboardLoaded } = this.state;
+    return (
+      <Row>
+        <Col md={6}>
+          <h3 style={{ marginTop: '1em' }}>{t('Access')}</h3>
+          <FormLabel htmlFor="owners">{t('Owners')}</FormLabel>
+          <AsyncSelect
+            name="owners"
+            isMulti
+            value={values.owners}
+            loadOptions={loadAccessOptions('owners')}
+            defaultOptions // load options on render
+            cacheOptions
+            onChange={this.onOwnersChange}
+            disabled={!isDashboardLoaded}
+            filterOption={null} // options are filtered at the api
+          />
+          <p className="help-block">
+            {t(
+              'Owners is a list of users who can alter the dashboard. Searchable by name or username.',
+            )}
+          </p>
+        </Col>
+        <Col md={6}>
+          <h3 style={{ marginTop: '1em' }}>{t('Colors')}</h3>
+          <ColorSchemeControlWrapper
+            onChange={this.onColorSchemeChange}
+            colorScheme={values.colorScheme}
+          />
+        </Col>
+      </Row>
+    );
+  }
+
+  getRowsWithRoles() {

Review comment:
       `getRowsWithoutRoles ` and `getRowsWithRoles ` are very similar.  Maybe `DASHBOARD_RBAC` feature flag could be used for making `AsyncSelect` with role show or hidden instead of using two function 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina edited a comment on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-790638473


   @simcha90 This change is causing the test to fail in Node 15 since in this version of Node they changed the way to [handle promise rejections](https://developer.ibm.com/languages/node-js/blogs/nodejs-15-release-blog/). In previous versions of Node, promise rejections are handled as warnings, so the test pass but that doesn't mean that the test is correct. I don't know if we support Node 15 but I'm using it in development for security reasons. Can you check what's causing the promise rejection?
   
   <img width="1320" alt="Screen Shot 2021-03-04 at 11 26 26 AM" src="https://user-images.githubusercontent.com/70410625/109978301-81f68c00-7cdc-11eb-826c-86a3874da8fa.png">


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13145: feat(dashboard_rbac): manage roles for dashboard

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13145:
URL: https://github.com/apache/superset/pull/13145#issuecomment-783321945


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=h1) Report
   > Merging [#13145](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=desc) (26556fc) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `5.10%`.
   > The diff coverage is `69.25%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13145/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13145      +/-   ##
   ==========================================
   + Coverage   53.06%   58.16%   +5.10%     
   ==========================================
     Files         489      478      -11     
     Lines       17314    16005    -1309     
     Branches     4482     4127     -355     
   ==========================================
   + Hits         9187     9310     +123     
   + Misses       8127     6695    -1432     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `58.16% <69.25%> (+5.10%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../src/SqlLab/components/EstimateQueryCostButton.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0VzdGltYXRlUXVlcnlDb3N0QnV0dG9uLmpzeA==) | `20.83% <ø> (ø)` | |
   | [...end/src/SqlLab/components/ExploreResultsButton.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0V4cGxvcmVSZXN1bHRzQnV0dG9uLmpzeA==) | `8.00% <ø> (ø)` | |
   | [...et-frontend/src/SqlLab/components/QueryHistory.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5SGlzdG9yeS5qc3g=) | `50.00% <0.00%> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <0.00%> (-0.09%)` | :arrow_down: |
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (ø)` | |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <ø> (ø)` | |
   | [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `57.89% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `75.67% <0.00%> (-1.04%)` | :arrow_down: |
   | ... and [212 more](https://codecov.io/gh/apache/superset/pull/13145/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=footer). Last update [f85497e...26556fc](https://codecov.io/gh/apache/superset/pull/13145?src=pr&el=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 to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org