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/23 20:23:01 UTC
[GitHub] [superset] yardz opened a new pull request #13305: test: Tests for OmniContainer
yardz opened a new pull request #13305:
URL: https://github.com/apache/superset/pull/13305
### SUMMARY
- Moving the `OmniContainer.tsx` file to `OmniContainer` folder
- Split into different files
### TEST PLAN
No change in behavior, all tests must pass.
Notes: In the type of `omnibar` there are no props `id` or `className`. However, in the original javascript implementation, the prop `className` were being used. I checked that in execution both attributes were send to `<input />`.
So to be able to change the file to typescript and implement the tests I split `Ominibar` in another file, with the same interface that was being used in `OminiContainer`.
I marked component XXX as `@deprecated` and create a test to ensuring that the input will have the id that was sent...
To work correctly, I had to use the "// @ts-ignore" tag before the id attribute.
I believe that the correct fix for this is a PR in `omnibar` types or find some other lib that does the same and can meet the implementation requirements.
----------------------------------------------------------------
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 #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-785058486
# [Codecov](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=h1) Report
> Merging [#13305](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=desc) (87af48e) into [master](https://codecov.io/gh/apache/superset/commit/29d6420ecc2187db951f08dd47289277a11f4db5?el=desc) (29d6420) will **decrease** coverage by `14.76%`.
> The diff coverage is `85.00%`.
[](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13305 +/- ##
===========================================
- Coverage 77.20% 62.43% -14.77%
===========================================
Files 872 574 -298
Lines 45101 20705 -24396
Branches 5435 5441 +6
===========================================
- Hits 34820 12928 -21892
+ Misses 10158 7567 -2591
- Partials 123 210 +87
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `62.43% <85.00%> (+0.09%)` | :arrow_up: |
| python | `?` | |
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/13305?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/common/components/Modal/Modal.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL01vZGFsL01vZGFsLnRzeA==) | `100.00% <ø> (ø)` | |
| [...tend/src/components/OmniContainer/getDashboards.ts](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9nZXREYXNoYm9hcmRzLnRz) | `37.50% <37.50%> (ø)` | |
| [...et-frontend/src/components/OmniContainer/index.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9pbmRleC50c3g=) | `96.42% <96.42%> (ø)` | |
| [...-frontend/src/components/OmniContainer/Omnibar.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9PbW5pYmFyLnRzeA==) | `100.00% <100.00%> (ø)` | |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/reducers/index.js](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvaW5kZXguanM=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/filters/components/Time/types.ts](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9UaW1lL3R5cGVzLnRz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...et-frontend/src/dashboard/containers/Dashboard.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [509 more](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13305?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/13305?src=pr&el=footer). Last update [29d6420...87af48e](https://codecov.io/gh/apache/superset/pull/13305?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] yardz commented on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
yardz commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-785030474
@ktmud If you can review this PR, I will appreciate it :-D
----------------------------------------------------------------
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] yardz commented on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
yardz commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-786167265
> It's broken in `master`, too. cc @hughhhh
Do you think that in this case we can merge it anyway and create an issue to fix it?
As the component was split and changed a lot, a fix on the master (on the component as it was) may end up having to redo this job.
----------------------------------------------------------------
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] ktmud commented on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-815128529
I'd vote for removing it for now since it's not working anyway...
cc @hughhhh
--
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] ktmud merged pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
ktmud merged pull request #13305:
URL: https://github.com/apache/superset/pull/13305
----------------------------------------------------------------
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] yardz commented on a change in pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13305:
URL: https://github.com/apache/superset/pull/13305#discussion_r582850641
##########
File path: superset-frontend/src/components/OmniContainer/getDashboards.ts
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { t, SupersetClient } from '@superset-ui/core';
+import to from 'await-to-js';
Review comment:
I commented explaining the reason for using it. If you still don't agree I will remove this lib.
----------------------------------------------------------------
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] ktmud commented on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-786263024
I think it's OK to merge once CI errors are fixed..
----------------------------------------------------------------
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] yardz commented on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
yardz commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-786659524
> I think it's OK to merge once CI errors are fixed..
Done!
----------------------------------------------------------------
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 edited a comment on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
eschutho edited a comment on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-812646781
@yardz @ktmud I'm getting an npm conflict with this package. It only accepts `react@"^16.13.1"` and we're on v 16.14. The package hasn't been updated in 2 years. Do you think we can remove this feature and add it back in at some point with better functionality? cc @yousoph
--
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 #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-785058486
# [Codecov](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=h1) Report
> Merging [#13305](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=desc) (41b35b9) into [master](https://codecov.io/gh/apache/superset/commit/29d6420ecc2187db951f08dd47289277a11f4db5?el=desc) (29d6420) will **decrease** coverage by `18.69%`.
> The diff coverage is `42.85%`.
[](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13305 +/- ##
===========================================
- Coverage 77.20% 58.50% -18.70%
===========================================
Files 872 482 -390
Lines 45101 16006 -29095
Branches 5435 4131 -1304
===========================================
- Hits 34820 9365 -25455
+ Misses 10158 6641 -3517
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `58.50% <42.85%> (-0.03%)` | :arrow_down: |
| javascript | `?` | |
| python | `?` | |
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/13305?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/common/components/Modal/Modal.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL01vZGFsL01vZGFsLnRzeA==) | `100.00% <ø> (ø)` | |
| [...-frontend/src/components/OmniContainer/Omnibar.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9PbW5pYmFyLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...tend/src/components/OmniContainer/getDashboards.ts](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9nZXREYXNoYm9hcmRzLnRz) | `16.66% <16.66%> (ø)` | |
| [...et-frontend/src/components/OmniContainer/index.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9pbmRleC50c3g=) | `55.00% <55.00%> (ø)` | |
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...tersConfigModal/Footer/CancelConfirmationAlert.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0Zvb3Rlci9DYW5jZWxDb25maXJtYXRpb25BbGVydC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ConfigModal/FiltersConfigForm/FilterScope/state.ts](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0ZpbHRlclNjb3BlL3N0YXRlLnRz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...odal/FiltersConfigForm/FilterScope/ScopingTree.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0ZpbHRlclNjb3BlL1Njb3BpbmdUcmVlLnRzeA==) | `6.25% <0.00%> (-93.75%)` | :arrow_down: |
| ... and [742 more](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13305?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/13305?src=pr&el=footer). Last update [29d6420...35f3339](https://codecov.io/gh/apache/superset/pull/13305?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] ktmud commented on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-785463038
BTW, do we plan to keep investing in this OmniBar feature? I think it's very useful and could have great potential. We may even insert it to all pages and let users search for any entities. (An alternative would be a global search bar that's always visible in the global nav).
cc @hughhhh @junlincc @willbarrett
----------------------------------------------------------------
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 #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-812646781
@yardz @ktmud I'm getting an npm conflict with this package. It only accepts `react@"^16.13.1"` and we're on v 16.14. The packages hasn't been updated in 2 years. Do you think we can remove this feature and add it back in at some point with better functionality? cc @yousoph
--
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] yardz commented on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
yardz commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-785924042
> With feature flag `OMNIBAR` turned on, it is supposed to show a dashboard list like in #6745 But I'm not seeing anything even though there are API requests fired.
@ktmud To develop I removed the feature flag check and tested the bar (before doing this "refactor" and tests). It no shows anything, I thought it was a work in progress...
Can you check if when I did the refactor I accidentally inserted a bug? or if it was already like that on the master. (I don't know how to activate feature flags in this project rsrsrs)
----------------------------------------------------------------
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 #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-785058486
# [Codecov](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=h1) Report
> Merging [#13305](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=desc) (fea16be) into [master](https://codecov.io/gh/apache/superset/commit/29d6420ecc2187db951f08dd47289277a11f4db5?el=desc) (29d6420) will **decrease** coverage by `4.46%`.
> The diff coverage is `61.01%`.
[](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13305 +/- ##
==========================================
- Coverage 77.20% 72.73% -4.47%
==========================================
Files 872 596 -276
Lines 45101 21258 -23843
Branches 5435 5420 -15
==========================================
- Hits 34820 15462 -19358
+ Misses 10158 5672 -4486
- Partials 123 124 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `57.62% <26.44%> (-0.92%)` | :arrow_down: |
| javascript | `62.50% <61.01%> (+0.16%)` | :arrow_up: |
| python | `?` | |
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/13305?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `64.15% <0.00%> (+4.50%)` | :arrow_up: |
| [...set-frontend/src/common/components/Modal/Modal.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL01vZGFsL01vZGFsLnRzeA==) | `100.00% <ø> (ø)` | |
| [...rset-frontend/src/components/AsyncSelect/index.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQXN5bmNTZWxlY3QvaW5kZXguanN4) | `96.29% <ø> (ø)` | |
| [...rset-frontend/src/components/DeleteModal/index.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRGVsZXRlTW9kYWwvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
| [...et-frontend/src/components/Icons/icons.stories.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvaWNvbnMuc3Rvcmllcy5qc3g=) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/components/Label/index.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGFiZWwvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
| [...c/components/ListViewCard/ListViewCard.stories.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXdDYXJkL0xpc3RWaWV3Q2FyZC5zdG9yaWVzLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...set-frontend/src/components/ListViewCard/index.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXdDYXJkL2luZGV4LnRzeA==) | `100.00% <ø> (+5.45%)` | :arrow_up: |
| [...et-frontend/src/components/Menu/LanguagePicker.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9MYW5ndWFnZVBpY2tlci50c3g=) | `64.28% <ø> (ø)` | |
| [superset-frontend/src/components/Menu/SubMenu.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9TdWJNZW51LnRzeA==) | `100.00% <ø> (ø)` | |
| ... and [377 more](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13305?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/13305?src=pr&el=footer). Last update [29d6420...35f3339](https://codecov.io/gh/apache/superset/pull/13305?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] ktmud commented on pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-786142513
It's broken in `master`, too. cc @hughhhh
Here's the documentation on turning on/off feature flags:
https://superset.apache.org/docs/installation/configuring-superset#feature-flags
----------------------------------------------------------------
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 #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13305:
URL: https://github.com/apache/superset/pull/13305#issuecomment-785058486
# [Codecov](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=h1) Report
> Merging [#13305](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=desc) (41b35b9) into [master](https://codecov.io/gh/apache/superset/commit/29d6420ecc2187db951f08dd47289277a11f4db5?el=desc) (29d6420) will **decrease** coverage by `18.69%`.
> The diff coverage is `42.85%`.
[](https://codecov.io/gh/apache/superset/pull/13305?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13305 +/- ##
===========================================
- Coverage 77.20% 58.50% -18.70%
===========================================
Files 872 482 -390
Lines 45101 16006 -29095
Branches 5435 4131 -1304
===========================================
- Hits 34820 9365 -25455
+ Misses 10158 6641 -3517
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `58.50% <42.85%> (-0.03%)` | :arrow_down: |
| javascript | `?` | |
| python | `?` | |
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/13305?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/common/components/Modal/Modal.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL01vZGFsL01vZGFsLnRzeA==) | `100.00% <ø> (ø)` | |
| [...-frontend/src/components/OmniContainer/Omnibar.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9PbW5pYmFyLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...tend/src/components/OmniContainer/getDashboards.ts](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9nZXREYXNoYm9hcmRzLnRz) | `16.66% <16.66%> (ø)` | |
| [...et-frontend/src/components/OmniContainer/index.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9pbmRleC50c3g=) | `55.00% <55.00%> (ø)` | |
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...tersConfigModal/Footer/CancelConfirmationAlert.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0Zvb3Rlci9DYW5jZWxDb25maXJtYXRpb25BbGVydC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ConfigModal/FiltersConfigForm/FilterScope/state.ts](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0ZpbHRlclNjb3BlL3N0YXRlLnRz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...odal/FiltersConfigForm/FilterScope/ScopingTree.tsx](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0ZpbHRlclNjb3BlL1Njb3BpbmdUcmVlLnRzeA==) | `6.25% <0.00%> (-93.75%)` | :arrow_down: |
| ... and [742 more](https://codecov.io/gh/apache/superset/pull/13305/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13305?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/13305?src=pr&el=footer). Last update [29d6420...8813bd8](https://codecov.io/gh/apache/superset/pull/13305?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] ktmud commented on a change in pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13305:
URL: https://github.com/apache/superset/pull/13305#discussion_r583100797
##########
File path: superset-frontend/src/components/OmniContainer/getDashboards.ts
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { t, SupersetClient } from '@superset-ui/core';
+import to from 'await-to-js';
+
+interface DashboardItem {
+ changed_by_name: string;
+ changed_on: string;
+ creator: string;
+ dashboard_link: string;
+ dashboard_title: string;
+ id: number;
+ modified: string;
+ url: string;
+}
+
+interface Dashboards extends DashboardItem {
+ title: string;
+}
+
+export const getDashboards = async (
+ query: string,
+): Promise<(Dashboards | { title: string })[]> => {
+ // todo: Build a dedicated endpoint for dashboard searching
+ // i.e. superset/v1/api/dashboards?q=${query}
+ const [error, response] = await to(
+ SupersetClient.get({
+ endpoint: `/dashboardasync/api/read?_oc_DashboardModelViewAsync=changed_on&_od_DashboardModelViewAsync=desc&_flt_2_dashboard_title=${query}`,
+ }),
+ );
+ if (error) {
+ return [{ title: t('An error occurred while fetching dashboards') }];
+ }
Review comment:
Let's try not to add a new dependency. Try/catch is universal and easy enough, IMO. It seems all this library does it transforming error handling into another style which not all developers are familiar with. It takes some cognitive load understand what this library does, even it's something dead simple---people would wonder, sure, this seems to handle errors from a promise, but does it do anything extra?
In the future, all async API requests and error handling may be abstracted away by the [`useResource`](https://github.com/apache/superset/pull/13218) hook @suddjian is working on so this is probably even more unnecessary.
Even right now, [`makeApi`](https://github.com/apache-superset/superset-ui/blob/482da217938bcbd70db8b79117abacf00f8037e2/packages/superset-ui-core/test/query/api/v1/makeApi.test.ts#L170-L187) has already made it easy to handle response errors (it ensures erroring HTTP status code and errors in response body always throw and capturable by `try/catch`).
----------------------------------------------------------------
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] yardz commented on a change in pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13305:
URL: https://github.com/apache/superset/pull/13305#discussion_r582849585
##########
File path: superset-frontend/src/components/OmniContainer/getDashboards.ts
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { t, SupersetClient } from '@superset-ui/core';
+import to from 'await-to-js';
+
+interface DashboardItem {
+ changed_by_name: string;
+ changed_on: string;
+ creator: string;
+ dashboard_link: string;
+ dashboard_title: string;
+ id: number;
+ modified: string;
+ url: string;
+}
+
+interface Dashboards extends DashboardItem {
+ title: string;
+}
+
+export const getDashboards = async (
+ query: string,
+): Promise<(Dashboards | { title: string })[]> => {
+ // todo: Build a dedicated endpoint for dashboard searching
+ // i.e. superset/v1/api/dashboards?q=${query}
+ const [error, response] = await to(
+ SupersetClient.get({
+ endpoint: `/dashboardasync/api/read?_oc_DashboardModelViewAsync=changed_on&_od_DashboardModelViewAsync=desc&_flt_2_dashboard_title=${query}`,
+ }),
+ );
+ if (error) {
+ return [{ title: t('An error occurred while fetching dashboards') }];
+ }
Review comment:
@ktmud I know that try / catch produces the same result, but I wanted to show some points. I really like to use `await to` because the code is much more readable and easier to understand.
```ts
const [err, resp] = asyncFunc();
if(err)// do something
// do something
vs
try{
const resp = asyncFunc();
// do something
}catch(err){
// do something
}
```
The first option looks much more like the node's error first pattern.
It also makes requests a lot easier. For example, if same request returns an error (status 400) and some useful information in the body (error messages), with `await to` it is very simple to capture.
In general, I try to avoid `try/catch` as they break the error first pattern and makes the code a little more complex to read. Not that `try/catch` is very hard, it is because the `await to` is much easier.
If you still disagree, I can remove it, but I think it helps to decrease code complexity.
----------------------------------------------------------------
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] ktmud commented on a change in pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13305:
URL: https://github.com/apache/superset/pull/13305#discussion_r582358725
##########
File path: superset-frontend/src/components/OmniContainer/index.tsx
##########
@@ -0,0 +1,82 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useRef, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import Modal from 'src/common/components/Modal';
+import { useComponentDidMount } from 'src/common/hooks/useComponentDidMount';
+import { Omnibar } from './Omnibar';
+import { LOG_ACTIONS_OMNIBAR_TRIGGERED } from '../../logger/LogUtils';
Review comment:
```suggestion
import { LOG_ACTIONS_OMNIBAR_TRIGGERED } from 'src/logger/LogUtils';
```
##########
File path: superset-frontend/src/components/OmniContainer/getDashboards.ts
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { t, SupersetClient } from '@superset-ui/core';
+import to from 'await-to-js';
+
+interface DashboardItem {
+ changed_by_name: string;
+ changed_on: string;
+ creator: string;
+ dashboard_link: string;
+ dashboard_title: string;
+ id: number;
+ modified: string;
+ url: string;
+}
+
+interface Dashboards extends DashboardItem {
+ title: string;
+}
+
+export const getDashboards = async (
+ query: string,
+): Promise<(Dashboards | { title: string })[]> => {
+ // todo: Build a dedicated endpoint for dashboard searching
+ // i.e. superset/v1/api/dashboards?q=${query}
+ const [error, response] = await to(
+ SupersetClient.get({
+ endpoint: `/dashboardasync/api/read?_oc_DashboardModelViewAsync=changed_on&_od_DashboardModelViewAsync=desc&_flt_2_dashboard_title=${query}`,
+ }),
+ );
+ if (error) {
+ return [{ title: t('An error occurred while fetching dashboards') }];
+ }
Review comment:
```suggestion
let response;
try {
response = await SupersetClient.get({
endpoint: `/dashboardasync/api/read?_oc_DashboardModelViewAsync=changed_on&_od_DashboardModelViewAsync=desc&_flt_2_dashboard_title=${query}`,
});
} catch (error) {
return [{ title: t('An error occurred while fetching dashboards') }];
}
```
##########
File path: superset-frontend/src/components/OmniContainer/getDashboards.ts
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { t, SupersetClient } from '@superset-ui/core';
+import to from 'await-to-js';
Review comment:
I don't think this library is needed. See suggestion below.
----------------------------------------------------------------
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] ktmud commented on a change in pull request #13305: test: Tests for OmniContainer
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13305:
URL: https://github.com/apache/superset/pull/13305#discussion_r583863953
##########
File path: superset-frontend/src/components/OmniContainer/index.tsx
##########
@@ -0,0 +1,82 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useRef, useState } from 'react';
+import { styled } from '@superset-ui/core';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import Modal from 'src/common/components/Modal';
+import { useComponentDidMount } from 'src/common/hooks/useComponentDidMount';
+import { Omnibar } from './Omnibar';
+import { LOG_ACTIONS_OMNIBAR_TRIGGERED } from '../../logger/LogUtils';
Review comment:
Not a big deal but we prefer absolute paths nowadays.
##########
File path: superset-frontend/src/components/OmniContainer/Omnibar.tsx
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import OmnibarDeprecated from 'omnibar';
+
+interface Props {
+ id: string;
+ placeholder: string;
+ extensions: ((query: string) => Promise<any>)[];
+}
+
+/**
+ * @deprecated Component "omnibar" does not support prop className or id (the original implementation used className). However, the original javascript code was sending these prop and was working correctly. lol
+ * As this behavior is unpredictable, and does not works whitch types, I have isolated this component so that in the future a better solution can be found and implemented.
+ * We need to find a substitute for this component or some way of working around this problem
Review comment:
Thanks for the notes.
----------------------------------------------------------------
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