sadpandajoe commented on code in PR #45090:
URL: https://github.com/apache/superset/pull/45090#discussion_r4388276166
##########
superset/commands/canvas/apply_ops.py:
##########
@@ -112,27 +112,60 @@ def apply_or_raise(
definition: dict[str, Any],
ops: list[Operation],
pending: dict[str, StoredWidget] | None = None,
+ first_index: int = 0,
) -> tuple[dict[str, Any], list[AppliedOperation]]:
"""
Apply ``ops`` to a copy of ``definition``, as one atomic batch; widgets
they create or change are added to ``pending``.
+
+ ``first_index`` is where these operations start in the caller's own list,
+ so an error reported against a slice still names the operation the caller
+ sent.
"""
try:
return apply_operations(definition, ops, pending=pending)
except OperationError as ex:
raise DefinitionInvalidError(
str(ex),
- issues=[Issue(pointer("ops", ex.index), ex.message)],
- operation_index=ex.index,
+ issues=[Issue(pointer("ops", first_index + ex.index), ex.message)],
+ operation_index=first_index + ex.index,
) from ex
except DefinitionValidationError as ex:
raise DefinitionInvalidError(
"The operations produce an invalid definition",
issues=ex.issues,
- operation_index=ex.operation,
+ operation_index=(
+ None if ex.operation is None else first_index + ex.operation
+ ),
) from ex
+def apply_stepwise(
+ definition: dict[str, Any],
+ ops: list[Operation],
+ pending: dict[str, StoredWidget] | None = None,
+) -> tuple[dict[str, Any], list[AppliedOperation]]:
+ """
+ Apply ``ops`` one at a time, resolving layout after each.
+
+ For replaying a draft. A draft applies each operation as the user makes
+ it and resolves layout then, so a later operation sees where the one
+ before it pushed the widgets. Replaying them as a single batch resolves
+ collisions only once and can reach a different layout -- drag a widget
+ over another and back, and the one that was pushed aside stays pushed in
+ the draft but returns on publish. The canvas would then not match the plan
+ that was approved.
+
+ One resolve per operation rather than one for the batch, which is why this
+ is kept to the commit path.
+ """
+ applied: list[AppliedOperation] = []
+ for index, op in enumerate(ops):
+ definition, step = apply_or_raise(definition, [op], pending, index)
Review Comment:
Publishing a plan can still lay widgets out differently than the draft
showed. The draft applies each PATCH's `ops` as one batch and resolves
collisions once (`apply_or_raise` in `draft.py`), but the commit replays the
flattened op log one operation at a time, resolving after each, and the batch
boundaries are no longer in the log. Take three full-width, one-row widgets
A/B/C at rows 1/2/3 and send one draft PATCH (the UI queues gestures into one
request while a save is pending) with A -> row 2 then B -> row 1: the draft
ends B/A/C at rows 1/2/3, but the stepwise replay pushes C down to row 4 on
publish. Could the draft keep each request's ops together (or store the
resolved layouts) so the replay resolves per request, and add a command-level
test that publishes a multi-op draft request and asserts the published
placements equal the draft's?
--
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]
