You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "ethan-l-geotab (via GitHub)" <gi...@apache.org> on 2025/05/08 17:47:29 UTC
[PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
ethan-l-geotab opened a new pull request, #33392:
URL: https://github.com/apache/superset/pull/33392
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Looking to fix: https://github.com/apache/superset/issues/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
<!--- Skip this if not applicable -->
first half is the broken, second half is working
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: https://github.com/apache/superset/issues/31592
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2129129596
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
@rusackas so i was thinking about this again, and noticed that for datasets, there are actually multiple get requests. I was wondering if it was just better to return the data inside the put request as a response, or do it in a second get request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2277226639
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
shouldn't the PUT return the new object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-2863819878
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`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](f100494b-427c-406d-a046-2c9d9361d934)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2299410751
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -192,20 +192,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
Review Comment:
DRY = "don't repeat yourself" - i.e. set a variable for the endpoint and use it twice rather than repeating the string :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2294670250
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -192,20 +192,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
Review Comment:
DRY up that endpoint top-of try block
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2080210495
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,24 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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 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,
+ };
+ });
Review Comment:
### Embedded Name Parsing Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The owner name parsing logic is directly embedded in the submit handler, making it difficult to reuse and test. This logic should be extracted into a separate utility function.
###### Why this matters
Having business logic directly in event handlers reduces code reusability and makes it harder to maintain or modify the name parsing logic if it needs to be used elsewhere.
###### Suggested change ∙ *Feature Preview*
Extract the logic into a separate utility function:
```typescript
const parseOwnerName = (owner: { label: string; value: number }) => {
const nameParts = owner.label.split(' ');
return {
id: owner.value,
first_name: nameParts[0] || '',
last_name: nameParts.length > 1 ? nameParts.slice(1).join(' ') : ''
};
};
// Usage in component
const newOwners = selectedOwnersArray.map(parseOwnerName);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e46bad0-2e9a-456d-b8e0-6bd3e7b7c71c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e46bad0-2e9a-456d-b8e0-6bd3e7b7c71c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e46bad0-2e9a-456d-b8e0-6bd3e7b7c71c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e46bad0-2e9a-456d-b8e0-6bd3e7b7c71c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e46bad0-2e9a-456d-b8e0-6bd3e7b7c71c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d9e444ca-2cf2-4fdc-958f-d869452f076b -->
[](d9e444ca-2cf2-4fdc-958f-d869452f076b)
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,24 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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 nameParts = owner.label.split(' ');
+ const first_name = nameParts[0] || '';
+ const last_name = nameParts.length > 1 ? nameParts.slice(1).join(' ') : '';
Review Comment:
### Complex Name Parsing Without Types <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The name parsing logic is complex and lacks a clear type definition for the owner parameter, making the transformation from label to first/last name hard to follow.
###### Why this matters
Without proper typing and a dedicated function, this name parsing logic is brittle and its purpose isn't immediately clear to future maintainers.
###### Suggested change ∙ *Feature Preview*
```typescript
interface Owner {
label: string;
value: number;
}
function parseOwnerName(owner: Owner): { first_name: string; last_name: string } {
const nameParts = owner.label.split(' ');
return {
first_name: nameParts[0] || '',
last_name: nameParts.length > 1 ? nameParts.slice(1).join(' ') : ''
};
}
const newOwners = selectedOwnersArray.map((owner: Owner) => ({
id: owner.value,
...parseOwnerName(owner)
}));
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8dc4544e-e622-477b-9a61-6e1b7e850401/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8dc4544e-e622-477b-9a61-6e1b7e850401?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8dc4544e-e622-477b-9a61-6e1b7e850401?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8dc4544e-e622-477b-9a61-6e1b7e850401?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8dc4544e-e622-477b-9a61-6e1b7e850401)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a57ef742-4b39-475b-9e70-1d8d5ddf1145 -->
[](a57ef742-4b39-475b-9e70-1d8d5ddf1145)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2263822920
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
I don't have anything against an extra call, as long as it's not something that's going to happen a million times, like in a CRUD view (for obvious performance reasons).
@michael-s-molina @justinpark @mistercrunch can provide a better second opinion here, perhaps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "legal-s (via GitHub)" <gi...@apache.org>.
legal-s commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3479533034
Hi,
Have you got news for merge this pull request ?
We need this correction.
Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3169064048
Hey! Sorry I went AWOL on the thread here :) I think we can likely merge this, pending a second opinion on the extra API call (probably not an issue). The main blocker at the moment is that there's a rebase needed, which I hope isn't too painful with the changes that have taken place in the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2294677200
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
are you sure the PUT doesn't actually returns the payload already? looking at https://superset.apache.org/docs/api it seems like it might
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2298702714
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -192,20 +192,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
Review Comment:
Hey, sorry I'm not sure I'm catching the meaning of this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2080186719
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,22 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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(' ');
+ return {
+ id: owner.value,
+ first_name,
+ last_name: last_name_parts.join(' '),
+ };
+ });
Review Comment:
### Inefficient Owner Name Transformation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
String splitting and joining operations are performed unnecessarily on every save operation to transform owner names back to the redux format.
###### Why this matters
This transformation could be expensive for large sets of owners and is being performed every time the chart properties are saved, when it could be handled once during initial data loading or maintained in the correct format throughout.
###### Suggested change ∙ *Feature Preview*
Store owners in the desired format from the beginning or create a mapping when owners are first fetched. Example:
```typescript
const ownerMap = chart?.owners?.reduce((acc, owner) => {
acc[owner.id] = {
first_name: owner.first_name,
last_name: owner.last_name
};
return acc;
}, {});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3509b88c-43b2-496a-95a3-5d955a6c3566/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3509b88c-43b2-496a-95a3-5d955a6c3566?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3509b88c-43b2-496a-95a3-5d955a6c3566?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3509b88c-43b2-496a-95a3-5d955a6c3566?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3509b88c-43b2-496a-95a3-5d955a6c3566)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f3e3c061-054f-46bc-9d99-d9de244e5a4c -->
[](f3e3c061-054f-46bc-9d99-d9de244e5a4c)
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,22 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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(' ');
Review Comment:
### Unsafe Name Splitting Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes that the owner's name always has at least a first name and last name, and splits the name based on spaces. This can cause issues with names that don't follow this format.
###### Why this matters
Names that contain only one word, or names with special characters (like hyphens or apostrophes) could be incorrectly parsed, leading to incorrect owner information in the redux state.
###### Suggested change ∙ *Feature Preview*
Add validation and fallback handling for different name formats:
```typescript
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,
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3978e966-ac14-4e9e-9b88-70edcc7d19a1/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3978e966-ac14-4e9e-9b88-70edcc7d19a1?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3978e966-ac14-4e9e-9b88-70edcc7d19a1?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3978e966-ac14-4e9e-9b88-70edcc7d19a1?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3978e966-ac14-4e9e-9b88-70edcc7d19a1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:bf412400-eb0a-4e39-ba46-40b2c8dbd5b1 -->
[](bf412400-eb0a-4e39-ba46-40b2c8dbd5b1)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2080228625
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,24 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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 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,
+ };
+ });
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2082152322
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,24 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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 nameParts = owner.label.split(' ');
+ const first_name = nameParts[0] || '';
+ const last_name = nameParts.length > 1 ? nameParts.slice(1).join(' ') : '';
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2294967499
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
related: https://chatgpt.com/share/68a90a13-5390-8010-b149-6a87d0014de3
Personally would say: got for it, make it such that all 2 PUT/POST/GET all return the same schema as per best practices. If something break, a test should fail or we'll know soon enough.
Might be good to grep/search or add Claude Code or AI to do a deep search in the repo to confirm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2277228077
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
@korbit-ai what do you think? REST api best practices on a PUT, return the new committed object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2288913534
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
@ethan-l-geotab I see that CI is looking good again after your rebase, thanks! I think this part of the thread is the crux of whether we merge it now or not. Is getting/using the response from the PUT call doable? If so, that does seem like the more elegant approach, but if not, I have no personal objection to fixing the bug with the current approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2080224863
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,24 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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 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,
+ };
+ });
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3338491863
Hi @ethan-l-geotab, let me know if you need any help with completing this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "JZ6 (via GitHub)" <gi...@apache.org>.
JZ6 commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3168351776
@rusackas we are running into this issue too, can this pr be merged, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2277230126
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
how do you summon these things!? Which we had our buddy @claude in this repo ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-2895624038
@rusackas Hello, I went with a much more simple approach I saw use by datasets where i just make 1 get request to the resource and then update from the resource from the put request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2080355064
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,24 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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 nameParts = owner.label.split(' ');
+ const first_name = nameParts[0] || '';
+ const last_name = nameParts.length > 1 ? nameParts.slice(1).join(' ') : '';
Review 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...
<img width="701" alt="image" src="https://github.com/user-attachments/assets/04b2e056-4ed4-4914-bfe3-8c37a0d77ebc" />
<img width="544" alt="image" src="https://github.com/user-attachments/assets/1562c78e-a209-4cfa-9b94-4b66ce3fe8fb" />
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3032479950
Hey, can i get any updates on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2277226639
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
should the PUT return the new object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2263820632
##########
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -44,62 +44,125 @@ const createProps = () =>
addSuccessToast: jest.fn(),
}) as PropertiesModalProps;
-fetchMock.get('glob:*/api/v1/chart/318', {
- body: {
- description_columns: {},
- id: 318,
- label_columns: {
- cache_timeout: 'Cache Timeout',
- 'dashboards.dashboard_title': 'Dashboards Dashboard Title',
- 'dashboards.id': 'Dashboards Id',
- description: 'Description',
- 'owners.first_name': 'Owners First Name',
- 'owners.id': 'Owners Id',
- 'owners.last_name': 'Owners Last Name',
- 'owners.username': 'Owners Username',
- params: 'Params',
- slice_name: 'Slice Name',
- viz_type: 'Viz Type',
- },
- result: {
- cache_timeout: null,
- certified_by: 'John Doe',
- certification_details: 'Sample certification',
- dashboards: [
- {
- dashboard_title: 'FCC New Coder Survey 2018',
- id: 23,
+let callCount = 0;
Review Comment:
Using a module-level counter variable can lead to test interdependence and flaky tests. Consider using a more robust approach like jest.fn().mockReturnValueOnce() or resetting the counter in a beforeEach block.
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
Review Comment:
Making two sequential API calls (PUT then GET) could impact performance. Consider if the PUT response can be enhanced to return complete owner data, eliminating the need for the additional GET request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2288906432
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
Hmmm... I thought @korbit-ai should do it. Maybe @copilot or @korbit or @dosu. C'mon bots!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3207510927
Looks technically mergeable. [This](https://github.com/apache/superset/pull/33392#discussion_r2288913534) (or the [bot version](https://github.com/apache/superset/pull/33392#discussion_r2263820617) thereof) seems to be the last standing question.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2294746288
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
Kinda.
put returns:
<img width="227" height="50" alt="image" src="https://github.com/user-attachments/assets/10506a08-75a0-4d23-9451-254ed9f8c88c" />
get returns:
<img width="479" height="56" alt="image" src="https://github.com/user-attachments/assets/a1580226-f331-4e28-bd20-95bc695a327c" />
I think cuz schema is int instead of relationship. I just am not sure if theres other parts of the code that expects the owner list to be returned as a list of ints.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3352595505
Sorry, got distracted with a lot of other stuff. Yes please, any help would be nice, could someone let me know what i have to do to modify whatever schemas or anything else i'd have to do?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2080230511
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,24 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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 nameParts = owner.label.split(' ');
+ const first_name = nameParts[0] || '';
+ const last_name = nameParts.length > 1 ? nameParts.slice(1).join(' ') : '';
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2080233094
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -185,12 +185,24 @@ function PropertiesModal({
body: JSON.stringify(payload),
});
// update the redux state
+ // 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 nameParts = owner.label.split(' ');
+ const first_name = nameParts[0] || '';
+ const last_name = nameParts.length > 1 ? nameParts.slice(1).join(' ') : '';
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2277229279
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
or @Copilot WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2288916985
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
@rusackas It's doable. I'm just worried theres other part of the code that "require" the specific return from the put request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "JZ6 (via GitHub)" <gi...@apache.org>.
JZ6 commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3207291836
> Hey! Sorry I went AWOL on the thread here :) I think we can likely merge this, pending a second opinion on the extra API call (probably not an issue). The main blocker at the moment is that there's a rebase needed, which I hope isn't too painful with the changes that have taken place in the tests.
should be rebased now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2294968091
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
Higher level solution I've been meaning to push forward: the superset client should be dynamically generated from the openapi spec, and be fully typed. It would mean that typescript would then be able to catch this kind of stuff at compile time. Meaning as you change the openapi spec to return this consistent schema across PUT/POST/GET, it would regenerate a fully typed client, and `tsc` would tell you exactly which calls break. Fun project if someone wants to take this on!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2294967499
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
related: https://chatgpt.com/share/68a90a13-5390-8010-b149-6a87d0014de3
Personally would say: got for it, make it such that all 3 PUT/POST/GET all return the same schema as per best practices. If something break, a test should fail or we'll know soon enough.
Might be good to grep/search or add Claude Code or AI to do a deep search in the repo to confirm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2294968091
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
Higher level solution I've been meaning to push forward: the superset client should be dynamically generated from the openapi spec, and be fully typed. It would mean that typescript would then be able to catch this kind of stuff at compile time. Fun project if someone wants to take this on!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on code in PR #33392:
URL: https://github.com/apache/superset/pull/33392#discussion_r2294746288
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -179,20 +179,15 @@ function PropertiesModal({
}
try {
- const res = await SupersetClient.put({
+ let res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
- // update the redux state
- const updatedChart = {
- ...payload,
- ...res.json.result,
- tags,
- id: slice.slice_id,
- owners: selectedOwners,
- };
- onSave(updatedChart);
+ res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ onSave(res.json.result);
Review Comment:
Kinda.
put returns:
<img width="227" height="50" alt="image" src="https://github.com/user-attachments/assets/10506a08-75a0-4d23-9451-254ed9f8c88c" />
get returns:
<img width="479" height="56" alt="image" src="https://github.com/user-attachments/assets/a1580226-f331-4e28-bd20-95bc695a327c" />
I think cuz schema is int instead of relationship
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-3230965129
Need help figuring out how to get the PUT to return the same object as the GET?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #33392:
URL: https://github.com/apache/superset/pull/33392
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart list): Facepile shows correct users when saving chart properties [superset]
Posted by "ethan-l-geotab (via GitHub)" <gi...@apache.org>.
ethan-l-geotab commented on PR #33392:
URL: https://github.com/apache/superset/pull/33392#issuecomment-2863820792
/korbit-review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org