aminghadersohi commented on code in PR #40685:
URL: https://github.com/apache/superset/pull/40685#discussion_r3342887454
##########
superset-frontend/src/dashboard/components/PropertiesModal/utils.test.ts:
##########
@@ -60,13 +60,14 @@ test('builds a new owner from the option text label when
not already in state',
});
test('falls back to a string label when the option has no text label', () => {
+ // No option in the cache => email stays undefined (not an empty string).
expect(
parseSelectedOwners([{ value: 4, label: 'Plain Name' }], [], []),
- ).toEqual([{ id: 4, full_name: 'Plain Name', email: '' }]);
+ ).toEqual([{ id: 4, full_name: 'Plain Name', email: undefined }]);
});
test('yields an empty name for a non-string label with no text label', () => {
expect(parseSelectedOwners([{ value: 5, label: 1 }], [], [])).toEqual([
- { id: 5, full_name: '', email: '' },
+ { id: 5, full_name: '', email: undefined },
]);
});
Review Comment:
**NIT:** The updated tests cover `email: undefined` only when `options = []`
(so `optionsById.get(id)` returns `undefined` and `opt` is never defined). The
`?? undefined` fix also applies to the path where the option **is** in the
cache but carries no `OWNER_EMAIL_PROP` — that code path goes untested.
Suggested addition:
```ts
test('leaves email undefined when option is cached but carries no email', ()
=> {
const options = [
{ value: 6, label: 'Dave Doe', [OWNER_TEXT_LABEL_PROP]: 'Dave Doe' },
// intentionally no OWNER_EMAIL_PROP
];
expect(parseSelectedOwners([{ value: 6, label: 'Dave Doe' }], options,
[])).toEqual([
{ id: 6, full_name: 'Dave Doe', email: undefined },
]);
});
```
Non-blocking — the PR's stated goal (honoring the optional `email?` type) is
correct regardless. Happy to approve without it.
--
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]
