Skip to content
Closed
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
2 changes: 1 addition & 1 deletion client/dist/js/bundle.js

Large diffs are not rendered by default.

11 changes: 3 additions & 8 deletions client/src/components/LinkField/LinkField.js
Original file line number Diff line number Diff line change
Expand Up @@ -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={{
Expand All @@ -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}
Expand Down
25 changes: 20 additions & 5 deletions client/src/components/LinkField/tests/LinkField-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand All @@ -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,
Expand All @@ -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);
});
43 changes: 24 additions & 19 deletions src/Controllers/LinkFieldController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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();
Expand All @@ -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');
}
}
50 changes: 50 additions & 0 deletions src/Form/AbstractLinkField.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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);
}
}
}
}
60 changes: 60 additions & 0 deletions tests/behat/features/create-edit-linkfield.feature
Original file line number Diff line number Diff line change
Expand Up @@ -211,3 +211,63 @@ 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\FrameworkTest\LinkField\Extensions\LinkPageExtension" to the "Company" class
And I go to "/dev/build?flush"
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

# Test that the single + multiple link fields are visible on unsaved records
Then I should see the "[data-field-id='Form_EditForm_HasOneLink'] button" element
And I should see the "[data-field-id='Form_EditForm_HasManyLinks'] button" element

# Test saving - first add a link to the single link field
Then I click on the "[data-field-id='Form_EditForm_HasOneLink'] .dropdown-item:nth-of-type(2)" 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 first link (external link) to the multi link field
Then I click on the "[data-field-id='Form_EditForm_HasManyLinks'] button" element
And I click on the "[data-field-id='Form_EditForm_HasManyLinks'] .dropdown-item:nth-of-type(3)" element
And I wait for 5 seconds
Then I fill in "LinkText" with "External URL"
And I fill in "ExternalUrl" with "https://www.silverstripe.org"
And I check "Open in new window"
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_EditForm_HasManyLinks'] button" element
And I click on the "[data-field-id='Form_EditForm_HasManyLinks'] .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 "Email link" in the "[data-field-id='Form_EditForm_HasOneLink']" element
And I should see "email@example.com" in the "[data-field-id='Form_EditForm_HasOneLink'] .link-picker__link" element

# Check links are present - multi link 1
And I should see "External URL" in the "[data-field-id='Form_EditForm_HasManyLinks'] .link-picker__link:nth-of-type(1)" element
And I should see "https://www.silverstripe.org" in the "[data-field-id='Form_EditForm_HasManyLinks'] .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_EditForm_HasManyLinks'] .link-picker__link:nth-of-type(2)" element
And I should see "12345678" in the "[data-field-id='Form_EditForm_HasManyLinks'] .link-picker__link:nth-of-type(2)" element

47 changes: 47 additions & 0 deletions tests/php/Form/AbstractLinkFieldTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Forms\FieldList;
use SilverStripe\LinkField\Form\LinkField;
use SilverStripe\LinkField\Form\MultiLinkField;
use SilverStripe\LinkField\Tests\Form\AbstractLinkFieldTest\TestBlock;
use SilverStripe\LinkField\Tests\Controllers\LinkFieldControllerTest\TestPhoneLink;
use SilverStripe\Forms\Form;
Expand All @@ -14,12 +15,14 @@
use SilverStripe\LinkField\Models\Link;
use SilverStripe\LinkField\Models\PhoneLink;
use SilverStripe\LinkField\Models\EmailLink;
use SilverStripe\LinkField\Tests\Models\LinkTest\LinkOwner;

class AbstractLinkFieldTest extends SapphireTest
{
protected static $fixture_file = 'AbstractLinkFieldTest.yml';

protected static $extra_dataobjects = [
LinkOwner::class,
TestBlock::class,
TestPhoneLink::class,
];
Expand Down Expand Up @@ -72,4 +75,48 @@ private function getKeysForAllowedTypes(LinkField $field): array
sort($keys);
return $keys;
}

public function testSaveIntoAssignsMissingRelations(): void
{
// Test has_one links via LinkField
$owner = new LinkOwner();
$link = new Link(['OwnerClass' => LinkOwner::class]);
$link->write();

$linkField = new LinkField('Link');
$linkField->setSubmittedValue($link->ID);
$linkField->saveInto($owner);
$owner->write();

// Check the main relation to the link is stored
$this->assertEquals($owner->LinkID, $link->ID);

// Check the reverse relation from the link to the "owner" object is stored
// Re-fetch link as only the ID is passed to the form field, so $link object instance will be out of date
$link = Link::get()->byID($link->ID);
$this->assertEquals($link->Owner()?->ID, $owner->ID);

// Test has_many links via MultiLinkField
$owner = new LinkOwner();
$linkList1 = new Link(['OwnerClass' => LinkOwner::class]);
$linkList1->write();
$linkList2 = new Link(['OwnerClass' => LinkOwner::class]);
$linkList2->write();

$multiLinkField = new MultiLinkField('LinkList');
// POST-ed value for MultiLinkField is a string in the format [1,2,3]
$multiLinkField->setSubmittedValue('[' . implode(',', [$linkList1->ID, $linkList2->ID]) . ']');
$multiLinkField->saveInto($owner);
$owner->write();

// Check the main relations to the links are stored
$this->assertListContains([['ID' => $linkList1->ID], ['ID' => $linkList2->ID]], $owner->LinkList());

// Check the reverse relation from the links to the "owner" object are stored
foreach ([$linkList1, $linkList2] as $link) {
// Re-fetch link as only the ID is passed to the form field, so $link object instance will be out of date
$link = Link::get()->byID($link->ID);
$this->assertEquals($link->Owner()?->ID, $owner->ID);
}
}
}
Loading