Merge pull request #425 from bigfork/link-before-save

ENH: Allow adding link(s) to unsaved DataObjects (closes #387)

Author: GuySartorelli

client/dist/js/bundle.js

@@ -545,14 +545,10 @@ const LinkField = ({
       });
   }
 
-  const saveRecordFirst = !loadingError && ownerID === 0;
   const renderLoadingError = loadingError;
-  const renderPicker = !loadingError && !inHistoryViewer && !saveRecordFirst && (isMulti || linkIDs.length === 0);
-  const renderModal = !loadingError && !saveRecordFirst && Boolean(editingID);
+  const renderPicker = !loadingError && !inHistoryViewer && (isMulti || linkIDs.length === 0);
+  const renderModal = !loadingError && Boolean(editingID);
   const loadingErrorText = i18n._t('LinkField.FAILED_TO_LOAD_LINKS', 'Failed to load link(s)');
-  const saveRecordFirstText = isMulti
-    ? i18n._t('LinkField.SAVE_RECORD_FIRST', 'Cannot add links until the record has been saved')
-    : i18n._t('LinkField.SAVE_RECORD_FIRST_SINGLE', 'Cannot add link until the record has been saved');
   const links = renderLinks();
 
   return <LinkFieldContext.Provider value={{
@@ -566,8 +562,7 @@ const LinkField = ({
   }}>
     <div className="link-field__container">
       { renderLoadingError && <div className="link-field__loading-error">{loadingErrorText}</div> }
-      { saveRecordFirst && <div className="link-field__save-record-first">{saveRecordFirstText}</div>}
-      { loading && !isSorting && !saveRecordFirst && <Loading containerClass="link-field__loading"/> }
+      { loading && !isSorting && <Loading containerClass="link-field__loading"/> }
       { renderPicker && <LinkPicker
           onModalSuccess={handleModalSuccess}
           onModalClosed={handleModalClosed}

client/dist/styles/bundle.css

@@ -219,13 +219,12 @@ test('LinkField tab order', async () => {
   // e.g. el.getBoundingClientRect() will always return 0,0,0,0
 });
 
-test('LinkField will render save-record-first div if ownerID is 0', async () => {
+test('LinkField will render loading indicator if ownerID is 0', async () => {
   const { container } = render(<LinkField {...makeProps({
     ownerID: 0
   })}
   />);
-  expect(container.querySelectorAll('.link-field__save-record-first')).toHaveLength(1);
-  expect(container.querySelectorAll('.link-field__loading')).toHaveLength(0);
+  expect(container.querySelectorAll('.link-field__loading')).toHaveLength(1);
   expect(container.querySelectorAll('.link-picker')).toHaveLength(0);
 });
 
@@ -234,11 +233,28 @@ test('LinkField will render loading indicator if ownerID is not 0', async () =>
     ownerID: 1
   })}
   />);
-  expect(container.querySelectorAll('.link-field__save-record-first')).toHaveLength(0);
   expect(container.querySelectorAll('.link-field__loading')).toHaveLength(1);
   expect(container.querySelectorAll('.link-picker')).toHaveLength(0);
 });
 
