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

feat(filter-set): Add filterset resource #14015

Merged
merged 73 commits into from
Sep 23, 2021

Conversation

ofekisr
Copy link
Contributor

@ofekisr ofekisr commented Apr 8, 2021

SUMMARY

this is the second milestone in the filter set feature.
it is about managing a scope for filter set either Dashboard/User

  1. Dashboard scoped filter sets will be available for all dashboard permitted user and will be maintained with admins dashboard owners
  2. User scoped filter sets will be private for each user separately

implementation:
a subset API of dashboard "/<dashboard_id>/filtersets" for managing a dashboard

notes:
future milestone will deal with sharing and collaborating on filter-sets

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

tests suites
create_api_tests.py
delete_api_tests.py
get_api_tests.py
update_api_tests.py

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • [X ] Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@ofekisr ofekisr requested a review from a team as a code owner April 8, 2021 08:15
@ofekisr ofekisr marked this pull request as draft April 8, 2021 08:20
@amitmiran137 amitmiran137 changed the title Add filterset resource feat(filter-set): Add filterset resource Apr 8, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2021

⚠️ @ofekisr Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

@amitmiran137 amitmiran137 marked this pull request as ready for review April 13, 2021 14:16
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

First pass, left some comments

superset/dashboards/filter_sets/api.py Outdated Show resolved Hide resolved
superset/dashboards/filter_sets/api.py Outdated Show resolved Hide resolved
superset/dashboards/filter_sets/api.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

⚠️ @ofekisr Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

@ofekisr ofekisr closed this Apr 14, 2021
@ofekisr ofekisr reopened this Apr 14, 2021
@ofekisr ofekisr closed this Apr 14, 2021
@ofekisr ofekisr reopened this Apr 14, 2021
@ofekisr ofekisr requested a review from amitmiran137 April 14, 2021 12:20
@amitmiran137 amitmiran137 requested a review from a team April 14, 2021 14:44
@github-actions
Copy link
Contributor

⚠️ @ofekisr Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

2 similar comments
@github-actions
Copy link
Contributor

⚠️ @ofekisr Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

@github-actions
Copy link
Contributor

⚠️ @ofekisr Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

First pass comments

superset/commands/exceptions.py Show resolved Hide resolved
not_found = CommandException
not_found: Type[CommandException] = CommandException
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is needed

superset/common/request_contexed_based.py Outdated Show resolved Hide resolved
superset/dashboards/filter_sets/commands/base.py Outdated Show resolved Hide resolved
superset/models/dashboard.py Outdated Show resolved Hide resolved
superset/models/filter_set.py Outdated Show resolved Hide resolved
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2021

⚠️ @ofekisr Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

A few quick questions, will review/test more tomorrow

@github-actions
Copy link
Contributor

⚠️ @ofekisr Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the hard work! Looking forward to the next iteration!

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Oh, I just noticed the migration down revision needs to be updated!

@amitmiran137 amitmiran137 merged commit 84f7614 into apache:master Sep 23, 2021
@amitmiran137 amitmiran137 deleted the feat/fileter_set branch September 23, 2021 08:28
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@stevetracvc
Copy link
Contributor

Crap, I did this myself already (although mine is way more simple/naive). Any idea when the UI might get merged in? My names apparently conflict with these and break Superset, so if it's going to be a while I'll rename all of my stuff, and I might be able to help a bit with the UI.

Thanks!

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* Add filterset resource

* fix: fix pre-commit

* add tests

* add tests and fixes based of failures

* Fix pre-commit errors

* chore init filterset resource under ff constraint

* Fix migration conflicts

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* add tests and fixes based of failures

* Fix missing license

* fix down revision

* update down_revision

* fix: update down_revision

* chore: add description to migration

* fix: type

* refactor: is_user_admin

* fix: use get_public_role

* fix: move import to the relevant location

* chore: add openSpec api schema

* chore: cover all openspec API

* fix: pre-commit and lint

* fix: put and post schemas

* fix: undo superset_test_config.py

* fix: limit filterSetsApi to include_route_methods = {"get_list", "put", "post", "delete"}

* renaming some params

* chore: add debug in test config

* fix: rename database to different name

* fix: try to make conftest.py harmless

* fix: pre-commit

* fix: new down_revision ref

* fix: bad ref

* fix: bad ref 2

* fix: bad ref 3

* fix: add api in initiatior

* fix: open spec

* fix: convert name to str to include int usecases

* fix: pylint

* fix: pylint

* Update superset/common/request_contexed_based.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

* chore: resolve PR comments

* chore: resolve PR comments

* chore: resolve PR comments

* fix failed tests

* fix pylint

* Update conftest.py

* chore remove BaseCommand to remove abstraction

* chore remove BaseCommand to remove abstraction

* chore remove BaseCommand to remove abstraction

* chore remove BaseCommand to remove abstraction

* chore fix migration

Co-authored-by: Ofeknielsen <ofek.israel@nieslen.com>
Co-authored-by: amitmiran137 <amit.miran@nielsen.com>
Co-authored-by: Amit Miran <47772523+amitmiran137@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* Add filterset resource

* fix: fix pre-commit

* add tests

* add tests and fixes based of failures

* Fix pre-commit errors

* chore init filterset resource under ff constraint

* Fix migration conflicts

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* Fix pylint and migrations issues

* add tests and fixes based of failures

* Fix missing license

* fix down revision

* update down_revision

* fix: update down_revision

* chore: add description to migration

* fix: type

* refactor: is_user_admin

* fix: use get_public_role

* fix: move import to the relevant location

* chore: add openSpec api schema

* chore: cover all openspec API

* fix: pre-commit and lint

* fix: put and post schemas

* fix: undo superset_test_config.py

* fix: limit filterSetsApi to include_route_methods = {"get_list", "put", "post", "delete"}

* renaming some params

* chore: add debug in test config

* fix: rename database to different name

* fix: try to make conftest.py harmless

* fix: pre-commit

* fix: new down_revision ref

* fix: bad ref

* fix: bad ref 2

* fix: bad ref 3

* fix: add api in initiatior

* fix: open spec

* fix: convert name to str to include int usecases

* fix: pylint

* fix: pylint

* Update superset/common/request_contexed_based.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

* chore: resolve PR comments

* chore: resolve PR comments

* chore: resolve PR comments

* fix failed tests

* fix pylint

* Update conftest.py

* chore remove BaseCommand to remove abstraction

* chore remove BaseCommand to remove abstraction

* chore remove BaseCommand to remove abstraction

* chore remove BaseCommand to remove abstraction

* chore fix migration

Co-authored-by: Ofeknielsen <ofek.israel@nieslen.com>
Co-authored-by: amitmiran137 <amit.miran@nielsen.com>
Co-authored-by: Amit Miran <47772523+amitmiran137@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.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 dashboard:filtersets Related to the filtersets of the Dashboard risk:db-migration PRs that require a DB migration size/XXL 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants