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.

Sign up for GitHub

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

NEW SingleRecordAdmin class for editing one record at a time #1842

Merged
merged 1 commit into from
Nov 6, 2024
Merged

NEW SingleRecordAdmin class for editing one record at a time #1842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
91 changes: 25 additions & 66 deletions code/CMSProfileController.php
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

Anything removed from this class was either not necessary (likely related to legacy CMS 3 or older code that no longer exists) or is already being done in one of the superclasses.

Original file line number Diff line number Diff line change
Expand Up @@ -2,116 +2,75 @@

namespace SilverStripe\Admin;

use SilverStripe\CMS\Controllers\CMSMain;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Forms\Form;
use SilverStripe\Forms\HiddenField;
use SilverStripe\Forms\FormAction;
use SilverStripe\Model\List\ArrayList;
use SilverStripe\Security\Member;
use SilverStripe\Security\Permission;
use SilverStripe\Security\Security;

class CMSProfileController extends LeftAndMain
class CMSProfileController extends SingleRecordAdmin
{
private static $url_segment = 'myprofile';

private static $menu_title = 'My Profile';

private static $required_permission_codes = 'CMS_ACCESS';

/**
* @deprecated 5.4.0 Will be renamed to model_class
*/
private static $tree_class = Member::class;
private static $model_class = Member::class;
Comment on lines -26 to +19
Copy link
Member Author

Choose a reason for hiding this comment

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

See change in LeftAndMain


private static $ignore_menuitem = true;

public function getEditForm($id = null, $fields = null)
{
$this->setCurrentPageID(Security::getCurrentUser()->ID);

$form = parent::getEditForm($id, $fields);
private static bool $restrict_to_single_record = false;

if ($form instanceof HTTPResponse) {
return $form;
}
private static bool $allow_new_record = false;

$form->Fields()->removeByName('LastVisited');
$form->Fields()->push(new HiddenField('ID', null, Security::getCurrentUser()->ID));
$form->Actions()->push(
FormAction::create('save', _t(CMSMain::class . '.SAVE', 'Save'))
->addExtraClass('btn-primary font-icon-save')
->setUseButtonTag(true)
);

$form->Actions()->removeByName('action_delete');

if ($member = Security::getCurrentUser()) {
$form->setValidator($member->getValidator());
} else {
$form->setValidator(Member::singleton()->getValidator());
}

if ($form->Fields()->hasTabSet()) {
$form->Fields()->findOrMakeTab('Root')->setTemplate('SilverStripe\\Forms\\CMSTabSet');
protected function init()
{
parent::init();
if (!$this->getResponse()->isRedirect()) {
$this->setCurrentPageID(Security::getCurrentUser()->ID);
Copy link
Member Author

Choose a reason for hiding this comment

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

Moving this into init instead of getEditForm so it takes affect for all actions on this controller (including any that might be added through extensions).

Skipping for redirects because at that point we're not doing anything in this controller anyway.

}
}

$form->addExtraClass('member-profile-form root-form cms-edit-form center fill-height');

public function getEditForm($id = null, $fields = null): Form
{
$form = parent::getEditForm($id, $fields);
$form->Fields()->push(HiddenField::create('ID', null, Security::getCurrentUser()->ID));
return $form;
}

public function canView($member = null)
{
$currentUser = Security::getCurrentUser();

if (!$member && $member !== false) {
$member = $currentUser;
}

// cms menus only for logged-in members
// cms menus are only for logged-in members
if (!$member) {
return false;
}

// Check they are trying to edit themselves and they have permissions
return $member->ID === $currentUser->ID && parent::canView($member);
}

public function save(array $data, Form $form): HTTPResponse
{
$member = Member::get()->byID($data['ID']);
if (!$member) {
$this->httpError(404);
}
$origLocale = $member->Locale;

if (!$member->canEdit()) {
$form->sessionMessage(_t(__CLASS__.'.CANTEDIT', 'You don\'t have permission to do that'), 'bad');
return $this->redirectBack();
// Make sure the ID is a positive number
$id = $data['ID'] ?? null;
if ((!is_int($id) && !ctype_digit($id)) || $id < 1) {
$this->httpError(400);
}

// Get the current member locale so we can see if it changes
$member = Member::get()->byID($id);
$origLocale = $member?->Locale;
// actual save (along with error handling e.g. if there's no member with that ID) is handled by the parent class
$response = parent::save($data, $form);

// If the member locale changes, reload the full CMS so localisation can kick in.
// Otherwise only the edit form will reload.
if (isset($data['Locale']) && $origLocale != $data['Locale']) {
$response->addHeader('X-Reload', true);
$response->addHeader('X-ControllerURL', $this->Link());
}

return $response;
}

/**
* Only show first element, as the profile form is limited to editing
* the current member it doesn't make much sense to show the member name
* in the breadcrumbs.
*
* @param bool $unlinked
*/
public function Breadcrumbs($unlinked = false)
{
$items = parent::Breadcrumbs($unlinked);
return new ArrayList([$items[0]]);
}
}
43 changes: 13 additions & 30 deletions code/LeftAndMain.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class LeftAndMain extends AdminController implements PermissionProvider
* @var string
* @deprecated 5.4.0 Will be renamed to model_class
*/
private static $tree_class = null;
private static $model_class = null;
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved

/**
* @var array
Expand Down Expand Up @@ -939,7 +939,7 @@ public function PreviewPanel()
*/
public function getRecord($id)
{
$className = $this->config()->get('tree_class');
$className = $this->config()->get('model_class');
if (!$className) {
return null;
}
Expand Down Expand Up @@ -1005,7 +1005,7 @@ protected function getSearchFilter()
public function save(array $data, Form $form): HTTPResponse
{
$request = $this->getRequest();
$className = $this->config()->get('tree_class');
$className = $this->config()->get('model_class');

// Existing or new record?
$id = $data['ID'];
Expand All @@ -1018,7 +1018,7 @@ public function save(array $data, Form $form): HTTPResponse
$this->httpError(404, "Bad record ID #" . (int)$id);
}
} else {
if (!singleton($this->config()->get('tree_class'))->canCreate()) {
if (!singleton($this->config()->get('model_class'))->canCreate()) {
return Security::permissionFailure($this);
}
$record = $this->getNewItem($id, false);
Expand Down Expand Up @@ -1054,7 +1054,7 @@ public function save(array $data, Form $form): HTTPResponse
*/
public function getNewItem($id, $setID = true)
{
$class = $this->config()->get('tree_class');
$class = $this->config()->get('model_class');
$object = Injector::inst()->create($class);
if ($setID) {
$object->ID = $id;
Expand All @@ -1064,7 +1064,7 @@ public function getNewItem($id, $setID = true)

public function delete(array $data, Form $form): HTTPResponse
{
$className = $this->config()->get('tree_class');
$className = $this->config()->get('model_class');

$id = $data['ID'];
$record = DataObject::get_by_id($className, $id);
Expand Down Expand Up @@ -1096,7 +1096,7 @@ public function delete(array $data, Form $form): HTTPResponse
* method in an entwine subclass. This method can accept a record identifier,
* selected either in custom logic, or through {@link currentPageID()}.
* The form usually construct itself from {@link DataObject->getCMSFields()}
* for the specific managed subclass defined in {@link LeftAndMain::$tree_class}.
* for the specific managed subclass defined in {@link LeftAndMain::$model_class}.
*
* @param HTTPRequest $request Passed if executing a HTTPRequest directly on the form.
* If empty, this is invoked as $EditForm in the template
Expand Down Expand Up @@ -1149,8 +1149,8 @@ public function getEditForm($id = null, $fields = null)
$fields->push(new HiddenField('ClassName'));
}

$tree_class = $this->config()->get('tree_class');
if ($tree_class::has_extension(Hierarchy::class)
$modelClass = $this->config()->get('model_class');
if ($modelClass::has_extension(Hierarchy::class)
&& !$fields->dataFieldByName('ParentID')
) {
$fields->push(new HiddenField('ParentID'));
Expand All @@ -1171,14 +1171,14 @@ public function getEditForm($id = null, $fields = null)
if (!$actions || !$actions->count()) {
if ($record->hasMethod('canEdit') && $record->canEdit()) {
$actions->push(
FormAction::create('save', _t(CMSMain::class . '.SAVE', 'Save'))
FormAction::create('save', _t(__CLASS__ . '.SAVE', 'Save'))
->addExtraClass('btn btn-primary')
->addExtraClass('font-icon-add-circle')
);
}
if ($record->hasMethod('canDelete') && $record->canDelete()) {
$actions->push(
FormAction::create('delete', _t(ModelAdmin::class . '.DELETE', 'Delete'))
FormAction::create('delete', _t(__CLASS__ . '.DELETE', 'Delete'))
->addExtraClass('btn btn-secondary')
);
}
Expand Down Expand Up @@ -1221,24 +1221,7 @@ public function getEditForm($id = null, $fields = null)
$form->addExtraClass('cms-previewable');
}
$form->addExtraClass('fill-height');

if ($record->hasMethod('getCMSCompositeValidator')) {
// As of framework v4.7, a CompositeValidator is always available form a DataObject, but it may be
// empty (which is fine)
$form->setValidator($record->getCMSCompositeValidator());
} elseif ($record->hasMethod('getCMSValidator')) {
// BC support for framework < v4.7
$validator = $record->getCMSValidator();

// The clientside (mainly LeftAndMain*.js) rely on ajax responses
// which can be evaluated as javascript, hence we need
// to override any global changes to the validation handler.
if ($validator) {
$form->setValidator($validator);
}
Comment on lines -1225 to -1238
Copy link
Member Author

Choose a reason for hiding this comment

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

All of this boils down to $form->setValidator($record->getCMSCompositeValidator()), because all DataObject classes have that method, and $record will always be a DataObject.

} else {
$form->unsetValidator();
}
Comment on lines -1239 to -1241
Copy link
Member Author

Choose a reason for hiding this comment

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

That method just sets it null anyway so even if somehow getCMSCompositeValidator() returned null (which it can't) the result would be the same.

$form->setValidator($record->getCMSCompositeValidator());

// Check if this form is readonly
if (!$record->canEdit()) {
Expand Down Expand Up @@ -1330,7 +1313,7 @@ public function EditFormTools()
*/
public function batchactions()
{
return new CMSBatchActionHandler($this, 'batchactions', $this->config()->get('tree_class'));
return new CMSBatchActionHandler($this, 'batchactions', $this->config()->get('model_class'));
}

/**
Expand Down
5 changes: 1 addition & 4 deletions code/SecurityAdmin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,7 @@ class SecurityAdmin extends ModelAdmin implements PermissionProvider

private static $menu_priority = 0;

/**
* @deprecated 5.4.0 Will be renamed to model_class
*/
private static $tree_class = Group::class;
private static $model_class = Group::class;
Comment on lines -64 to +61
Copy link
Member Author

Choose a reason for hiding this comment

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

I strongly suspect this config doesn't actually do anything for this class, but checking that is out of scope for this PR. I'm just updating it to the config that LeftAndMain expects.


private static $required_permission_codes = 'CMS_ACCESS_SecurityAdmin';

Expand Down
56 changes: 56 additions & 0 deletions code/SingleRecordAdmin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php

namespace SilverStripe\Admin;

use SilverStripe\Forms\Form;
use SilverStripe\ORM\DataObject;

/**
* The base class for admin sections that exist for editing a single record.
*/
abstract class SingleRecordAdmin extends LeftAndMain
{
/**
* Determines if there should be a single record in the database that this admin edits.
*
* If false, you need to provide a mechanism to tell the form which record it should be editing.
* This could be an action (e.g. edit/$ID), or could be based on something about the current member, etc.
*/
private static bool $restrict_to_single_record = true;

/**
* If no record exists, allow a new one to be created
*/
private static bool $allow_new_record = true;
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved

public function getEditForm($id = null, $fields = null): ?Form
{
$form = parent::getEditForm($id, $fields);
// Keep the tabs on the top row, rather than underneath
if ($form?->Fields()->hasTabSet()) {
$form->Fields()->findOrMakeTab('Root')->setTemplate('SilverStripe/Forms/CMSTabSet');
}
return $form;
}

public function getRecord($id): ?DataObject
{
if (!$id) {
return $this->getSingleRecord();
}
return parent::getRecord($id);
Copy link
Member

Choose a reason for hiding this comment

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

What's the scenario where $id is truthy? Seems like we should be throwing an exception here instead?

Copy link
Member Author

@GuySartorelli GuySartorelli Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

$id is truthy if an ID is passed in. So we either want the "single record" based on the configuration (where a null or 0 is passed in), or we want the specified record if a specific ID is passed in.

}

protected function getSingleRecord(): ?DataObject
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
protected function getSingleRecord(): ?DataObject
private function getSingleRecord(): ?DataObject

If we're providing only_one_record and allow_new_record and implementing logic to do stuff based on how they're set, why would anyone need to override this in a subclass? Things should be private unless there's an immediate need to make it protected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please see the docs where overriding this method is explicitly mentioned as a way to provide more complex behaviour for getting this record.

{
$record = null;
$modelClass = static::config()->get('model_class');
if (static::config()->get('restrict_to_single_record')) {
$record = DataObject::get_one($modelClass);
}
if (!$record && static::config()->get('allow_new_record')) {
$record = $modelClass::create();
}
return $record;
}
}
4 changes: 4 additions & 0 deletions tests/behat/features/profile.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,7 @@ Feature: Manage my own settings
And I select "German (Germany)" from "Interface Language"
And I press the "Save" button
Then I should see "Sprache"

Scenario: Breadcrumbs are in the correct place
Then I should see "Main" in the ".cms-content-header-tabs" element
And I should not see "Main" in the "#Form_EditForm" element
2 changes: 1 addition & 1 deletion tests/php/LeftAndMainTest/MyTreeController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class MyTreeController extends LeftAndMain implements TestOnly
{
private static $url_segment = 'mytree/edit';

private static $tree_class = MyTree::class;
private static $model_class = MyTree::class;

private static $allowed_actions = [
'EditForm'
Expand Down
Loading
Loading