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

fix: moved alerts and reports default values to config #22880

Merged
merged 10 commits into from
Mar 31, 2023

Conversation

Always-prog
Copy link
Contributor

@Always-prog Always-prog commented Jan 27, 2023

SUMMARY

Typically, users do not verify the working timeout and default repetition for creating alerts or reports. As a result, if a Superset administrator wants to change the default values for these options, they need to modify the code. To address this issue, I submitted a pull request that moves these default values to the config file.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #22880 (5157886) into master (260ac40) will decrease coverage by 0.01%.
The diff coverage is 72.62%.

❗ Current head 5157886 differs from pull request most recent head a150689. Consider uploading reports for the commit a150689 to get more accurate results

@@            Coverage Diff             @@
##           master   #22880      +/-   ##
==========================================
- Coverage   67.40%   67.39%   -0.01%     
==========================================
  Files        1876     1880       +4     
  Lines       72084    72229     +145     
  Branches     7857     7882      +25     
==========================================
+ Hits        48587    48678      +91     
- Misses      21480    21527      +47     
- Partials     2017     2024       +7     
Flag Coverage Δ
hive ?
javascript 53.91% <68.75%> (+0.15%) ⬆️
mysql 78.24% <61.76%> (-0.02%) ⬇️
postgres 78.31% <61.76%> (-0.02%) ⬇️
presto 52.62% <52.94%> (-0.01%) ⬇️
python 81.96% <100.00%> (-0.15%) ⬇️
sqlite 76.62% <58.82%> (-0.02%) ⬇️
unit 52.51% <91.17%> (+0.08%) ⬆️

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

Impacted Files Coverage Δ
...hart-controls/src/operators/timeCompareOperator.ts 100.00% <ø> (ø)
...-chart-controls/src/sections/advancedAnalytics.tsx 14.28% <ø> (ø)
...trols/components/ColumnConfigControl/constants.tsx 100.00% <ø> (ø)
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...i-chart-controls/src/utils/expandControlConfig.tsx 100.00% <ø> (ø)
...perset-ui-core/src/chart/components/SuperChart.tsx 97.22% <ø> (ø)
...ages/superset-ui-core/src/math-expression/index.ts 100.00% <ø> (ø)
...ges/superset-ui-core/src/query/buildQueryObject.ts 100.00% <ø> (ø)
...ges/superset-ui-core/src/query/normalizeOrderBy.ts 100.00% <ø> (ø)
...superset-ui-core/src/query/types/PostProcessing.ts 100.00% <ø> (ø)
... and 133 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

Nice improvement 👍 LGTM and tested to work as expected with one minor naming comment.

@Always-prog
Copy link
Contributor Author

@villebro Hello!
I fixed the passing test cases because the test cases were running without the python config. We still need to have default values in the code. However, if they are declared in config, they will be used.
Can you check my code?

@villebro
Copy link
Member

villebro commented Jan 30, 2023

@villebro Hello!
I fixed the passing test cases because the test cases were running without the python config. We still need to have default values in the code. However, if they are declared in config, they will be used.
Can you check my code?

@Always-prog yeah, this is a bit of a problem when the frontend assets don't have context on regarding the python config values. You can probably either mock the values on the test (more work) or just add the same defaults on the frontend code path (less work). I'm ok with both, as in practice they won't affect this functionality.

@Always-prog
Copy link
Contributor Author

@villebro Hello!
I fixed the passing test cases because the test cases were running without the python config. We still need to have default values in the code. However, if they are declared in config, they will be used.
Can you check my code?

@Always-prog yeah, this is a bit of a problem when the frontend assets don't have context on regarding the python config values. You can probably either mock the values on the test (more work) or just add the same defaults on the frontend code path (less work). I'm ok with both, as in practice they won't affect this functionality.

I added default values in code.

@villebro
Copy link
Member

villebro commented Feb 1, 2023

@Always-prog can you rebase the PR? That might help resolve that failed test.

@Always-prog
Copy link
Contributor Author

Hello @villebro and @rusackas!
I have fixed the frontend tests and they are now passing. Could you please review my code once more?

@Always-prog
Copy link
Contributor Author

I need to create a new PR for the fix?
There are many commits that are not mine, due to rebase.

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.

probably something went wrong with the rebase

@Always-prog Always-prog force-pushed the feat/default-alert-config-in-conf branch from 4c6997c to f01fc4b Compare February 7, 2023 19:29
@Always-prog
Copy link
Contributor Author

Always-prog commented Feb 7, 2023

probably something went wrong with the rebase

I reverted rebase commit.

@Always-prog
Copy link
Contributor Author

On my fork all CI test has passed.
image

Maybe need to restart them @dpgaspar?

@villebro
Copy link
Member

villebro commented Feb 8, 2023

@dpgaspar I believe the rebasing problems have been resolved, can you take another look?

@rusackas rusackas requested review from dpgaspar and removed request for dpgaspar February 8, 2023 19:03
@Always-prog
Copy link
Contributor Author

@dpgaspar Hello!
Can you please review py PR again?

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 this and apologies for letting this get stale

@villebro villebro dismissed dpgaspar’s stale review March 31, 2023 08:58

The requested changes have been addressed

@villebro villebro merged commit 09757dc into apache:master Mar 31, 2023
@Always-prog
Copy link
Contributor Author

Thanks for approving!

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 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/M 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants