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

Author: lozcalver

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/src/components/LinkField/LinkField.js

@@ -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/tests/LinkField-test.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": {

composer.json

@@ -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
@@ -322,9 +330,8 @@ private function createLinkForm(Link $link, string $operation, bool $excludeLink
         $name = sprintf(LinkFieldController::FORM_NAME_TEMPLATE, $id);
         /** @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();
 
         // Remove LinkText if appropriate
@@ -387,6 +394,9 @@ private function getFieldIsReadonlyOrDisabled(): bool
     {
         $ownerClass = $this->getOwnerClassFromRequest();
         $ownerRelation = $this->getOwnerRelationFromRequest();
+        if (!$ownerClass || !$ownerRelation) {
+            return false;
+        }
 
         /** @var LinkField|MultiLinkField $field */
         $field = Injector::inst()->get($ownerClass)->getCMSFields()->dataFieldByName($ownerRelation);
@@ -480,12 +490,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;
@@ -498,22 +508,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();
@@ -536,21 +546,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');
     }
 }

src/Controllers/LinkFieldController.php

@@ -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/Form/AbstractLinkField.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);
+        } else if ($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

src/Models/Link.php

@@ -211,3 +211,61 @@ I want to add links to pages, files, external URLs, email addresses and phone nu
     And I should see "File1" in the "[data-field-id='Form_EditForm_HasManyLinks'] .link-picker__link--is-first" element
     And I should see "folder1/file1.jpg" in the "[data-field-id='Form_EditForm_HasManyLinks'] .link-picker__link--is-first" element
     And I should see "Draft" in the "[data-field-id='Form_EditForm_HasManyLinks'] .link-picker__link--is-first" element
+
+  Scenario: Create links on an unsaved record
+    Given I add an extension "SilverStripe\LinkField\Tests\Behat\Context\Extension\LinkCompanyExtension" to the "SilverStripe\FrameworkTest\Model\Company" class
+    And the "group" "EDITOR" has permissions "Access to 'Test ModelAdmin' section" and "TEST_DATAOBJECT_EDIT"
+    When I go to "/admin/test"
+    Then I press the "Add new Company" button
+    And I wait for 2 seconds
+
+    # Test that the single + multiple link fields are visible on unsaved records
+    Then I should see the "[data-field-id='Form_ItemEditForm_CompanyWebSiteLink'] button" element
+    And I should see the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] button" element
+
+    # Test saving - first add a link to the single link field
+    Then I click on the "[data-field-id='Form_ItemEditForm_CompanyWebSiteLink'] button" element
+    And I click on the "[data-field-id='Form_ItemEditForm_CompanyWebSiteLink'] .dropdown-item:nth-of-type(1)" element
+    Then I fill in "LinkText" with "External URL"
+    And I fill in "ExternalUrl" with "https://www.silverstripe.org"
+    And I press the "Create link" button
+    And I wait for 2 seconds
+
+    # Then add the first link (email link) to the multi link field
+    Then I click on the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] button" element
+    And I click on the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] .dropdown-item:nth-of-type(4)" element
+    And I wait for 5 seconds
+    Then I fill in "LinkText" with "Email link"
+    And I fill in "Email" with "email@example.com"
+    And I press the "Create link" button
+    And I wait for 2 seconds
+
+    # Then add the second link (phone link) to the multi link field
+    Then I click on the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] button" element
+    And I click on the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] .dropdown-item:nth-of-type(5)" element
+    And I wait for 5 seconds
+    Then I fill in "LinkText" with "Phone"
+    And I fill in "Phone" with "12345678"
+    And I press the "Create link" button
+    And I wait for 2 seconds
+
+    # Fill out required field on Company records
+    And I fill in "Name" with "A company with links"
+    And I press the "Create" button
+    And I wait for 2 seconds
+    Then I should see a "Saved company" message
+
+    # Reload the page to ensure links have saved, and haven't just been re-populated from POST data
+    Then I reload the page
+
+    # Check links are present - single link
+    And I should see "External URL" in the "[data-field-id='Form_ItemEditForm_CompanyWebSiteLink']" element
+    And I should see "https://www.silverstripe.org" in the "[data-field-id='Form_ItemEditForm_CompanyWebSiteLink'] .link-picker__link" element
+
+    # Check links are present - multi link 1
+    And I should see "Email link" in the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] .link-picker__link:nth-of-type(1)" element
+    And I should see "email@example.com" in the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] .link-picker__link:nth-of-type(1)" element
+
+    # Check links are present - multi link 2
+    And I should see "Phone number" in the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] .link-picker__link:nth-of-type(2)" element
+    And I should see "12345678" in the "[data-field-id='Form_ItemEditForm_ManyCompanyWebSiteLink'] .link-picker__link:nth-of-type(2)" element

tests/behat/features/create-edit-linkfield.feature

@@ -0,0 +1,27 @@
+<?php
+
+namespace SilverStripe\LinkField\Tests\Behat\Context\Extension;
+
+use SilverStripe\Core\Extension;
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\Forms\FieldList;
+use SilverStripe\LinkField\Form\LinkField;
+use SilverStripe\LinkField\Form\MultiLinkField;
+
+class LinkCompanyExtension extends Extension implements TestOnly
+{
+    protected function updateCMSFields(FieldList $fields): void
+    {
+        // Re-create these fields so that has_many field is displayed on unsaved records (it would be skipped during
+        // automatic scaffolding) and to ensure they're at the top of the form so are visible in any screenshots
+        $fields->removeByName(['CompanyWebSiteLink', 'ManyCompanyWebSiteLink']);
+        $fields->addFieldsToTab(
+            'Root.Main',
+            [
+                LinkField::create('CompanyWebSiteLink', 'Company web site link'),
+                MultiLinkField::create('ManyCompanyWebSiteLink', 'Many company web site links'),
+            ],
+            'Category',
+        );
+    }
+}