Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(bootstrap-data): always check flashes #22659

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

villebro
Copy link
Member

@villebro villebro commented Jan 10, 2023

SUMMARY

#21018 and #21439 added memoization of common bootstrap data to avoid repetitive calls to regenerate user perms in bootstrap data. However, this had the side-effect of also using cached toasts, causing toasts to either 1) not display 2) display with a lag and not clear.

This breaks out the expensive logic from common_bootstrap_payload into a separate function cached_common_bootstrap_data which is memoized, and always checks flashed messages to ensure that they always accurately reflect the current toasts.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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 10, 2023

Codecov Report

Merging #22659 (12fb98c) into master (5b2ca97) will decrease coverage by 1.26%.
The diff coverage is 87.14%.

❗ Current head 12fb98c differs from pull request most recent head 1c26db0. Consider uploading reports for the commit 1c26db0 to get more accurate results

@@            Coverage Diff             @@
##           master   #22659      +/-   ##
==========================================
- Coverage   66.88%   65.62%   -1.27%     
==========================================
  Files        1859     1869      +10     
  Lines       71103    71524     +421     
  Branches     7782     7814      +32     
==========================================
- Hits        47557    46937     -620     
- Misses      21520    22546    +1026     
- Partials     2026     2041      +15     
Flag Coverage Δ
mysql ?
postgres 78.17% <95.13%> (+0.13%) ⬆️
presto ?
python 78.24% <95.13%> (-2.81%) ⬇️
sqlite 76.56% <95.13%> (?)
unit ?

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

Impacted Files Coverage Δ
superset/views/core.py 74.81% <ø> (+0.37%) ⬆️
superset/views/dashboard/views.py 66.27% <ø> (ø)
superset-frontend/src/views/routes.tsx 55.00% <50.00%> (-0.27%) ⬇️
superset/dao/base.py 92.78% <70.00%> (-2.62%) ⬇️
...ews/CRUD/rowlevelsecurity/RowLevelSecurityList.tsx 72.54% <72.54%> (ø)
...ws/CRUD/rowlevelsecurity/RowLevelSecurityModal.tsx 73.39% <73.39%> (ø)
...ConfigModal/FiltersConfigForm/FilterScope/utils.ts 86.20% <87.50%> (-0.84%) ⬇️
...uperset/row_level_security/commands/bulk_delete.py 88.00% <88.00%> (ø)
superset/connectors/sqla/views.py 87.96% <90.90%> (+0.66%) ⬆️
superset/row_level_security/commands/update.py 91.66% <91.66%> (ø)
... and 86 more

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

@villebro villebro force-pushed the villebro/bootstrap-flash branch from 3bb7147 to eaf7a3d Compare January 10, 2023 10:54
Copy link
Member

@bkyryliuk bkyryliuk left a comment

Choose a reason for hiding this comment

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

thanks for the fix, LG%small nit

@@ -77,7 +77,7 @@ def embedded(
)

bootstrap_data = {
"common": common_bootstrap_payload(g.user),
"common": common_bootstrap_payload(),
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to keep user as an argument here, last time I made a bug and assumed that common_bootstrap_payload was user agnostic as it did not take any params

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I'll reintroduce it 👍

@pull-request-size pull-request-size bot added size/S and removed size/M labels Jan 10, 2023
@villebro villebro merged commit 73e53fa into apache:master Jan 10, 2023
@villebro villebro deleted the villebro/bootstrap-flash branch January 16, 2023 12:46
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants