fix(chart list): Facepile shows correct users when saving chart properties #33392
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
|
/korbit-review. |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Unsafe Name Splitting Logic ▹ view | ✅ Fix detected |
 |
Inefficient Owner Name Transformation ▹ view | 🧠 Not in scope |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/explore/components/PropertiesModal/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| const selectedOwnersArray = ensureIsArray(selectedOwners); | ||
| const newOwners = selectedOwnersArray.map((owner: any) => { | ||
| const [first_name, ...last_name_parts] = owner.label.split(' '); | ||
| return { | ||
| id: owner.value, | ||
| first_name, | ||
| last_name: last_name_parts.join(' '), | ||
| }; | ||
| }); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // convert the tags to the format used in the selectOwners back to original redux format | ||
| const selectedOwnersArray = ensureIsArray(selectedOwners); | ||
| const newOwners = selectedOwnersArray.map((owner: any) => { | ||
| const [first_name, ...last_name_parts] = owner.label.split(' '); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Complex Name Parsing Without Types ▹ view | 🧠 Not in scope |
 |
Embedded Name Parsing Logic ▹ view | 🧠 Not in scope |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/explore/components/PropertiesModal/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| const newOwners = selectedOwnersArray.map((owner: any) => { | ||
| const nameParts = owner.label.split(' '); | ||
| const first_name = nameParts[0] || ''; | ||
| const last_name = nameParts.length > 1 ? nameParts.slice(1).join(' ') : ''; | ||
| return { | ||
| id: owner.value, | ||
| first_name, | ||
| last_name, | ||
| }; | ||
| }); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I think it's a pretty specific case where this one component handles owners in a specific way which when trying to update redux, the "shape" of the object is different, so i have to translate it back to the same shape which originally existed.
There was a problem hiding this comment.
I see - since this is specifically about matching the Redux state shape in this component, keeping the transformation logic local makes sense. Thanks for clarifying the context.
| const nameParts = owner.label.split(' '); | ||
| const first_name = nameParts[0] || ''; | ||
| const last_name = nameParts.length > 1 ? nameParts.slice(1).join(' ') : ''; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
This is probably OK, but I'm slightly nervous about things like middle names. This would mean if we have Michael J. Fox sign up, it'd be "Michael" "J. Fox" instead of "Michael J." "Fox."
Is there other first/last name logic currently in place, or is this a new concept?
There was a problem hiding this comment.
Valid concern about middle name handling. Before suggesting changes, could you clarify if there's existing first/last name parsing logic elsewhere in the codebase we should match? If so, we should reuse that pattern instead of introducing a new one.
There was a problem hiding this comment.
I thought about that too actually. "Updating" only happens around user ID's. As for display, I'm on the same line as you where it's "probably okay".
There is no real existing "full_name" split. I tried checking out how dashboards/dataset list handles it, but they're both different :(.
I dont think there is a true way of doing it unless we retrieve it from the backend.
I have tried doing something like this in the backend to "pass ownership data" along with the put request
and then just removing the modification to the owner field and using just the returned owner from the put request, but I'm not sure that's the best design either...
I've also tried seeing if I could sort of "cache" the load options and see if it was split there when im getting the eligible users to add as owners, but that won't work either because it's only item.text and makes no differentiation of first and last names.
There was a problem hiding this comment.
There is no real existing "full_name" split. I tried checking out how dashboards/dataset list handles it, but they're both different :(.
This is what I was worried about. If there's any logic to be DRY'd up (moved to a util or something) that would be preferable than implementing another bit of inline logic.
ethan-l-geotab
force-pushed
the
facepile-fix
branch
from
46bc195 to
b71c413
Compare
rusackas
added
the
review:checkpoint
SUMMARY
Looking to fix: #31592.
It's reproducible on all the test-env up environments. Add "Superset Admin" as user on a chart and then save shows the undefined undefined.
I just update the redux with the owner list back in it's original "shape".
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
first half is the broken, second half is working
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION