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.

Sign up for GitHub

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

Jump to bottom

fix: Add extra check to loggerMiddleware #14492

Merged
merged 4 commits into from
May 7, 2021
Merged

fix: Add extra check to loggerMiddleware #14492

merged 4 commits into from
May 7, 2021

Conversation

geido
Copy link
Member

@geido geido commented May 5, 2021

SUMMARY

I could not reproduce the issue described in #13158. However, I am adding this extra check in the part of the code that was causing the problem originally in case it might help to stop the issue from happening in other places.

CURRENT MASTER BEHAVIOR

Watch the video

TEST PLAN

  1. Create a new dashboard or open an existing one
  2. Add new tabs
  3. Without moving focus to the tab you are willing to delete, click on the "X" to delete it
  4. Confirm the deletion
  5. Observe that the modal is closed and the tab deleted successfully

ADDITIONAL INFORMATION

@geido
Copy link
Member Author

geido commented May 5, 2021

@junlincc @rusackas

@codecov
Copy link

codecov bot commented May 5, 2021 •
edited
Loading

Codecov Report

Merging #14492 (4111ba2) into master (96289e9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14492   +/-   ##
=======================================
  Coverage   77.14%   77.14%           
=======================================
  Files         958      958           
  Lines       48241    48241           
  Branches     5636     5636           
=======================================
  Hits        37217    37217           
  Misses      10823    10823           
  Partials      201      201
Flag Coverage Δ
javascript 72.01% <100.00%> (ø)

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

Impacted Files Coverage Δ
...perset-frontend/src/middleware/loggerMiddleware.js 67.50% <100.00%> (ø)

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 96289e9...4111ba2. Read the comment docs.

@rusackas
Copy link
Member

rusackas commented May 5, 2021

Fixes #13158

@rusackas rusackas self-requested a review May 5, 2021 22:56
@rusackas
Copy link
Member

rusackas commented May 5, 2021

/testenv up

@rusackas
Copy link
Member

rusackas commented May 5, 2021

@simcha90 since you were in the loop on the original issue, please review if you're able. Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2021

@rusackas Ephemeral environment spinning up at http://18.236.70.42:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

simcha90
simcha90 reviewed May 6, 2021
View reviewed changes
superset-frontend/src/middleware/loggerMiddleware.js Outdated
if (
eventData.target_id &&
dashboardLayout.present &&
dashboardLayout.present[eventData.target_id]
Copy link
Contributor

Choose a reason for hiding this comment

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

can we simplify?

eventData.target_id &&
    dashboardLayout.present?.[eventData.target_id]

Copy link
Member

Choose a reason for hiding this comment

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

good catch - should it be dashboardLayout?.present?.[eventData.target_id]

@rusackas rusackas merged commit 10a53b7 into apache:master May 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2021

Ephemeral environment shutdown and build artifacts deleted.

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@geido @rusackas
fix: Add extra check to loggerMiddleware (apache#14492)
7b1f95f 
* Add extra check

* Update superset-frontend/src/middleware/loggerMiddleware.js

Co-authored-by: Evan Rusackas <evan@preset.io>

* Fix linting issue

Co-authored-by: Evan Rusackas <evan@preset.io>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@geido @rusackas
fix: Add extra check to loggerMiddleware (apache#14492)
f92879f 
* Add extra check

* Update superset-frontend/src/middleware/loggerMiddleware.js

Co-authored-by: Evan Rusackas <evan@preset.io>

* Fix linting issue

Co-authored-by: Evan Rusackas <evan@preset.io>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@geido @rusackas
fix: Add extra check to loggerMiddleware (apache#14492)
b26e4d3 
* Add extra check

* Update superset-frontend/src/middleware/loggerMiddleware.js

Co-authored-by: Evan Rusackas <evan@preset.io>

* Fix linting issue

Co-authored-by: Evan Rusackas <evan@preset.io>
@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
Reviewers

@villebro villebro villebro left review comments

@simcha90 simcha90 simcha90 left review comments

@rusackas rusackas rusackas approved these changes

@michael-s-molina michael-s-molina michael-s-molina approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@geido @rusackas @villebro @simcha90 @michael-s-molina @mistercrunch