This is an automated email from the ASF dual-hosted git repository.
voidmatcha pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new d3e932f0b1 [ZEPPELIN-6591] Discard unsaved note permission edits on
Cancel
d3e932f0b1 is described below
commit d3e932f0b1e61aea2a2cec3b52666947294fca8b
Author: JangAyeon <[email protected]>
AuthorDate: Thu Oct 1 23:09:28 2026 +0900
[ZEPPELIN-6591] Discard unsaved note permission edits on Cancel
### 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 `<at>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
* [x] Edit a detached draft instead of the parent-owned permissions object
* [x] Refresh the saved permissions from the backend after Save
* [x] Add unit tests for cancel, reopen, reset and save
### What is the Jira issue?
[ZEPPELIN-6591](https://issues.apache.org/jira/browse/ZEPPELIN-6591)
### How should this be tested?
* add unit tests for changed behavior
```bash
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
Closes #5510 from JangAyeon/ZEPPELIN-6591.
Signed-off-by: YONGJAE LEE <[email protected]>
---
.../workspace/notebook/notebook.component.html | 1 +
.../permissions/permissions.component.html | 16 +--
.../permissions/permissions.component.spec.ts | 112 +++++++++++++++++++++
.../notebook/permissions/permissions.component.ts | 31 ++++--
4 files changed, 143 insertions(+), 17 deletions(-)
diff --git
a/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.html
b/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.html
index 6dc08633df..f17bf0cdf6 100644
---
a/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.html
+++
b/zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.html
@@ -61,6 +61,7 @@
[noteId]="note.id"
[(activatedExtension)]="activatedExtension"
[permissions]="permissions"
+ (permissionsSaved)="getPermissions(note)"
></zeppelin-notebook-permissions>
}
@case ('revisions') {
diff --git
a/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.html
b/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.html
index 37595eabb3..3f4a73d59f 100644
---
a/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.html
+++
b/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.html
@@ -29,13 +29,13 @@
<nz-form-control [nzSpan]="4">
<nz-select
nzSize="small"
- [(ngModel)]="permissions.owners"
+ [(ngModel)]="draftPermissions.owners"
nzServerSearch
(nzOnSearch)="searchUser($event)"
nzMode="multiple"
name="owners"
>
- @for (item of permissions.owners; track item) {
+ @for (item of draftPermissions.owners; track item) {
<nz-option [nzLabel]="item" [nzValue]="item"></nz-option>
}
@for (item of listOfUserAndRole; track item) {
@@ -56,13 +56,13 @@
<nz-form-control [nzSpan]="4">
<nz-select
nzSize="small"
- [(ngModel)]="permissions.writers"
+ [(ngModel)]="draftPermissions.writers"
nzServerSearch
(nzOnSearch)="searchUser($event)"
nzMode="multiple"
name="writers"
>
- @for (item of permissions.writers; track item) {
+ @for (item of draftPermissions.writers; track item) {
<nz-option [nzLabel]="item" [nzValue]="item"></nz-option>
}
@for (item of listOfUserAndRole; track item) {
@@ -83,13 +83,13 @@
<nz-form-control [nzSpan]="4">
<nz-select
nzSize="small"
- [(ngModel)]="permissions.runners"
+ [(ngModel)]="draftPermissions.runners"
nzServerSearch
(nzOnSearch)="searchUser($event)"
nzMode="multiple"
name="runners"
>
- @for (item of permissions.runners; track item) {
+ @for (item of draftPermissions.runners; track item) {
<nz-option [nzLabel]="item" [nzValue]="item"></nz-option>
}
@for (item of listOfUserAndRole; track item) {
@@ -110,13 +110,13 @@
<nz-form-control [nzSpan]="4">
<nz-select
nzSize="small"
- [(ngModel)]="permissions.readers"
+ [(ngModel)]="draftPermissions.readers"
nzServerSearch
(nzOnSearch)="searchUser($event)"
nzMode="multiple"
name="readers"
>
- @for (item of permissions.readers; track item) {
+ @for (item of draftPermissions.readers; track item) {
<nz-option [nzLabel]="item" [nzValue]="item"></nz-option>
}
@for (item of listOfUserAndRole; track item) {
diff --git
a/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.spec.ts
b/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.spec.ts
new file mode 100644
index 0000000000..908ffdca20
--- /dev/null
+++
b/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.spec.ts
@@ -0,0 +1,112 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { ChangeDetectorRef, SimpleChange } from '@angular/core';
+import { NzMessageService } from 'ng-zorro-antd/message';
+import { NzModalService } from 'ng-zorro-antd/modal';
+import { of } from 'rxjs';
+import { describe, expect, it, vi } from 'vitest';
+
+import { Permissions } from '@zeppelin/interfaces';
+import { SecurityService, TicketService } from '@zeppelin/services';
+
+import { NotebookPermissionsComponent } from './permissions.component';
+
+const savedPermissions = (): Permissions => ({
+ owners: ['alice'],
+ writers: ['bob'],
+ runners: ['carol'],
+ readers: ['dave']
+});
+
+const createComponent = (permissions: Permissions) => {
+ const setPermissions = vi.fn(() => of(undefined));
+ const component = new NotebookPermissionsComponent(
+ { setPermissions } as unknown as SecurityService,
+ { markForCheck: vi.fn() } as unknown as ChangeDetectorRef,
+ { success: vi.fn() } as unknown as NzMessageService,
+ { ticket: { principal: 'alice' } } as unknown as TicketService,
+ { create: vi.fn() } as unknown as NzModalService
+ );
+ component.noteId = 'note-1';
+ component.permissions = permissions;
+ component.ngOnInit();
+ return { component, setPermissions };
+};
+
+const editEveryList = (component: NotebookPermissionsComponent) => {
+ component.draftPermissions.owners = [];
+ component.draftPermissions.writers.push('eve');
+ component.draftPermissions.runners = ['frank'];
+ component.draftPermissions.readers.pop();
+};
+
+describe('NotebookPermissionsComponent cancel', () => {
+ it('leaves the parent permissions unchanged when edits are cancelled', () =>
{
+ const parent = savedPermissions();
+ const { component, setPermissions } = createComponent(parent);
+
+ editEveryList(component);
+ component.closePermissions();
+
+ expect(parent).toEqual(savedPermissions());
+ expect(setPermissions).not.toHaveBeenCalled();
+ });
+
+ it('shows the saved values when the panel is reopened after cancel', () => {
+ const parent = savedPermissions();
+ const { component: first } = createComponent(parent);
+ editEveryList(first);
+ first.closePermissions();
+
+ const { component: reopened } = createComponent(parent);
+
+ expect(reopened.draftPermissions).toEqual(savedPermissions());
+ });
+
+ it('restores the draft from the saved values on reset', () => {
+ const { component } = createComponent(savedPermissions());
+ editEveryList(component);
+
+ component.resetPermissions();
+
+ expect(component.draftPermissions).toEqual(savedPermissions());
+ });
+
+ it('refreshes the draft when new saved permissions arrive', () => {
+ const { component } = createComponent(savedPermissions());
+ editEveryList(component);
+ const next: Permissions = { owners: ['zoe'], writers: [], runners: [],
readers: [] };
+
+ component.permissions = next;
+ component.ngOnChanges({ permissions: new SimpleChange(undefined, next,
false) });
+
+ expect(component.draftPermissions).toEqual(next);
+ });
+});
+
+describe('NotebookPermissionsComponent save', () => {
+ it('persists all four edited lists and reports them to the parent', () => {
+ const parent = savedPermissions();
+ const { component, setPermissions } = createComponent(parent);
+ const saved = vi.fn();
+ component.permissionsSaved.subscribe(saved);
+ const expected: Permissions = { owners: ['alice'], writers: ['bob',
'eve'], runners: ['frank'], readers: [] };
+
+ component.draftPermissions = { ...expected, writers: [...expected.writers]
};
+ component.savePermissions();
+
+ expect(setPermissions).toHaveBeenCalledWith('note-1', expected);
+ expect(saved).toHaveBeenCalledWith(expected);
+ expect(parent).toEqual(savedPermissions());
+ });
+});
diff --git
a/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.ts
b/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.ts
index f6015e09b3..2d4438d452 100644
---
a/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.ts
+++
b/zeppelin-web-angular/src/app/pages/workspace/notebook/permissions/permissions.component.ts
@@ -18,7 +18,8 @@ import {
Input,
OnChanges,
OnInit,
- Output
+ Output,
+ SimpleChanges
} from '@angular/core';
import { NzMessageService } from 'ng-zorro-antd/message';
@@ -27,6 +28,13 @@ import { NzModalService } from 'ng-zorro-antd/modal';
import { Permissions } from '@zeppelin/interfaces';
import { SecurityService, TicketService } from '@zeppelin/services';
+const clonePermissions = (permissions: Permissions): Permissions => ({
+ owners: [...permissions.owners],
+ writers: [...permissions.writers],
+ runners: [...permissions.runners],
+ readers: [...permissions.readers]
+});
+
@Component({
selector: 'zeppelin-notebook-permissions',
templateUrl: './permissions.component.html',
@@ -41,7 +49,8 @@ export class NotebookPermissionsComponent implements OnInit,
OnChanges {
@Output() readonly activatedExtensionChange = new EventEmitter<
'interpreter' | 'permissions' | 'revisions' | 'hide'
>();
- permissionsBack!: Permissions;
+ @Output() readonly permissionsSaved = new EventEmitter<Permissions>();
+ draftPermissions!: Permissions;
listOfUserAndRole: Array<{ text: string; children: string[] }> = [];
savePermissions() {
@@ -57,7 +66,7 @@ export class NotebookPermissionsComponent implements OnInit,
OnChanges {
'Please fill the [Owners] field. If not, it will set as current
user. ' +
`Current user : [ ${this.ticketService.ticket.principal.trim()} ]`,
nzOnOk: () => {
- this.permissions.owners = [this.ticketService.ticket.principal];
+ this.draftPermissions.owners = [this.ticketService.ticket.principal];
this.setPermissions();
},
nzOnCancel: () => {
@@ -87,18 +96,20 @@ export class NotebookPermissionsComponent implements
OnInit, OnChanges {
}
setPermissions() {
- this.securityService.setPermissions(this.noteId,
this.permissions).subscribe(() => {
+ const saved = clonePermissions(this.draftPermissions);
+ this.securityService.setPermissions(this.noteId, saved).subscribe(() => {
this.nzMessageService.success('Permissions Saved Successfully');
+ this.permissionsSaved.emit(saved);
this.closePermissions();
});
}
resetPermissions() {
- this.permissions = { ...this.permissionsBack };
+ this.draftPermissions = clonePermissions(this.permissions);
}
isOwnerEmpty() {
- return !this.permissions.owners.some(o => o.trim().length > 0);
+ return !this.draftPermissions.owners.some(o => o.trim().length > 0);
}
searchUser(search: string) {
@@ -130,10 +141,12 @@ export class NotebookPermissionsComponent implements
OnInit, OnChanges {
) {}
ngOnInit() {
- this.permissionsBack = { ...this.permissions };
+ this.resetPermissions();
}
- ngOnChanges(): void {
- this.permissionsBack = { ...this.permissions };
+ ngOnChanges(changes: SimpleChanges): void {
+ if (changes.permissions) {
+ this.resetPermissions();
+ }
}
}
