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/01/30 15:02:33 UTC

[GitHub] [superset] lyndsiWilliams opened a new pull request #12845: refactor: move CTAS/CVAS field

lyndsiWilliams opened a new pull request #12845:
URL: https://github.com/apache/superset/pull/12845


   ### SUMMARY
   Moved CTAS/CVAS Schema Field Closer to the "Allow Create Table As" In Edit Datasource Modal. This required nested conditional rendering, so I had to use some useState/useEffect in order to hold that state at a higher level for all functions to run properly.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   #### Field before:
   <img width="750" alt="ctascvas-before" src="https://user-images.githubusercontent.com/55605634/106359617-77aa3200-62d9-11eb-9484-63c01d9df377.png">
   #### Field after:
   <img width="750" alt="ctascvas-after-1" src="https://user-images.githubusercontent.com/55605634/106359624-7e38a980-62d9-11eb-8911-97b2709c4cc7.png">
   <img width="750" alt="ctascvas-after-2" src="https://user-images.githubusercontent.com/55605634/106359626-809b0380-62d9-11eb-963a-52702ca996b7.png">
   <img width="750" alt="ctascvas-after-3" src="https://user-images.githubusercontent.com/55605634/106359627-8264c700-62d9-11eb-95ed-838ba5d3b5ff.png">
   
   ### TEST PLAN
   Testing has not yet been implemented for this.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [x] 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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -467,87 +535,101 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                   tooltip={t('Allow this database to be queried in SQL Lab')}
                 />
               </div>
+              <StyledExpandableForm
+                style={{ display: isOpen ? 'inherit' : 'none' }}
+              >
+                <StyledInputContainer>
+                  <div className="input-container">
+                    <IndeterminateCheckbox
+                      id="allow_ctas"
+                      indeterminate={false}
+                      checked={db ? !!db.allow_ctas : false}
+                      onChange={onInputChange}
+                    />
+                    <div>{t('Allow CREATE TABLE AS')}</div>
+                    <InfoTooltip
+                      tooltip={t(
+                        'Allow creation of new tables based on queries',
+                      )}
+                    />
+                  </div>
+                </StyledInputContainer>
+                <StyledInputContainer>
+                  <div className="input-container">
+                    <IndeterminateCheckbox
+                      id="allow_cvas"
+                      indeterminate={false}
+                      checked={db ? !!db.allow_cvas : false}

Review comment:
       similar to line 546




----------------------------------------------------------------
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] hughhhh commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -467,87 +535,101 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                   tooltip={t('Allow this database to be queried in SQL Lab')}
                 />
               </div>
+              <StyledExpandableForm

Review comment:
       Can we add some test for this component to make sure it interacts properly




----------------------------------------------------------------
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] eschutho commented on pull request #12845: refactor: move CTAS/CVAS field

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


   @lyndsiWilliams do you want to put this in draft mode for the time being?


