Merge pull request #8351 from opf/fix/avoid-second-form-post

Avoid requesting the form more than once per changeset instance

frontend/src/app/components/time-entries/time-entry-changeset.ts

@@ -7,10 +7,8 @@ export class TimeEntryChangeset extends ResourceChangeset<TimeEntryResource> {
     super.setValue(key, val);
 
     // Update the form for fields that may alter the form itself
-    // when the time entry is new. Otherwise, the save request afterwards
-    // will update the form automatically.
-    if (this.pristineResource.isNew && (key === 'workPackage')) {
-      this.updateForm().then(() => this.push());
+    if (key === 'workPackage') {
+      this.updateForm();
     }
   }
 

frontend/src/app/components/wp-edit/work-package-changeset.ts

@@ -6,10 +6,7 @@ export class WorkPackageChangeset extends ResourceChangeset<WorkPackageResource>
   public setValue(key:string, val:any) {
     super.setValue(key, val);
 
-    // Update the form for fields that may alter the form itself
-    // when the work package is new. Otherwise, the save request afterwards
-    // will update the form automatically.
-    if (this.pristineResource.isNew && (key === 'project' || key === 'type')) {
+    if (key === 'project' || key === 'type') {
       this.updateForm();
     }
   }

frontend/src/app/modules/fields/changeset/resource-changeset.ts

@@ -2,9 +2,11 @@ import {SchemaResource} from "core-app/modules/hal/resources/schema-resource";
 import {FormResource} from "core-app/modules/hal/resources/form-resource";
 import {HalResource} from "core-app/modules/hal/resources/hal-resource";
 import {ChangeMap, Changeset} from "core-app/modules/fields/changeset/changeset";
-import {InputState} from "reactivestates";
+import {input, InputState} from "reactivestates";
 import {IFieldSchema} from "core-app/modules/fields/field.base";
 import {debugLog} from "core-app/helpers/debug_output";
+import {take} from "rxjs/operators";
+import {Form} from "@angular/forms";
 
 /**
  * Temporary class living while a resource is being edited
@@ -21,7 +23,7 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
   protected changeset = new Changeset();
 
   /** Reference and load promise for the current form */
-  private formPromise:Promise<FormResource>|null;
+  protected form$ = input<FormResource>();
 
   /** Flag whether this is currently being saved */
   public inFlight = false;
@@ -40,7 +42,11 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
 
   constructor(public pristineResource:T,
               public readonly state?:InputState<ResourceChangeset<T>>,
-              public form:FormResource|null = null) {
+              loadedForm:FormResource|null = null) {
+
+    if (loadedForm) {
+      this.form$.putValue(loadedForm);
+    }
   }
 
   /**
@@ -56,25 +62,14 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
   /**
    * Build the request attributes against the fresh form
    */
-  public buildRequestPayload():Promise<[FormResource, Object]> {
+  public buildRequestPayload():Promise<Object> {
     return this
-      .updateForm()
-      .then(form => [form, this.buildPayloadFromChanges()]) as Promise<[FormResource, Object]>;
+      .getForm()
+      .then(() => this.buildPayloadFromChanges());
   }
 
 
 
-  /**
-   * Returns the current work package form.
-   * This may be different from the base form when project or type is changed.
-   */
-  public getForm():Promise<FormResource> {
-    if (!this.form) {
-      return this.updateForm();
-    } else {
-      return Promise.resolve(this.form);
-    }
-  }
 
   public getSchemaName(attribute:string):string {
     if (this.projectedResource.getSchemaName) {
@@ -85,29 +80,40 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
   }
 
   /**
-   * Update the form resource from the API.
+   * Returns the cached form or loads it if necessary.
+   */
+  public getForm():Promise<FormResource> {
+    if (this.form$.isPristine() && !this.form$.hasActivePromiseRequest()) {
+      return this.updateForm();
+    }
+
+    return this
+      .form$
+      .values$()
+      .pipe(take(1))
+      .toPromise();
+  }
+
+
+  /**
+   * Posts to the form with the current changes
+   * to get the up to date projected object.
    */
   protected updateForm():Promise<FormResource> {
     let payload = this.buildPayloadFromChanges();
 
-    if (!this.formPromise) {
-      this.formPromise = this.pristineResource.$links
-        .update(payload)
-        .then((form:FormResource) => {
-          this.formPromise = null;
-          this.form = form;
-          this.setNewDefaults(form);
-          this.push();
-          return form;
-        })
-        .catch((error:any) => {
-          this.formPromise = null;
-          this.form = null;
-          throw error;
-        }) as Promise<FormResource>;
-    }
-
-    return this.formPromise;
+    const promise = this.pristineResource
+      .$links
+      .update(payload)
+      .then((form:FormResource) => {
+        this.form$.putValue(form);
+        this.setNewDefaults(form);
+        this.push();
+        return form;
+      });
+
+    this.form$.putFromPromiseIfPristine(() => promise);
+    return promise;
   }
 
   /**
@@ -215,9 +221,9 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
   }
 
   public clear() {
-   this.state && this.state.clear();
-   this.changeset.clear();
-   this.form = null;
+    this.state && this.state.clear();
+    this.changeset.clear();
+    this.form$.clear();
   }
 
   /**
@@ -243,7 +249,7 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
    * and contains available values.
    */
   public get schema():SchemaResource {
-    return (this.form || this.pristineResource).schema;
+    return this.form$.getValueOr(this.pristineResource).schema;
   }
 
   protected get minimalPayload() {
@@ -259,8 +265,8 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
   protected applyChanges(plainPayload:any) {
     // Fall back to the last known state of the HalResource should the form not be loaded.
     let reference = this.pristineResource.$source;
-    if (this.form) {
-      reference = this.form.payload.$source;
+    if (this.form$.value) {
+      reference = this.form$.value.payload.$source;
     }
 
     _.each(this.changeset.all, (val:unknown, key:string) => {
@@ -291,8 +297,8 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
     if (this.pristineResource.isNew) {
       // If the resource is new, we need to pass the entire form payload
       // to let all default values be transmitted (type, status, etc.)
-      if (this.form) {
-        payload = this.form.payload.$source;
+      if (this.form$.value) {
+        payload = this.form$.value.payload.$source;
       } else {
         payload = this.pristineResource.$source;
       }
@@ -305,7 +311,7 @@ export class ResourceChangeset<T extends HalResource|{ [key:string]:unknown; } =
           .attachments
           .elements
           .map((a:HalResource) => {
-            return {href: a.href};
+            return { href: a.href };
           });
       }
 

frontend/src/app/modules/fields/edit/edit-form/edit-form.ts

@@ -89,7 +89,7 @@ export abstract class EditForm<T extends HalResource = HalResource> {
    */
   protected onSaved(isInitial:boolean, saved:HalResource):void {
     const eventType = isInitial ? 'created' : 'updated';
-    this.halEvents.push(saved, {eventType});
+    this.halEvents.push(saved, { eventType });
   }
 
   protected abstract focusOnFirstError():void;
@@ -168,6 +168,9 @@ export abstract class EditForm<T extends HalResource = HalResource> {
       return Promise.resolve(this.resource);
     }
 
+    // Mark changeset as in flight
+    this.change.inFlight = true;
+
     // Reset old error notifcations
     this.errorsPerAttribute = {};
 
@@ -188,6 +191,7 @@ export abstract class EditForm<T extends HalResource = HalResource> {
           this.halNotification.showSave(result.resource, result.wasNew);
           this.editMode = false;
           this.onSaved(result.wasNew, result.resource);
+          this.change.inFlight = false;
         })
         .catch((error:ErrorResource|Object) => {
           this.halNotification.handleRawError(error, this.resource);
@@ -196,6 +200,8 @@ export abstract class EditForm<T extends HalResource = HalResource> {
             this.handleSubmissionErrors(error);
             reject();
           }
+
+          this.change.inFlight = false;
         });
     });
   }

frontend/src/app/modules/fields/edit/services/hal-resource-editing.service.ts

@@ -103,17 +103,8 @@ export class HalResourceEditingService extends StateCacheService<ResourceChanges
   }
 
   public async save<V extends HalResource, T extends ResourceChangeset<V>>(change:T):Promise<ResourceChangesetCommit<V>> {
-    change.inFlight = true;
-
     // Form the payload we're going to save
-    const [form, payload] = await change.buildRequestPayload();
-    // Reject errors when occurring in form validation
-    const errors = form.getErrors();
-    if (errors !== null) {
-      change.inFlight = false;
-      throw(errors);
-    }
-
+    const payload = await change.buildRequestPayload();
     const savedResource = await change.pristineResource.$links.updateImmediately(payload);
 
     // Initialize any potentially new HAL values
@@ -121,8 +112,6 @@ export class HalResourceEditingService extends StateCacheService<ResourceChanges
 
     this.onSaved(savedResource);
 
-    change.inFlight = false;
-
     // Complete the change
     return this.complete(change, savedResource);
   }

frontend/src/app/modules/work_packages/routing/wp-full-view/wp-full-view.component.ts

@@ -32,12 +32,17 @@ import {Component, Injector, OnInit} from '@angular/core';
 import {WorkPackageViewSelectionService} from 'core-app/modules/work_packages/routing/wp-view-base/view-services/wp-view-selection.service';
 import {WorkPackageSingleViewBase} from "core-app/modules/work_packages/routing/wp-view-base/work-package-single-view.base";
 import {of} from "rxjs";
+import {HalResourceNotificationService} from "core-app/modules/hal/services/hal-resource-notification.service";
+import {WorkPackageNotificationService} from "core-app/modules/work_packages/notifications/work-package-notification.service";
 
 @Component({
   templateUrl: './wp-full-view.html',
   selector: 'wp-full-view-entry',
   // Required class to support inner scrolling on page
-  host: { 'class': 'work-packages-page--ui-view' }
+  host: { 'class': 'work-packages-page--ui-view' },
+  providers: [
+    { provide: HalResourceNotificationService, useExisting: WorkPackageNotificationService }
+  ]
 })
 export class WorkPackagesFullViewComponent extends WorkPackageSingleViewBase implements OnInit {
   // Watcher properties

spec/features/work_packages/details/custom_fields/custom_field_spec.rb

@@ -183,8 +183,10 @@ describe 'custom field inplace editor', js: true do
         field.set_value ''
         field.expect_invalid
 
-        expect(WorkPackages::UpdateService).not_to receive(:new)
         field.save!
+
+        work_package.reload
+        expect(work_package.send("custom_field_#{custom_field.id}")).to eq 123
       end
     end
   end

spec/features/work_packages/table/edit_work_packages_spec.rb

@@ -110,8 +110,8 @@ describe 'Inline editing work packages', js: true do
       subject_field.set_value('')
       subject_field.expect_invalid
 
-      expect(WorkPackages::UpdateService).not_to receive(:new)
       subject_field.save!
+      expect(work_package.reload.subject).to eq 'Foobar'
     end
   end