codeant-ai-for-open-source[bot] commented on code in PR #37906:
URL: https://github.com/apache/superset/pull/37906#discussion_r2799108062
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -240,10 +242,15 @@ const PropertiesModal = ({
}
};
- const handleOnChangeOwners = (owners: { value: number; label: string }[]) =>
{
- const parsedOwners: Owners = ensureIsArray(owners).map(o => ({
+ const handleOnChangeOwners = (
+ owners: { value: number; label: string }[],
+ options: Record<string, unknown>[],
+ ) => {
+ const parsedOwners: Owners = ensureIsArray(owners).map((o, i) => ({
id: o.value,
- full_name: o.label,
+ full_name:
+ (options?.[i]?.[OWNER_TEXT_LABEL_PROP] as string) ||
+ (typeof o.label === 'string' ? o.label : ''),
}));
Review Comment:
**Suggestion:** When dashboard owners are changed, the updated owners stored
in state only retain the full name and drop the email/username, so subsequent
renders of the owners selector lose the secondary username line for those
owners; the handler should also persist the email from the AsyncSelect options
into the `owners` state. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Owners selector loses username subtitle after editing owners.
- ⚠️ Harder to distinguish owners with identical names.
```
</details>
```suggestion
import {
OWNER_TEXT_LABEL_PROP,
OWNER_EMAIL_PROP,
} from 'src/features/owners/OwnerSelectLabel';
import { fetchTags, OBJECT_TYPES } from 'src/features/tags/tags';
import {
applyColors,
getColorNamespace,
getFreshLabelsColorMapEntries,
} from 'src/utils/colorScheme';
import { useDispatch } from 'react-redux';
import {
setColorScheme,
setDashboardMetadata,
} from 'src/dashboard/actions/dashboardState';
import { areObjectsEqual } from 'src/reduxUtils';
import { StandardModal, useModalValidation } from 'src/components/Modal';
import {
BasicInfoSection,
AccessSection,
StylingSection,
RefreshSection,
CertificationSection,
AdvancedSection,
} from './sections';
type PropertiesModalProps = {
dashboardId: number;
dashboardTitle?: string;
dashboardInfo?: Record<string, any>;
show?: boolean;
onHide?: () => void;
colorScheme?: string;
onSubmit?: (params: Record<string, any>) => void;
addSuccessToast: (message: string) => void;
addDangerToast: (message: string) => void;
onlyApply?: boolean;
};
type Roles = { id: number; name: string }[];
type Owners = {
id: number;
full_name?: string;
first_name?: string;
last_name?: string;
email?: string;
}[];
type DashboardInfo = {
id: number;
title: string;
slug: string;
certifiedBy: string;
certificationDetails: string;
isManagedExternally: boolean;
metadata: Record<string, any>;
common?: {
conf?: {
SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT?: number;
};
};
};
const PropertiesModal = ({
addSuccessToast,
addDangerToast,
colorScheme: currentColorScheme,
dashboardId,
dashboardInfo: currentDashboardInfo,
dashboardTitle,
onHide = () => {},
onlyApply = false,
onSubmit = () => {},
show = false,
}: PropertiesModalProps) => {
const dispatch = useDispatch();
const [form] = Form.useForm();
const [isLoading, setIsLoading] = useState(true);
const [isApplying, setIsApplying] = useState(false);
const [colorScheme, setCurrentColorScheme] = useState(currentColorScheme);
const [jsonMetadata, setJsonMetadata] = useState('');
const [dashboardInfo, setDashboardInfo] = useState<DashboardInfo>();
// JSON validation for metadata
const jsonAnnotations = useJsonValidation(jsonMetadata, {
errorPrefix: 'Invalid JSON metadata',
});
const [owners, setOwners] = useState<Owners>([]);
const [roles, setRoles] = useState<Roles>([]);
const saveLabel = onlyApply ? t('Apply') : t('Save');
const [tags, setTags] = useState<TagType[]>([]);
const [customCss, setCustomCss] = useState('');
const [refreshFrequency, setRefreshFrequency] = useState(0);
const [selectedThemeId, setSelectedThemeId] = useState<number |
null>(null);
const [showChartTimestamps, setShowChartTimestamps] = useState(false);
const [themes, setThemes] = useState<
Array<{
id: number;
theme_name: string;
}>
>([]);
const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
const originalDashboardMetadata = useRef<Record<string, any>>({});
const handleErrorResponse = async (response: Response) => {
const { error, statusText, message } = await
getClientErrorObject(response);
let errorText = error || statusText || t('An error has occurred');
if (
typeof message === 'object' &&
'json_metadata' in message &&
typeof (message as { json_metadata: unknown }).json_metadata ===
'string'
) {
errorText = (message as { json_metadata: string }).json_metadata;
} else if (typeof message === 'string') {
errorText = message;
if (message === 'Forbidden') {
errorText = t('You do not have permission to edit this dashboard');
}
}
addDangerToast(String(errorText));
};
const handleDashboardData = useCallback(
dashboardData => {
const {
id,
dashboard_title,
slug,
certified_by,
certification_details,
owners,
roles,
metadata,
is_managed_externally,
theme,
css,
} = dashboardData;
const dashboardInfo = {
id,
title: dashboard_title,
slug: slug || '',
certifiedBy: certified_by || '',
certificationDetails: certification_details || '',
isManagedExternally: is_managed_externally || false,
css: css || '',
metadata,
};
form.setFieldsValue(dashboardInfo);
setDashboardInfo(dashboardInfo);
setOwners(owners);
setRoles(roles);
setCustomCss(css || '');
setCurrentColorScheme(metadata?.color_scheme);
setSelectedThemeId(theme?.id || null);
const metaDataCopy = omit(metadata, [
'positions',
'shared_label_colors',
'map_label_colors',
'color_scheme_domain',
'show_chart_timestamps',
]);
setJsonMetadata(metaDataCopy ? jsonStringify(metaDataCopy) : '');
setRefreshFrequency(metadata?.refresh_frequency || 0);
setShowChartTimestamps(metadata?.show_chart_timestamps ?? false);
originalDashboardMetadata.current = metadata;
},
[form],
);
const fetchDashboardDetails = useCallback(() => {
// We fetch the dashboard details because not all code
// that renders this component have all the values we need.
// At some point when we have a more consistent frontend
// datamodel, the dashboard could probably just be passed as a prop.
SupersetClient.get({
endpoint: `/api/v1/dashboard/${dashboardId}`,
}).then(response => {
const dashboard = response.json.result;
const jsonMetadataObj = dashboard.json_metadata?.length
? JSON.parse(dashboard.json_metadata)
: {};
handleDashboardData({
...dashboard,
metadata: jsonMetadataObj,
});
setIsLoading(false);
}, handleErrorResponse);
}, [dashboardId, handleDashboardData]);
const getJsonMetadata = () => {
try {
const jsonMetadataObj = jsonMetadata?.length
? JSON.parse(jsonMetadata)
: {};
return jsonMetadataObj;
} catch (_) {
return {};
}
};
const handleOnChangeOwners = (
owners: { value: number; label: string }[],
options: Record<string, unknown>[],
) => {
const parsedOwners: Owners = ensureIsArray(owners).map((o, i) => {
const fullName =
(options?.[i]?.[OWNER_TEXT_LABEL_PROP] as string) ||
(typeof o.label === 'string' ? o.label : '');
const email =
(options?.[i]?.[OWNER_EMAIL_PROP] as string | undefined) ||
undefined;
return {
id: o.value,
full_name: fullName,
email,
};
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard and click "Edit dashboard properties" to open
`PropertiesModal`
(`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx`),
which fetches
data from `/api/v1/dashboard/<id>` and calls `handleDashboardData` around
lines 88–135,
setting `owners` state via `setOwners(owners)` with owner objects that
include `email`.
2. In the modal's "Access & ownership" section rendered by `AccessSection`
(passed
`owners` and `onChangeOwners` at the bottom of `index.tsx`), modify the
owners list (e.g.,
add or remove an owner) so the owners AsyncSelect triggers the
`onChangeOwners` callback.
3. The callback `handleOnChangeOwners` at lines ~232–255 receives `(owners,
options)` and
maps each selection to `{ id, full_name }`, explicitly omitting the `email`
field defined
in the `Owners` type, then calls `setOwners(parsedOwners)`, replacing
previously
email-rich owner objects with ones containing only `id` and `full_name`.
4. After this state update, React re-renders `AccessSection` with the
updated `owners`
prop that now lacks `email`, so any UI in the owners selector (using
`OwnerSelectLabel`
from `src/features/owners/OwnerSelectLabel`) that relies on `owner.email` or
associated
option metadata to render the secondary username line no longer has that
data, and the
username/secondary text disappears for the modified owners until the modal
is closed and
reopened (which reloads full owner data via `handleDashboardData`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
**Line:** 41:254
**Comment:**
*Logic Error: When dashboard owners are changed, the updated owners
stored in state only retain the full name and drop the email/username, so
subsequent renders of the owners selector lose the secondary username line for
those owners; the handler should also persist the email from the AsyncSelect
options into the `owners` state.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=be6e720b8acbd98ee108fe57cbdb663199e23fdabe219f536e941189671c596b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=be6e720b8acbd98ee108fe57cbdb663199e23fdabe219f536e941189671c596b&reaction=dislike'>👎</a>
##########
superset/charts/schemas.py:
##########
@@ -1671,6 +1671,7 @@ class UserSchema(Schema):
id = fields.Int()
first_name = fields.String()
last_name = fields.String()
+ email = fields.String()
Review Comment:
**Suggestion:** The schema adds an `email` field to the user instead of a
`username` field, which contradicts the feature intent (showing username) and
means the API will not provide the data the frontend needs to render usernames,
while also unnecessarily exposing email addresses. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Chart detail owners lack username required for new UI.
- ❌ Email addresses exposed in owners API response.
- ⚠️ Owner selectors cannot show username as secondary text.
```
</details>
```suggestion
username = fields.String()
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the Superset backend including `superset/charts/schemas.py` from
this PR.
2. Call the chart detail API `GET /api/v1/chart/<id>` which uses
`ChartGetResponseSchema`
with `owners = fields.List(fields.Nested(UserSchema))` defined in
`superset/charts/schemas.py`.
3. Inspect the JSON `owners` array in the response: each owner object
contains `id`,
`first_name`, `last_name`, and `email` (from `UserSchema` at
`superset/charts/schemas.py:1670-1674`), but no `username` field.
4. Open any owner selector (e.g., Chart Properties → Owners) where the UI
feature is
specified to show usernames as secondary text; the API does not provide a
`username`
field, so the frontend cannot render usernames and instead only has `email`,
unnecessarily
exposing email addresses and failing the stated username-based requirement.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/schemas.py
**Line:** 1674:1674
**Comment:**
*Logic Error: The schema adds an `email` field to the user instead of a
`username` field, which contradicts the feature intent (showing username) and
means the API will not provide the data the frontend needs to render usernames,
while also unnecessarily exposing email addresses.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=a1d7d44cd1d0a304a4c207fb8d13c145d474d0a581e428251fc889401c198b09&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=a1d7d44cd1d0a304a4c207fb8d13c145d474d0a581e428251fc889401c198b09&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -1936,12 +1956,21 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
label: (resource.database as DatabaseObject).database_name,
}
: undefined,
- owners: (alert?.owners || []).map(owner => ({
- value: (owner as MetaObject).value || owner.id,
- label:
+ owners: (alert?.owners || []).map(owner => {
+ const ownerName =
(owner as MetaObject).label ||
- `${(owner as Owner).first_name} ${(owner as Owner).last_name}`,
- })),
+ `${(owner as Owner).first_name} ${(owner as Owner).last_name}`;
+ return {
Review Comment:
**Suggestion:** When populating the owners field for an existing alert, the
code reads from the `alert` prop instead of the freshly fetched `resource`, so
if the prop does not include owners (or is stale), the modal will show no
owners and saving will silently drop existing owners for that alert; using
`resource.owners` ensures the owners list reflects the current server state.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Editing alerts can silently remove all existing owners.
- ⚠️ Owners field appears empty despite backend having owners.
- ⚠️ Alert ownership metadata becomes inconsistent with server state.
```
</details>
```suggestion
owners: (resource.owners || []).map(owner => {
const ownerName =
(owner as MetaObject).label ||
`${(owner as Owner).first_name} ${(owner as Owner).last_name}`;
return {
value: (owner as MetaObject).value || (owner as Owner).id,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an existing alert/report for editing so `AlertReportModal` mounts
with a non-null
`alert` prop containing at least `id` (component `AlertReportModal` in
`superset-frontend/src/features/alerts/AlertReportModal.tsx`).
2. `useSingleViewResource<AlertObject>('report', ...)` inside
`AlertReportModal` fetches
the full alert from `/api/v1/report/<id>` and populates `resource` with the
canonical
server state, including `resource.owners` (see hook usage around the top of
`AlertReportModal`).
3. When `resource` is set, the `useEffect` that depends on `resource` runs
and calls
`setCurrentAlert({...resource, owners: (alert?.owners || []).map(...) })` at
lines
1946–1973; because this uses `alert?.owners` instead of `resource.owners`,
if the `alert`
prop has an empty/missing `owners` array (for example, a lightweight list
row object),
`currentAlert.owners` becomes an empty array even though `resource.owners`
is populated on
the server.
4. The user clicks "Save" in the modal, which calls `onSave` (same file) and
serializes
`owners: (currentAlert?.owners || []).map(owner => (owner as
MetaObject).value ||
owner.id)` into the update payload; because `currentAlert.owners` is empty,
the request
sent to `/api/v1/report/<id>` removes all owners from the alert, silently
dropping
existing owners.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/AlertReportModal.tsx
**Line:** 1959:1963
**Comment:**
*Logic Error: When populating the owners field for an existing alert,
the code reads from the `alert` prop instead of the freshly fetched `resource`,
so if the prop does not include owners (or is stale), the modal will show no
owners and saving will silently drop existing owners for that alert; using
`resource.owners` ensures the owners list reflects the current server state.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=36e5246c2a17720e42a6d781803753c92fa086390b92e4da48379ab8918351cc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=36e5246c2a17720e42a6d781803753c92fa086390b92e4da48379ab8918351cc&reaction=dislike'>👎</a>
##########
superset/reports/api.py:
##########
@@ -152,6 +158,7 @@ def ensure_alert_reports_enabled(self) ->
Optional[Response]:
"owners.first_name",
"owners.id",
"owners.last_name",
+ "owners.email",
Review Comment:
**Suggestion:** The report list API is exposing `owners.email` instead of
`owners.username` in `list_columns`, so list views and owner selectors backed
by this endpoint will surface email addresses instead of usernames, again
diverging from the requirement to show usernames as the secondary text. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Report list endpoint exposes owner email, not username.
- ⚠️ Owner displays in lists diverge from username-based design.
```
</details>
```suggestion
"owners.username",
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure there is at least one user whose `username` differs from `email`,
and create
several report schedules owned by that user.
2. Call `GET /api/v1/report/` (the list endpoint for
`ReportScheduleRestApi`, exposed via
`resource_name = \"report\"` in `superset/reports/api.py`).
3. The response items are populated from `list_columns` at
`superset/reports/api.py:148–166`, which currently include `"owners.email"`
but not
`"owners.username"`.
4. Inspect any element in the returned list: owner-related fields expose
`email` instead
of `username`, making it impossible for list-based consumers (e.g., report
list views or
owner selectors that reuse this payload) to show usernames as the secondary
text as
required by the PR description.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/api.py
**Line:** 161:161
**Comment:**
*Logic Error: The report list API is exposing `owners.email` instead of
`owners.username` in `list_columns`, so list views and owner selectors backed
by this endpoint will surface email addresses instead of usernames, again
diverging from the requirement to show usernames as the secondary text.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=2523eaeb3ce407b7e90d5a216f8627c917380ad21e932130c4ccd96c72240f79&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=2523eaeb3ce407b7e90d5a216f8627c917380ad21e932130c4ccd96c72240f79&reaction=dislike'>👎</a>
##########
superset/reports/api.py:
##########
@@ -113,6 +118,7 @@ def ensure_alert_reports_enabled(self) ->
Optional[Response]:
"owners.first_name",
"owners.id",
"owners.last_name",
+ "owners.email",
"recipients.id",
Review Comment:
**Suggestion:** The report detail API is exposing `owners.email` instead of
`owners.username` in `show_columns`, which will cause the owner selector and
detail views to show email addresses rather than usernames, conflicting with
the intended behavior and potentially confusing users in environments where
usernames and emails differ. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Report detail endpoint cannot supply usernames for owners.
- ⚠️ Alert/Report modal cannot show usernames as secondary text.
```
</details>
```suggestion
"owners.username",
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR code deployed and create a user whose
`username` differs
from `email` (via the security/user management UI or API).
2. Create a report schedule owned by that user via `POST /api/v1/report/`
handled by
`ReportScheduleRestApi.post` in `superset/reports/api.py` (around lines
200–250).
3. Fetch the report details with `GET /api/v1/report/<report_id>` (resource
name is
`"report"` in `ReportScheduleRestApi`, so FAB exposes `/api/v1/report/<pk>`).
4. Inspect the JSON `result.owners` payload; it contains `first_name`,
`last_name`, and
`email` because `show_columns` at `superset/reports/api.py:118–124` includes
`"owners.email"` but not `"owners.username"`.
5. Any client (including the Alert/Report modal described in the PR) that
needs to render
the owner with "secondary text = username" must instead use the exposed
`email`, so the UI
will show email addresses where usernames were requested in the feature spec.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/api.py
**Line:** 122:122
**Comment:**
*Logic Error: The report detail API is exposing `owners.email` instead
of `owners.username` in `show_columns`, which will cause the owner selector and
detail views to show email addresses rather than usernames, conflicting with
the intended behavior and potentially confusing users in environments where
usernames and emails differ.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=1d0602a19e6036c2deda6ee6163ccb0617a3114e4fae3432d0f52ca846c6ca9d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37906&comment_hash=1d0602a19e6036c2deda6ee6163ccb0617a3114e4fae3432d0f52ca846c6ca9d&reaction=dislike'>👎</a>
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