----------------------------------------------------------------
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 #12845: refactor: move CTAS/CVAS field

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


   # [Codecov](https://codecov.io/gh/apache/superset/pull/12845?src=pr&el=h1) Report
   > Merging [#12845](https://codecov.io/gh/apache/superset/pull/12845?src=pr&el=desc) (e9eda24) into [master](https://codecov.io/gh/apache/superset/commit/a0e05a5eff51a117ff8427967d14fe8758a81332?el=desc) (a0e05a5) will **decrease** coverage by `3.60%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/12845/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/12845?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #12845      +/-   ##
   ==========================================
   - Coverage   67.00%   63.40%   -3.61%     
   ==========================================
     Files        1022      488     -534     
     Lines       50105    30173   -19932     
     Branches     5191        0    -5191     
   ==========================================
   - Hits        33572    19130   -14442     
   + Misses      16402    11043    -5359     
   + Partials      131        0     -131     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | javascript | `?` | |
   | python | `63.40% <ø> (-0.67%)` | :arrow_down: |
   
   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/12845?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
   | [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-17.31%)` | :arrow_down: |
   | [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `83.67% <0.00%> (-8.17%)` | :arrow_down: |
   | [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `84.31% <0.00%> (-6.28%)` | :arrow_down: |
   | [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `90.62% <0.00%> (-6.25%)` | :arrow_down: |
   | [superset/databases/commands/test\_connection.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `84.78% <0.00%> (-4.35%)` | :arrow_down: |
   | [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
   | [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `85.59% <0.00%> (-3.27%)` | :arrow_down: |
   | ... and [548 more](https://codecov.io/gh/apache/superset/pull/12845/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12845?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/12845?src=pr&el=footer). Last update [a0e05a5...e9eda24](https://codecov.io/gh/apache/superset/pull/12845?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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -235,6 +249,36 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
     }
   };
 
+  const expandForm = (trueOrFalse: boolean) => {
+    // Count starts a 0 (closed)
+    // Any checkbox in this for that gets checked
+    // will increase the count and keep the form open
+    if (trueOrFalse) {
+      setCount(count + 1);
+    } else {
+      setCount(count - 1);
+    }
+  };

Review comment:
       maybe more succinct:
   `setCount(count + (shouldExpand ? 1 : -1)`




----------------------------------------------------------------
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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -250,6 +294,30 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
         typeof target.value === 'string' ? target.value.trim() : target.value;
     }
 
+    if (target.id === 'expose_in_sqllab') {
+      setIsOpen(!isOpen);
+    }
+
+    // Conditional form rendering
+    const checkedTrue = target.checked === true;
+    /* The CTAS & CVAS schema field needs individual setters
+    so that the input will not disappear when one options goes
+    unchecked but the other is still checked */
+    if (target.id.includes('cvas')) {
+      if (!cvas && checkedTrue) {
+        cvasCtasCheck('cvas', true);
+      } else {
+        cvasCtasCheck('cvas', false);
+      }

Review comment:
       shorter: 
   ```
   const { id, checked } = target;
   const check = (!cvas && id.includes('cvas') && 'cvas') || (!ctas && id.includes('ctas') && 'ctas');
   cvasCtasCheck( check, checked);
   ```




----------------------------------------------------------------
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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -467,87 +535,101 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                   tooltip={t('Allow this database to be queried in SQL Lab')}
                 />
               </div>
+              <StyledExpandableForm
+                style={{ display: isOpen ? 'inherit' : 'none' }}
+              >
+                <StyledInputContainer>
+                  <div className="input-container">
+                    <IndeterminateCheckbox
+                      id="allow_ctas"
+                      indeterminate={false}
+                      checked={db ? !!db.allow_ctas : false}
+                      onChange={onInputChange}
+                    />
+                    <div>{t('Allow CREATE TABLE AS')}</div>
+                    <InfoTooltip
+                      tooltip={t(
+                        'Allow creation of new tables based on queries',
+                      )}
+                    />
+                  </div>
+                </StyledInputContainer>
+                <StyledInputContainer>
+                  <div className="input-container">
+                    <IndeterminateCheckbox
+                      id="allow_cvas"
+                      indeterminate={false}
+                      checked={db ? !!db.allow_cvas : false}
+                      onChange={onInputChange}
+                    />
+                    <div>{t('Allow CREATE VIEW AS')}</div>
+                    <InfoTooltip
+                      tooltip={t(
+                        'Allow creation of new views based on queries',
+                      )}
+                    />
+                  </div>
+                  <StyledInputContainer
+                    style={{ display: createAsOpen ? 'inherit' : 'none' }}
+                  >
+                    <div className="control-label">
+                      {t('CTAS & CVAS SCHEMA')}
+                    </div>
+                    <div className="input-container">
+                      <input
+                        type="text"
+                        name="force_ctas_schema"
+                        value={db ? db.force_ctas_schema || '' : ''}

Review comment:
       same refactor as above




----------------------------------------------------------------
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] lyndsiWilliams closed pull request #12845: refactor: move CTAS/CVAS field

Posted by GitBox <gi...@apache.org>.
lyndsiWilliams closed pull request #12845:
URL: https://github.com/apache/superset/pull/12845


   


-- 
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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -250,6 +294,30 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
         typeof target.value === 'string' ? target.value.trim() : target.value;
     }
 
+    if (target.id === 'expose_in_sqllab') {
+      setIsOpen(!isOpen);
+    }
+
+    // Conditional form rendering
+    const checkedTrue = target.checked === true;

Review comment:
       actually even better = `const { checked } = target`




----------------------------------------------------------------
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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -250,6 +294,30 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
         typeof target.value === 'string' ? target.value.trim() : target.value;
     }
 
+    if (target.id === 'expose_in_sqllab') {
+      setIsOpen(!isOpen);
+    }
+
+    // Conditional form rendering
+    const checkedTrue = target.checked === true;

Review comment:
       `const checkedTrue = target.checked` should work.




----------------------------------------------------------------
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] nytai commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -132,6 +141,11 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
   const [db, setDB] = useState<DatabaseObject | null>(null);
   const [isHidden, setIsHidden] = useState<boolean>(true);
   const [tabKey, setTabKey] = useState<string>(DEFAULT_TAB_KEY);
+  const [isOpen, setIsOpen] = useState<boolean>(true);

Review comment:
       does this refer to the modal being open or something else?




----------------------------------------------------------------
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] mistercrunch commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -120,6 +120,15 @@ const StyledJsonEditor = styled(JsonEditor)`
   border-radius: ${({ theme }) => theme.gridUnit}px;
 `;
 
+const StyledExpandableForm = styled.div`
+  padding-left: 28px;

Review comment:
       let's use `theme.gridUnit` as above with the right multiplier




----------------------------------------------------------------
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] hughhhh commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -235,6 +249,36 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
     }
   };
 
