Skip to content

[ZEPPELIN-6591] Discard unsaved note permission edits on Cancel - #5510

Merged
voidmatcha merged 1 commit into
apache:masterfrom
JangAyeon:ZEPPELIN-6591
Oct 1, 2026
Merged

voidmatcha merged 1 commit into
apache:masterfrom
JangAyeon:ZEPPELIN-6591

Conversation

@JangAyeon

Copy link
Copy Markdown
Contributor

What is this PR for?

In the New UI note permissions panel, each select was bound with [(ngModel)] directly to the permissions object owned by NotebookComponent. Editing a list mutated the parent's object, and Cancel only hid the panel. Reopening the panel reused the same mutated object, so it showed the unsaved edits while GET /api/notebook/{noteId}/permissions still returned the saved values. A later Save could also submit those stale edits.

This PR makes the panel edit a detached draft:

  • NotebookPermissionsComponent builds draftPermissions from the input when it initializes and when a new permissions input arrives. All four lists are copied, so in-place array edits do not reach the parent either.
  • The template binds every select to the draft. Cancel discards it and sends no request.
  • Save sends a copy of the draft through the existing endpoint, then emits permissionsSaved. NotebookComponent handles it by calling getPermissions(note), so the saved state (and isOwner) is refreshed from the backend.
  • The empty-Owners modal now fills and resets the draft. permissionsBack is removed because the draft replaces it.

As the issue notes, reassigning the child's @Input() would not repair an object already mutated in the parent. With this change the parent object is never mutated, and the panel is destroyed on close, so reopening always starts from the parent's saved state.

What type of PR is it?

Bug Fix

Todos

  • Edit a detached draft instead of the parent-owned permissions object
  • Refresh the saved permissions from the backend after Save
  • Add unit tests for cancel, reopen, reset and save

What is the Jira issue?

ZEPPELIN-6591

How should this be tested?

  • add unit tests for changed behavior
  cd zeppelin-web-angular
  npm run test:shell -- permissions.component.spec.ts

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@voidmatcha voidmatcha left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

AS-IS

before-pr-clean.mp4

TO-BE

after-pr-clean.mp4

@voidmatcha
voidmatcha merged commit d3e932f into apache:master Oct 1, 2026
24 checks passed
@voidmatcha

Copy link
Copy Markdown
Member

Merged into master (d3e932f).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants