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(reports): Owners no longer showing undefined in reports #17223

Merged
merged 5 commits into from
Oct 29, 2021

Conversation

lyndsiWilliams
Copy link
Member

@lyndsiWilliams lyndsiWilliams commented Oct 25, 2021

SUMMARY

"undefined undefined" was showing up in the owners list of a report every time it was edited. This was resolved by setting the owners data to match the state in alert (straight from redux state) instead of resource (coming from the alert fetch logic). Since resource is manipulated in the update, sometimes owners will be a list of owner IDs with no other data. This caused the label to return "undefined" for first_name and last_name in the owners object when it's updated. alert's state, however, consistently has the entire owners object. It now updates appropriately when updating the owners on a report.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

d08ac168-13e7-44b3-9fab-52994186c68b-hs_file_upload-report_bug (1)

AFTER

definedOwners

TESTING INSTRUCTIONS

  • Create and save a report
  • Edit the report and add another owner
  • Save changes
  • Go back and edit report again
  • Observe that all owners are displaying correctly, no instances of "undefined undefined"

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

@lyndsiWilliams lyndsiWilliams marked this pull request as ready for review October 26, 2021 19:19
@lyndsiWilliams lyndsiWilliams changed the title fix(reports): Owners no longer showing undefined in reports [WIP]fix(reports): Owners no longer showing undefined in reports Oct 26, 2021
@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #17223 (e042174) into master (029ed90) will increase coverage by 0.09%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17223      +/-   ##
==========================================
+ Coverage   76.91%   77.01%   +0.09%     
==========================================
  Files        1039     1037       -2     
  Lines       55566    55636      +70     
  Branches     7570     7598      +28     
==========================================
+ Hits        42740    42848     +108     
+ Misses      12576    12538      -38     
  Partials      250      250              
Flag Coverage Δ
javascript 71.14% <66.66%> (+0.20%) ⬆️

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

Impacted Files Coverage Δ
...frontend/src/views/CRUD/alert/AlertReportModal.tsx 60.70% <66.66%> (-0.44%) ⬇️
...erset-frontend/src/components/Pagination/index.tsx 72.72% <0.00%> (-27.28%) ⬇️
...nativeFilters/FiltersConfigModal/Footer/Footer.tsx 90.90% <0.00%> (-9.10%) ⬇️
...Filters/FilterBar/FilterControls/FilterControl.tsx 94.87% <0.00%> (-5.13%) ⬇️
...tend/src/filters/components/Select/controlPanel.ts 58.33% <0.00%> (-3.21%) ⬇️
superset/reports/api.py 85.92% <0.00%> (-2.54%) ⬇️
...frontend/src/dashboard/components/Header/index.jsx 68.30% <0.00%> (-0.41%) ⬇️
...et-frontend/src/components/TableSelector/index.tsx 74.28% <0.00%> (-0.25%) ⬇️
...tend/src/explore/components/ExploreChartHeader.jsx 6.84% <0.00%> (-0.20%) ⬇️
... and 57 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 029ed90...e042174. Read the comment docs.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Oct 26, 2021
@lyndsiWilliams lyndsiWilliams changed the title [WIP]fix(reports): Owners no longer showing undefined in reports fix(reports): Owners no longer showing undefined in reports Oct 26, 2021
superset/reports/api.py Outdated Show resolved Hide resolved
superset/reports/schemas.py Outdated Show resolved Hide resolved
@betodealmeida
Copy link
Member

betodealmeida commented Oct 27, 2021

Talked offline. We shouldn't modify the API to solve this, since it's strictly a frontend problem — the GET request before an edit already receives all the owners information correctly.

@eschutho
Copy link
Member

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Looks good!

It would be nice to fix resource so that it's behaving consistently, but I think this is ok for now.

@lyndsiWilliams
Copy link
Member Author

Looks good!

It would be nice to fix resource so that it's behaving consistently, but I think this is ok for now.

Agreed, state could use some cleanup here to avoid future complications.

@betodealmeida betodealmeida merged commit 456efc0 into apache:master Oct 29, 2021
@lyndsiWilliams lyndsiWilliams deleted the lyndsi/fix-undefined-owners branch October 29, 2021 17:43
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
* Owners no longer showing undefined

* Reverted owners change

* removed print

* Owners now defined - frontend only solution!

* Remove arbitrary space
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.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 preset-io size/S 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants