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: list view filters for Query History #11702

Merged
merged 9 commits into from
Nov 30, 2020

Conversation

nytai
Copy link
Member

@nytai nytai commented Nov 14, 2020

SUMMARY

  • Add filters for Query History list view, including a date range filter.
  • upgrade antd and moment due to incompatible versions in the types for the DatePicker

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:
Screen Shot 2020-11-17 at 10 54 02 PM
Screen Shot 2020-11-17 at 10 54 17 PM
Screen Shot 2020-11-17 at 10 54 35 PM

TEST PLAN

  • unit tested
  • manual testing:
    • filter by Database
    • filter by State
    • filter by User
    • filter by Time Range
    • filter by query text

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Nov 14, 2020

Codecov Report

Merging #11702 (4475013) into master (3cd94d6) will decrease coverage by 3.82%.
The diff coverage is 85.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11702      +/-   ##
==========================================
- Coverage   67.56%   63.74%   -3.83%     
==========================================
  Files         918      924       +6     
  Lines       44755    44827      +72     
  Branches     4237     4253      +16     
==========================================
- Hits        30240    28575    -1665     
- Misses      14412    16075    +1663     
- Partials      103      177      +74     
Flag Coverage Δ
cypress ?
javascript 63.03% <85.15%> (+0.14%) ⬆️
python 64.15% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...tend/src/views/CRUD/annotation/AnnotationModal.tsx 66.66% <ø> (+0.52%) ⬆️
...tend/src/components/ListView/Filters/DateRange.tsx 52.17% <52.17%> (ø)
...t-frontend/src/views/CRUD/data/query/QueryList.tsx 72.11% <77.77%> (-0.62%) ⬇️
...rontend/src/components/ListView/Filters/Search.tsx 86.66% <86.66%> (ø)
...frontend/src/components/ListView/Filters/index.tsx 86.95% <86.95%> (ø)
superset-frontend/src/components/ListView/utils.ts 84.61% <92.30%> (+3.74%) ⬆️
...t-frontend/src/components/ListView/Filters/Base.ts 100.00% <100.00%> (ø)
...rontend/src/components/ListView/Filters/Select.tsx 97.56% <100.00%> (ø)
superset-frontend/src/components/ListView/types.ts 100.00% <100.00%> (ø)
superset-frontend/src/views/CRUD/types.ts 100.00% <100.00%> (ø)
... and 183 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 3cd94d6...4475013. Read the comment docs.

@nytai nytai linked an issue Nov 18, 2020 that may be closed by this pull request
@nytai nytai changed the title feat: list view filters to Query History feat: list view filters for Query History Nov 24, 2020
@nytai nytai marked this pull request as ready for review November 24, 2020 19:47
@@ -128,7 +132,7 @@ function QueryList({ addDangerToast, addSuccessToast }: QueryListProps) {
...commonMenuData,
};

const initialSort = [{ id: 'changed_on', desc: true }];
const initialSort = [{ id: 'start_time', desc: true }];
Copy link
Member

Choose a reason for hiding this comment

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

nit: make this an enum initSortId at the top

Copy link
Member Author

Choose a reason for hiding this comment

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

I converted all the column names to an enum

.filter(f => {
if (typeof f.value === 'undefined') return false;
if (Array.isArray(f.value) && !f.value.length) return false;
return true;
Copy link
Member

Choose a reason for hiding this comment

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

how about

.filter(f => !((typeof f.value === 'undefined') || (Array.isArray(f.value) && !f.value.length)))

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you actually find that more readable? These are two separate conditions for filtering out a value, it makes sense to keep them seperate imo

Copy link
Member

@eschutho eschutho Nov 30, 2020

Choose a reason for hiding this comment

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

I do, since you're trying to see if it has a value on each type, but it's just preference. It mainly feels redundant to return an explicit boolean for a boolean conditional.
How about something like this for readability?

let hasValue = true;
hasValue = !(typeof f.value === 'undefined');
hasValue = !(Array.isArray(f.value) && !f.value.length);
return hasValue;

if that doesn't feel like an improvement, we can go with what you have. It's not anything major.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, updated. I have less of an issue with it now after the prettier formatting.

Header: t('Search by query text'),
id: 'sql',
input: 'search',
operator: 'ct',
Copy link
Member

Choose a reason for hiding this comment

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

second @hughhhh's comment on these strings, too.. they could prob all benefit from enums as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created an enum FilterOperators with more descriptive names for each operator.

@@ -89,3 +99,24 @@ export interface FetchDataConfig {
export interface InternalFilter extends FilterValue {
Header?: string;
}

export enum FilterOperators {
starts_with = 'sw',
Copy link
Member

Choose a reason for hiding this comment

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

could we make this snakeCase?

Copy link
Member Author

Choose a reason for hiding this comment

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

converted to camelCase since that's the js standard

@nytai
Copy link
Member Author

nytai commented Nov 30, 2020

merging this before I get any more comments! 😃

@nytai nytai merged commit 0117247 into apache:master Nov 30, 2020
@nytai nytai deleted the tai/query-history-filters branch November 30, 2020 22:21
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.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/XL 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL search query search string shouldn't be case sensititive
6 participants