+  const expandForm = (trueOrFalse: boolean) => {
+    // Count starts a 0 (closed)
+    // Any checkbox in this for that gets checked
+    // will increase the count and keep the form open
+    if (trueOrFalse) {
+      setCount(count + 1);
+    } else {
+      setCount(count - 1);
+    }
+  };
+
+  useEffect(() => {
+    // Opens the form if at least one option is checked
+    // Closes the form only if both options are unchecked
+    if (count > 0) {

Review comment:
       couldn't this just a be a boolean if `expandForm` is called for `Expose SQL lab`




----------------------------------------------------------------
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] eschutho commented on pull request #12845: refactor: move CTAS/CVAS field

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


   Looks great. I just did a syntax check. I can look again for overall logic/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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -467,87 +535,101 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                   tooltip={t('Allow this database to be queried in SQL Lab')}
                 />
               </div>
+              <StyledExpandableForm
+                style={{ display: isOpen ? 'inherit' : 'none' }}
+              >
+                <StyledInputContainer>
+                  <div className="input-container">
+                    <IndeterminateCheckbox
+                      id="allow_ctas"
+                      indeterminate={false}
+                      checked={db ? !!db.allow_ctas : false}

Review comment:
       `checked={db && !!db.allow_ctas}`




----------------------------------------------------------------
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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -235,6 +249,36 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
     }
   };
 
+  const expandForm = (trueOrFalse: boolean) => {
+    // Count starts a 0 (closed)
+    // Any checkbox in this for that gets checked
+    // will increase the count and keep the form open
+    if (trueOrFalse) {
+      setCount(count + 1);
+    } else {
+      setCount(count - 1);
+    }
+  };
+
+  useEffect(() => {
+    // Opens the form if at least one option is checked
+    // Closes the form only if both options are unchecked
+    if (count > 0) {
+      setCreateAsOpen(true);
+    } else {
+      setCreateAsOpen(false);
+    }

Review comment:
       also more succinct: 
   `setCreateAsOpen(count>0)`




----------------------------------------------------------------
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] eschutho commented on a change in pull request #12845: refactor: move CTAS/CVAS field

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



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -235,6 +249,36 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
     }
   };
 
+  const expandForm = (trueOrFalse: boolean) => {

Review comment:
       small nit, can we name this arg something like `shouldExpand`?




----------------------------------------------------------------
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