+test('LinkField will render link-picker if ownerID is 0 and isMulti and has finished loading', async () => {
+  const { container } = render(<LinkField {...makeProps({
+    ownerID: 0,
+    isMulti: true,
+  })}
+  />);
+  await doResolve({ json: () => ({
+    123: {
+      title: 'First title',
+      typeKey: 'mylink',
+    },
+  }) });
+  await screen.findByText('First title');
+
+  expect(container.querySelectorAll('.link-field__loading')).toHaveLength(0);
+  expect(container.querySelectorAll('.link-picker')).toHaveLength(1);
+});
+
 test('LinkField will render link-picker if ownerID is not 0 and isMulti and has finished loading', async () => {
   const { container } = render(<LinkField {...makeProps({
     ownerID: 1,
@@ -253,7 +269,6 @@ test('LinkField will render link-picker if ownerID is not 0 and isMulti and has
   }) });
   await screen.findByText('First title');
 
-  expect(container.querySelectorAll('.link-field__save-record-first')).toHaveLength(0);
   expect(container.querySelectorAll('.link-field__loading')).toHaveLength(0);
   expect(container.querySelectorAll('.link-picker')).toHaveLength(1);
 });

client/src/components/LinkField/LinkField.js

@@ -32,7 +32,8 @@
     "autoload": {
         "psr-4": {
             "SilverStripe\\LinkField\\": "src/",
-            "SilverStripe\\LinkField\\Tests\\": "tests/php/"
+            "SilverStripe\\LinkField\\Tests\\": "tests/php/",
+            "SilverStripe\\LinkField\\Tests\\Behat\\Context\\": "tests/behat/src/"
         }
     },
     "scripts": {

client/src/components/LinkField/tests/LinkField-test.js

@@ -227,9 +227,17 @@ public function save(array $data, Form $form): HTTPResponse
         // Get owner using getOwnerFromRequest() rather than $link->Owner() so that validation is run
         // on the owner params before updating the database
         $owner = $this->getOwnerFromRequest();
+        if (!$owner) {
+            $className = $this->getOwnerClassFromRequest();
+            $owner = $className ? Injector::inst()->get($className) : null;
+        }
         $ownerRelation = $this->getOwnerRelationFromRequest();
+        if (!$owner || !$ownerRelation) {
+            $this->jsonError(404);
+        }
+
         $ownerRelationID = "{$ownerRelation}ID";
-        $hasOne = Injector::inst()->get($owner->ClassName)->hasOne();
+        $hasOne = $owner->hasOne();
         if ($operation === 'create'
             && array_key_exists($ownerRelation, $hasOne)
             && $owner->$ownerRelationID !== $link->ID
@@ -323,10 +331,14 @@ private function createLinkForm(Link $link, string $operation, bool $excludeLink
         /** @var Form $form */
         $form = $formFactory->getForm($this, $name, ['Record' => $link]);
         $owner = $this->getOwnerFromRequest();
-        $ownerID = $owner->ID;
-        $ownerClassName = $owner->ClassName;
+        $ownerID = $this->getOwnerIDFromRequest();
+        $ownerClassName = $this->getOwnerClassFromRequest();
         $ownerRelation = $this->getOwnerRelationFromRequest();
 
+        if (!$owner) {
+            $owner = Injector::inst()->create($ownerClassName);
+        }
+
         // Remove LinkText if appropriate
         if ($excludeLinkTextField) {
             $form->Fields()->removeByName('LinkText');
@@ -477,12 +489,12 @@ private function typeKeyFromRequest(): string
     /**
      * Get the owner class based on the query string param OwnerClass
      */
-    private function getOwnerClassFromRequest(): string
+    private function getOwnerClassFromRequest(): string|null
     {
         $request = $this->getRequest();
         $ownerClass = $request->getVar('ownerClass') ?: $request->postVar('OwnerClass');
         if (!is_a($ownerClass, DataObject::class, true)) {
-            $this->jsonError(404);
+            return null;
         }
 
         return $ownerClass;
@@ -495,22 +507,22 @@ private function getOwnerIDFromRequest(): int
     {
         $request = $this->getRequest();
         $ownerID = (int) ($request->getVar('ownerID') ?: $request->postVar('OwnerID'));
-        if ($ownerID === 0) {
-            $this->jsonError(404);
-        }
-
         return $ownerID;
     }
 
     /**
      * Get the owner based on the query string params ownerID, ownerClass, ownerRelation
      * OR the POST vars OwnerID, OwnerClass, OwnerRelation
      */
-    private function getOwnerFromRequest(): DataObject
+    private function getOwnerFromRequest(): DataObject|null
     {
         $ownerID = $this->getOwnerIDFromRequest();
         $ownerClass = $this->getOwnerClassFromRequest();
         $ownerRelation = $this->getOwnerRelationFromRequest();
+        if (!$ownerID || !$ownerClass || !$ownerRelation) {
+            return null;
+        }
+
         /** @var DataObject $obj */
         $obj = Injector::inst()->get($ownerClass);
         $hasOne = $obj->hasOne();
@@ -533,21 +545,16 @@ private function getOwnerFromRequest(): DataObject
                 return $owner;
             }
         }
-        $this->jsonError(404);
+        return null;
     }
 
     /**
      * Get the owner relation based on the query string param ownerRelation
      * OR the POST var OwnerRelation
      */
-    private function getOwnerRelationFromRequest(): string
+    private function getOwnerRelationFromRequest(): string|null
     {
         $request = $this->getRequest();
-        $ownerRelation = $request->getVar('ownerRelation') ?: $request->postVar('OwnerRelation');
-        if (!$ownerRelation) {
-            $this->jsonError(404);
-        }
-
-        return $ownerRelation;
+        return $request->getVar('ownerRelation') ?: $request->postVar('OwnerRelation');
     }
 }

composer.json

@@ -16,6 +16,7 @@
 use SilverStripe\LinkField\Models\Link;
 use SilverStripe\LinkField\Services\LinkTypeService;
 use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\DataObjectInterface;
 use SilverStripe\VersionedAdmin\Controllers\HistoryViewerController;
 
 /**
@@ -244,4 +245,53 @@ private function validateTypes(array $types): void
             }
         }
     }
+
+    public function saveInto(DataObjectInterface $record)
+    {
+        parent::saveInto($record);
+
+        // We only need to run additional checks on records which are yet to be saved
+        if ($record->isInDB()) {
+            return;
+        }
+
+        $linkIDs = $this->value;
+        if (!is_array($linkIDs)) {
+            $linkIDs = explode(',', (string)$this->value);
+        }
+
+        foreach (array_filter($linkIDs) as $linkID) {
+            // Search for a matching link, ensuring that it’s yet to have its owner assigned
+            $link = Link::get()->filter([
+                'OwnerID' => 0,
+                'OwnerClass' => $record->ClassName,
+                'ID' => $linkID,
+            ])->first();
+            if (!$link) {
+                continue;
+            }
+
+            // Re-check canCreate() against a blank record as an extra check that the user can definitely create new
+            // links of this type, and they're not trying to hijack an orphaned record created by another user
+            $className = $link->ClassName;
+            $singletonLink = $className::create();
+            if (!$singletonLink->canCreate()) {
+                continue;
+            }
+
+            if (array_key_exists($this->name, $record->hasOne())) {
+                // As $record isn't written yet, we can't immediately write the OwnerID to the link. We also can't use
+                // setComponent() as that would immediately trigger a write on the owner object, when the form handler
+                // may opt not to write the record. There's also no UnsavedRelationList for has_one, so we have to use
+                // an extension hook instead to ensure that if the owner is written, the ownership relation is saved
+                $record->beforeExtending('onAfterWrite', function () use ($link, $record) {
+                    $link->OwnerID = $record->ID;
+                    $link->write();
+                });
+            } elseif (array_key_exists($this->name, $record->hasMany())) {
+                // Has_many is much cleaner as we can just utilise UnsavedRelationList
+                $record->{$this->name}()->add($link);
+            }
+        }
+    }
 }

src/Controllers/LinkFieldController.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\LinkField\Models;
 
 use SilverStripe\Core\ClassInfo;
+use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\FormField;
 use SilverStripe\LinkField\Services\LinkTypeService;
@@ -355,6 +356,10 @@ private function canPerformAction(string $canMethod, $member, $context = [])
                 $canMethod = 'canEdit';
             }
             return $owner->$canMethod($member, $context);
+        } elseif ($this->OwnerClass) {
+            // If owner doesn't exist yet, check the user can create new owner records
+            $owner = Injector::inst()->create($this->OwnerClass);
+            return $owner->canCreate($member, $context);
         }
 
         // Default to DataObject's permission checks