bito-code-review[bot] commented on code in PR #43350:
URL: https://github.com/apache/superset/pull/43350#discussion_r3823012285
##########
superset-frontend/src/explore/actions/saveModalActions.ts:
##########
@@ -233,21 +245,69 @@ export const updateSlice =
new?: boolean;
},
) =>
- async (dispatch: Dispatch, getState: () => Partial<QueryFormData>) => {
+ async (
+ dispatch: Dispatch,
+ getState: () => Partial<QueryFormData> & {
+ versionHistory?: {
+ chartNormalization?: ChartNormalizationTrackingState | null;
+ };
+ },
+ ) => {
const { slice_id: sliceId, editors, form_data: formDataFromSlice } = slice;
- const formData = getState().explore?.form_data;
+ const initialState = getState();
+ const formData = JSON.parse(
+ JSON.stringify(initialState.explore?.form_data ?? {}),
+ ) as QueryFormData;
+ const tracking = initialState.versionHistory?.chartNormalization;
+ const saveAttemptId = nanoid();
+ const matchingExclusions = Object.fromEntries(
+ Object.entries(tracking?.exclusions ?? {}).filter(
+ ([control, transition]) =>
+ !tracking?.invalidatedControls[control] &&
+ Object.hasOwn(formData, control) === transition.to_present &&
+ (!transition.to_present ||
+ JSON.stringify(formData[control]) ===
+ JSON.stringify(transition.to_value)),
+ ),
+ ) as AutomaticNormalizationExclusions;
+ const shouldAttachNormalization =
+ isFeatureEnabled(FeatureFlag.VersionHistory) &&
+ tracking?.chartId === sliceId;
+ if (shouldAttachNormalization) {
+ dispatch(
+ beginChartNormalizationSave(
+ sliceId,
+ tracking.hydrationSessionId,
+ saveAttemptId,
+ ),
+ );
+ }
try {
+ const payload = await getSlicePayload(
+ sliceName,
+ formData,
+ dashboards,
+ editors as [],
+ formDataFromSlice,
+ );
+ if (shouldAttachNormalization && Object.keys(matchingExclusions).length)
{
+ payload.normalization_changes = Object.values(matchingExclusions);
+ }
const response = await SupersetClient.put({
endpoint: `/api/v1/chart/${sliceId}`,
- jsonPayload: await getSlicePayload(
- sliceName,
- formData,
- dashboards,
- editors as [],
- formDataFromSlice,
- ),
+ jsonPayload: payload,
});
+ if (shouldAttachNormalization) {
+ dispatch(
+ completeChartNormalizationSave(
+ sliceId,
+ tracking.hydrationSessionId,
+ saveAttemptId,
+ {},
Review Comment:
<!-- Bito Reply -->
The suggestion is correct. Passing an empty object `{}` to
`completeChartNormalizationSave` causes the reducer to overwrite the tracking
state's `exclusions` with an empty object, which is inconsistent with the
payload sent to the server. You should pass the `matchingExclusions` variable
instead to ensure the client-side state remains synchronized with the server.
**superset-frontend/src/explore/actions/saveModalActions.ts**
```
dispatch(
completeChartNormalizationSave(
sliceId,
tracking.hydrationSessionId,
saveAttemptId,
matchingExclusions,
),
```
##########
superset/commands/chart/update.py:
##########
@@ -78,6 +88,15 @@ def run(self) -> Model:
self._properties["last_saved_at"] = datetime.now()
self._properties["last_saved_by"] = g.user
+ if "params" in self._properties:
+ register_matching_normalization_context(
+ db.session,
+ self._model.id,
+ self._normalization_changes,
+ self._model.params,
+ self._properties["params"],
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion to restore the guard is appropriate. By re-adding the check
for `self._normalization_changes` before parsing the parameters, you avoid
unnecessary processing and maintain consistency with the existing short-circuit
logic in the function.
**superset/commands/chart/update.py**
```
if "params" in self._properties and self._normalization_changes is not None:
register_matching_normalization_context(
db.session,
self._model.id,
self._normalization_changes,
self._model.params,
self._properties["params"],
)
```
##########
superset-frontend/src/features/versionHistory/sessionLogMiddleware.ts:
##########
@@ -151,5 +165,23 @@ export const versionSessionLogMiddleware: Middleware =
}),
);
}
+ if (action.type !== HYDRATE_EXPLORE) {
+ const state = store.getState() as SessionLogState;
+ const controls = changedFormDataKeys(before, state.explore?.form_data);
+ if (
+ action.type === SET_FIELD_VALUE &&
Review Comment:
<!-- Bito Reply -->
The suggestion correctly identifies a potential issue where non-programmatic
`SET_FIELD_VALUE` actions could trigger both the existing session-log dispatch
and the new invalidation dispatch. Adding the `!action.programmatic` guard
ensures that the invalidation logic is only executed when intended, preventing
redundant dispatches. This is a valid and recommended improvement to maintain
consistent dispatch behavior.
**superset-frontend/src/features/versionHistory/sessionLogMiddleware.ts**
```
if (
action.type === SET_FIELD_VALUE &&
!action.programmatic
) {
```
--
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]
