Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: use before_request hook for dynamic routes #14568

Merged
merged 5 commits into from
May 14, 2021

Conversation

benjreinhart
Copy link
Contributor

@benjreinhart benjreinhart commented May 11, 2021

SUMMARY

This PR leverages a new FAB feature to allow certain features to be enabled/disabled at runtime rather than once during boot. Routes will 404 if features are disabled, which is the same functionality present today. It also presents a consistent way to handle conditional route availability, giving us an opportunity to unify how this type of thing is handled.

Notes

This PR also introduces a new pattern for enabling/disabling feature flags per test using the with_feature_flags decorator. Example:

class TestYourFeature(SupersetTestCase):

    @with_feature_flags(YOUR_FEATURE=True)
    def test_your_feature_enabled(self):
        self.assertEqual(is_feature_enabled("YOUR_FEATURE"), True)

    @with_feature_flags(YOUR_FEATURE=False, ANOTHER_FEATURE=True)
    def test_your_feature_disabled(self):
        self.assertEqual(is_feature_enabled("YOUR_FEATURE"), False)
        self.assertEqual(is_feature_enabled("ANOTHER_FEATURE"), True)

TEST PLAN

Manual/unit/fab tests.

cc @dpgaspar @robdiciuccio @craig-rueda

Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #14568 (4a5b33e) into master (e4d2424) will decrease coverage by 0.17%.
The diff coverage is 97.29%.

❗ Current head 4a5b33e differs from pull request most recent head 5700897. Consider uploading reports for the commit 5700897 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14568      +/-   ##
==========================================
- Coverage   77.47%   77.30%   -0.18%     
==========================================
  Files         958      959       +1     
  Lines       48486    48512      +26     
  Branches     5679     5681       +2     
==========================================
- Hits        37565    37500      -65     
- Misses      10721    10812      +91     
  Partials      200      200              
Flag Coverage Δ
hive ?
mysql 81.39% <97.29%> (+0.15%) ⬆️
postgres 81.41% <97.29%> (+0.15%) ⬆️
presto ?
python 81.46% <97.29%> (-0.33%) ⬇️
sqlite ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/reports/logs/api.py 94.11% <87.50%> (-1.34%) ⬇️
superset/app.py 81.31% <100.00%> (-0.07%) ⬇️
superset/charts/api.py 86.22% <100.00%> (+4.29%) ⬆️
superset/dashboards/api.py 92.30% <100.00%> (+4.63%) ⬆️
superset/reports/api.py 87.78% <100.00%> (+0.68%) ⬆️
superset/utils/urls.py 100.00% <100.00%> (ø)
superset/db_engines/hive.py 0.00% <0.00%> (-82.15%) ⬇️
...et-frontend/src/components/Menu/LanguagePicker.tsx 64.28% <0.00%> (-20.72%) ⬇️
superset/db_engine_specs/hive.py 70.32% <0.00%> (-17.08%) ⬇️
superset/db_engine_specs/presto.py 83.36% <0.00%> (-6.95%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4d2424...5700897. Read the comment docs.

@benjreinhart benjreinhart force-pushed the benjreinhart/fab-registration branch from 0c10565 to 4a5833c Compare May 11, 2021 19:37
@robdiciuccio
Copy link
Member

Is there any test coverage for the affected endpoints?

@benjreinhart
Copy link
Contributor Author

@robdiciuccio yes and no. The feature flags are True in the test environment, and so the happy path is tested. However, there are no automated tests that flip the switch on/off and observe the behavior. I felt confident in the change and did not attempt to add those, but can do so to boost confidence.

@robdiciuccio
Copy link
Member

@benjreinhart Would be nice to have some tests to ensure the 404 response is working as intended. I realize it's a bit out of scope since these didn't exist in the first place, but hey, more test coverage!

@benjreinhart benjreinhart force-pushed the benjreinhart/fab-registration branch from d5105d0 to 5700897 Compare May 14, 2021 01:50
Copy link
Member

@robdiciuccio robdiciuccio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice test enhancements!

@robdiciuccio robdiciuccio merged commit 6d9d362 into apache:master May 14, 2021
@robdiciuccio robdiciuccio deleted the benjreinhart/fab-registration branch May 14, 2021 19:49
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* chore: use before_request hook for dynamic routes

* Shorten hook names

* Introduce with_feature_flags and update thumbnail tests

* Disable test that fails in CI but not locally

* Add test for reports
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* chore: use before_request hook for dynamic routes

* Shorten hook names

* Introduce with_feature_flags and update thumbnail tests

* Disable test that fails in CI but not locally

* Add test for reports
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* chore: use before_request hook for dynamic routes

* Shorten hook names

* Introduce with_feature_flags and update thumbnail tests

* Disable test that fails in CI but not locally

* Add test for reports
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants