Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(forms): correctly apply remote changes to form if removing a property #2020

Merged
merged 5 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import { Injectable } from "@angular/core";
import { Injectable, NgZone } from "@angular/core";
import { MatDialog } from "@angular/material/dialog";
import {
ConfirmationDialogButton,
ConfirmationDialogComponent,
YesNoButtons,
} from "./confirmation-dialog/confirmation-dialog.component";
import { map } from "rxjs/operators";
import { firstValueFrom } from "rxjs";

/**
Expand All @@ -25,7 +24,10 @@ import { firstValueFrom } from "rxjs";
*/
@Injectable({ providedIn: "root" })
export class ConfirmationDialogService {
constructor(private dialog: MatDialog) {}
constructor(
private dialog: MatDialog,
private ngZone: NgZone,
) {}

/**
* Open a dialog with the given configuration.
Expand All @@ -42,18 +44,17 @@ export class ConfirmationDialogService {
buttons: ConfirmationDialogButton[] = YesNoButtons,
closeButton = true,
): Promise<boolean> {
return firstValueFrom(
this.dialog
.open(ConfirmationDialogComponent, {
data: {
title: title,
text: text,
buttons: buttons,
closeButton: closeButton,
},
})
.afterClosed(),
);
const dialogRef = this.ngZone.run(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why this needs to run outside the zone?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, can't explain it 😅

It needs to run inside ngZone change detection, otherwise I got an empty dialog and the text only showed after I resized the window. No idea why this started happening now, however ...

return this.dialog.open(ConfirmationDialogComponent, {
data: {
title: title,
text: text,
buttons: buttons,
closeButton: closeButton,
},
});
});
return firstValueFrom(dialogRef.afterClosed());
}

getDiscardConfirmation() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ describe("EditTextWithAutocompleteComponent", () => {
let component: EditTextWithAutocompleteComponent;
let fixture: ComponentFixture<EditTextWithAutocompleteComponent>;
let loadTypeSpy: jasmine.Spy;
let mockConfirmationDialog: jasmine.SpyObj<ConfirmationDialogService>;

beforeEach(waitForAsync(() => {
mockConfirmationDialog = jasmine.createSpyObj(["getConfirmation"]);
TestBed.configureTestingModule({
imports: [
EditTextWithAutocompleteComponent,
Expand All @@ -23,7 +25,7 @@ describe("EditTextWithAutocompleteComponent", () => {
providers: [
{
provide: ConfirmationDialogService,
useValue: new ConfirmationDialogService(null),
useValue: mockConfirmationDialog,
},
],
}).compileComponents();
Expand Down Expand Up @@ -149,11 +151,7 @@ describe("EditTextWithAutocompleteComponent", () => {
rA1.assignedTo = ["user1", "user2"];
rA1.linkedGroups = ["group1", "group2"];
loadTypeSpy.and.resolveTo([rA1, rA2]);
const confirmationDialogueSpy: jasmine.Spy = spyOn(
TestBed.inject(ConfirmationDialogService),
"getConfirmation",
);
confirmationDialogueSpy.and.resolveTo(true);
mockConfirmationDialog.getConfirmation.and.resolveTo(true);

await component.ngOnInit();
component.formControl.setValue("test1");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,24 @@ describe("EntityFormComponent", () => {
);
});

it("should clear field in form for properties removed in updated remote entity", async () => {
const originalEntity = { projectNumber: "p1", name: "test" };
const formValues = { projectNumber: "p2", name: "test" };
const remoteValues = {
_rev: "new rev",
};
await expectApplyChangesPopup(
"no",
originalEntity,
formValues,
remoteValues,
{
projectNumber: "p2",
_rev: "new rev",
},
);
});

it("should not show popup if date was saved as day-only", async () => {
const form = { dateOfBirth: new DateWithAge() };
const dateOnly = new DateWithAge();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,48 +88,43 @@ export class EntityFormComponent<T extends Entity = Entity>
}
}

private async applyChanges(entity: T) {
if (this.formIsUpToDate(entity)) {
Object.assign(this.entity, entity as any);
private async applyChanges(externallyUpdatedEntity: T) {
if (this.formIsUpToDate(externallyUpdatedEntity)) {
Object.assign(this.entity, externallyUpdatedEntity);
return;
}

const userEditedFields = Object.entries(this.form.getRawValue()).filter(
([key]) => this.form.controls[key].dirty,
);
let userEditsWithoutConflicts = userEditedFields.filter(([key]) =>
// no conflict with updated values
this.entityEqualsFormValue(
externallyUpdatedEntity[key],
this.initialFormValues[key],
),
);
if (
this.changesOnlyAffectPristineFields(entity) ||
(await this.confirmationDialog.getConfirmation(
userEditsWithoutConflicts.length !== userEditedFields.length &&
!(await this.confirmationDialog.getConfirmation(
$localize`Load changes?`,
$localize`Local changes are in conflict with updated values synced from the server. Do you want the local changes to be overwritten with the latest values?`,
))
) {
Object.assign(this.initialFormValues, entity);
this.form.patchValue(entity as any);
Object.assign(this.entity, entity as any);
// user "resolved" conflicts by confirming to overwrite
userEditsWithoutConflicts = userEditedFields;
}
}

private changesOnlyAffectPristineFields(updatedEntity: T) {
if (this.form.pristine) {
return true;
}

const dirtyFields = Object.entries(this.form.controls).filter(
([_, form]) => form.dirty,
);
for (const [key] of dirtyFields) {
if (
this.entityEqualsFormValue(
updatedEntity[key],
this.initialFormValues[key],
)
) {
// keep our pending form field changes
delete updatedEntity[key];
} else {
// dirty form field has conflicting change
return false;
}
}
// apply update to all pristine (not user-edited) fields and update base entity (to avoid conflicts when saving)
Object.assign(this.entity, externallyUpdatedEntity);
Object.assign(this.initialFormValues, externallyUpdatedEntity);
this.form.reset(externallyUpdatedEntity as any);

return true;
// re-apply user-edited fields
userEditsWithoutConflicts.forEach(([key, value]) => {
this.form.get(key).setValue(value);
this.form.get(key).markAsDirty();
});
}

private formIsUpToDate(entity: T): boolean {
Expand Down