codeant-ai-for-open-source[bot] commented on code in PR #40193:
URL: https://github.com/apache/superset/pull/40193#discussion_r3253332999
##########
superset-frontend/src/dashboard/actions/dashboardState.ts:
##########
@@ -468,9 +468,8 @@ export function saveDashboardRequest(
);
const cleanedData: JsonObject = {
...data,
- certified_by: certified_by || '',
- certification_details:
- certified_by && certification_details ? certification_details : '',
+ ...(certified_by !== undefined && { certified_by }),
+ ...(certification_details !== undefined && { certification_details }),
Review Comment:
**Suggestion:** `certification_details` is now sent whenever it is defined,
even when certification is being removed (`certified_by` is empty). In flows
where the details field still has a previous value (for example after clearing
only the certifier), this re-saves stale certification text and leaves
certification state inconsistent. Gate `certification_details` on certification
being present (or explicitly clear both together) so clearing certification
does not keep old details. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Clearing certification leaves certification_details non-null in database.
- ⚠️ Layout save persists stale certification_details while uncertified.
- ⚠️ Downstream consumers see inconsistent certification metadata for
dashboard.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a certified dashboard and click "Edit properties" to open the
properties modal
(`superset-frontend/src/dashboard/components/Header/index.tsx`,
`showPropertiesModal`
around lines 114–120, which renders `PropertiesModal` from
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx`).
2. In the properties modal certification section (`CertificationSection` at
`superset-frontend/src/dashboard/components/PropertiesModal/sections/CertificationSection.tsx`,
lines ~27–45), clear only the "Certified by" input, but leave the
"Certification details"
input unchanged, then click Save.
3. On submit, `PropertiesModal`'s `onFinish` builds `saveData` with
`certified_by:
certifiedBy || null` and `certification_details: certifiedBy &&
certificationDetails ?
certificationDetails : null` and PUTs to `/api/v1/dashboard/${dashboardId}`
(index.tsx
lines 166–183), clearing both fields in the backend, but then calls the
`onSubmit`
callback, which in the header (`handleOnPropertiesChange` in
`Header/index.tsx` around
lines 156–171) dispatches `dashboardInfoChanged` with `certified_by:
updates.certifiedBy`
(now `''`) and `certification_details: updates.certificationDetails` (still
the old
non-empty text), leaving Redux `dashboardInfo` with an empty `certified_by`
and stale
`certification_details`.
4. While still in edit mode, modify the layout and click the main
Save/Overwrite button;
`overwriteDashboard` in `Header/index.tsx` (lines 32–61, 80) builds a `data`
object with
`certified_by: dashboardInfo.certified_by` (empty string) and
`certification_details:
dashboardInfo.certification_details` (stale text) and calls
`boundActionCreators.onSave(data, dashboardInfo.id, SAVE_TYPE_OVERWRITE)`,
which is
`saveDashboardRequest`. In `saveDashboardRequest` (`dashboardState.ts` lines
99–108 and
469–544), `cleanedData` includes both `certified_by` and
`certification_details` because
they are not `undefined` (`...(certified_by !== undefined && { certified_by
})` and
`...(certification_details !== undefined && { certification_details })`),
and the PUT body
constructed around lines 627–627 sends `certified_by: ''` together with
`certification_details: '<stale-text>'` to `/api/v1/dashboard/${id}`,
re-persisting
certification details even though certification has been cleared, leaving
backend
certification metadata inconsistent.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fdashboard%2Factions%2FdashboardState.ts%0A%2A%2ALine%3A%2A%2A%20472%3A472%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20%60certification_details%60%20is%20now%20sent%20whenever%20it%20is%20defined%2C%20even%20when%20certification%20is%20being%20removed%20%28%60certified_by%60%20is%20empty%29.%20In%20flows%20where%20the%20details%20field%20still%20has%20a%20previous%20value%20%28for%20example%20after%20clearing%20only%20the%20certifier%29%2C%20this%20re-saves%20stale%20certification%20text%20and%20leaves%20certification%20state%20inconsistent.%20Gate%20%60certification_details%60%20on%20certification%20being%20present%20%28or%20explicitly%20clear%20both%20together%29%20so%20clearing%20certification%20does%20not%20keep%20old%20details.%0A%0AValidate%20the%20correctness%20of%20the%2
0flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fdashboard%2Factions%2FdashboardState.ts%0A%2A%2ALine%3A%2A%2A%20472%3A472%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20%60certification_details%60%20is%20now%20sent%20whenever%20it%20is%20defined%2C%20even%20when%20certification%20is%20being%20removed%20%28%60certified_by%60%20is%20
empty%29.%20In%20flows%20where%20the%20details%20field%20still%20has%20a%20previous%20value%20%28for%20example%20after%20clearing%20only%20the%20certifier%29%2C%20this%20re-saves%20stale%20certification%20text%20and%20leaves%20certification%20state%20inconsistent.%20Gate%20%60certification_details%60%20on%20certification%20being%20present%20%28or%20explicitly%20clear%20both%20together%29%20so%20clearing%20certification%20does%20not%20keep%20old%20details.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/actions/dashboardState.ts
**Line:** 472:472
**Comment:**
*Incorrect Condition Logic: `certification_details` is now sent
whenever it is defined, even when certification is being removed
(`certified_by` is empty). In flows where the details field still has a
previous value (for example after clearing only the certifier), this re-saves
stale certification text and leaves certification state inconsistent. Gate
`certification_details` on certification being present (or explicitly clear
both together) so clearing certification does not keep old details.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40193&comment_hash=4922484e86b0c1bf6548f6c74d87865ab2f0f2a48b9f119710efab4d05167038&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40193&comment_hash=4922484e86b0c1bf6548f6c74d87865ab2f0f2a48b9f119710efab4d05167038&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]
