fix: Revert "fix(native-filters): Fix update ownState" #17311
Conversation
This reverts commit cf284ba.
etr2460
changed the title
Codecov Report
@@ Coverage Diff @@
## master #17311 +/- ##
==========================================
- Coverage 77.09% 77.08% -0.01%
==========================================
Files 1037 1037
Lines 55647 55625 -22
Branches 7608 7605 -3
==========================================
- Hits 42899 42877 -22
Misses 12498 12498
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM.
Agree to revert first. @simcha90 When change critical logic like this, please add tests first not revisit:add-tests.
|
Apologies for having been part of this regression. It seems we must be missing some fairly critical e2e tests for filter box, I thought there were cypress tests that would have caught a regression like this? |
|
It happens for complicated dashboard with tabs and scoped filters: when user apply a filer in the dashboard, it didn't trigger new query any more. i think unit test with complicated dashboard mock data should cover this. |
@graceguo-supercat I know there's an initiative right now for creating a comprehensive RTL test suite for dashboard filtering (ping @kgabryje ). It would be great if you could share some context on the setup in which this change broke filter boxes so we can add a test case for it. |
|
@villebro After some deliberations with Diego, later consulted with Adam, we decided that Cypress is better suited for such tests. That being said, some more context would help us a lot in developing comprehensive test suites |
Ok thanks for the info! |
|
I'm going to merge this to unbreak master. We can re PR out the perf improvement without the bugs (and ideally with some tests!) |
|
🏷️ 2021.44 |
…che#17311) This reverts commit cf284ba. (cherry picked from commit 7c6d6f4)
Reverts #17181
When deploying this to our environment with filter boxes enabled, it broke applying filters (if you added a filter and clicked apply, it wouldn't fire off new api requests for all the charts dependent on them).
I'm PRing a revert to get attention on this, but also since it's a pretty bad regression for filter box users. We're reverting it internally already to fix the critical bug.
to: @simcha90 @graceguo-supercat @villebro @rusackas