ENH Add Readonly field status #172

sabina-talipova · 2024-01-16T00:48:48Z

Description

The developer can set readonly using the setReadonly method and in the CMS the user will not be able to edit or delete record data. The user will only be able to view these records.
Also, this state will be used if the user can only view the page, but not edit it.

Test steps

Add the following code to your test DataObject or Page:

  private static array $has_one = [
       'HasOneLink' => Link::class,
   ];

   private static $has_many = [
       'HasManyLinksOne' => Link::class . '.Owner',
       'HasManyLinksTwo' => Link::class . '.Owner',
   ];

   private static array $owns = [
       'HasOneLink',
       'HasManyLinksOne',
       'HasManyLinksTwo',
   ];

   public function getCMSFields()
   {
       $fields = parent::getCMSFields();
       $fields->removeByName(['HasOneLinkID', 'HasManyLinksOne', 'HasManyLinksTwo']);

       $fields->addFieldsToTab(
           'Root.Main',
           [
               LinkField::create('HasOneLink')
               ->setReadonly(true),
               MultiLinkField::create('HasManyLinksOne'),
               MultiLinkField::create('HasManyLinksTwo')
               ->setReadonly(true),
           ],
       );

       return $fields;
   }

Note

There is some problem here, how this should work if the user can edit the page, but LinkField is set to read only. Which should take precedence over canEdit or ReadOnly? Since if the field is read only, then a user with canEdit permission will not be able to create a link. Should there be a super user who can create links? Based on this discussion: #141 (comment)

Parent issue

Read only #11

sabina-talipova · 2024-01-16T20:44:11Z

src/Controllers/LinkFieldController.php

+        $request = $this->getRequest();
+        $ownerClass = $request->getVar('ownerClass') ?: $request->postVar('OwnerClass');
+        $ownerRelation = $this->ownerRelationFromRequest();
+        $isReadOnly = Injector::inst()->get($ownerClass)->getCMSFields()->dataFieldByName($ownerRelation)?->isReadonly();


This method gets Owner's (e.g. Page) fields and check is this LinkField is read only.

sabina-talipova · 2024-01-16T21:29:43Z

src/Controllers/LinkFieldController.php

+        $ownerRelation = $this->ownerRelationFromRequest();
+        $isReadOnly = Injector::inst()->get($ownerClass)->getCMSFields()->dataFieldByName($ownerRelation)?->isReadonly();
+
+        return $isReadOnly ?? false;


isReadOnly can be null in some cases.

GuySartorelli · 2024-01-16T22:17:13Z

I'm not doing an actual review of this yet - just answering the question in the note:

There is some problem here, how this should work if the user can edit the page, but LinkField is set to read only. Which should take precedence over canEdit or ReadOnly? Since if the field is read only, then a user with canEdit permission will not be able to create a link. Should there be a super user who can create links? Based on this discussion: #141 (comment)

Readonly is more important than canEdit. It doesn't matter if we can edit anything if the field has been set to readonly - readonly explicitly means we're not allowed to edit the field, even if we have edit permissions for the model or the model's owner.

emteknetnz

I haven't tested locally yet

I really dislike the how 'readonly' has been stuffed into the value of 'canCreate'. It makes things much harder to understand what's going on.

Instead you should make 'readonly' its own separate key / prop

emteknetnz · 2024-01-17T01:55:47Z

client/src/components/LinkPicker/LinkPicker.scss

@@ -105,6 +105,10 @@
  &--published::before {
    display: none;
  }
+
+  &.readonly {


Suggested change

&.readonly {

&--readonly {

Safer to use a BEM class rather than a generic/global readonly that may accidentally get values set from somewhere else

You'll need to update this is bunch of places in this PR

emteknetnz · 2024-01-17T01:57:09Z

client/src/components/LinkPicker/LinkPickerTitle.js

      </div>
    </Button>
-    {canDelete &&
+    {(canDelete && canCreate) &&


This logic looks wrong, shouldn't have a canCreate check for a delete button

Updated. I have to use readonly, since we should hide "Archive" button if user cannot edit link field.

emteknetnz · 2024-01-17T02:01:08Z

src/Controllers/LinkFieldController.php

@@ -449,6 +467,11 @@ private function ownerRelationFromRequest(): string
        if (!$ownerRelation) {
            $this->jsonError(404, _t(__CLASS__ . '.INVALID_OWNER_RELATION', 'Invalid ownerRelation'));
        }
+
+        if (strpos($ownerRelation, '_Readonly') !== false) {


Why are we doing this with $ownerRelation? Owner relation should be what's set in the private static $has_many, not the _Readonly added to form fields when they are set to read only mode?

emteknetnz · 2024-01-17T02:07:03Z

src/Controllers/LinkFieldController.php

+    private function isReadOnlyField(): bool
+    {
+        $request = $this->getRequest();
+        $ownerClass = $request->getVar('ownerClass') ?: $request->postVar('OwnerClass');


Create a new private method getOwnerClassFromRequest() so that it's consistent with the rest of the file and throw a 404 error if the owner class !is_a DataObject

emteknetnz · 2024-01-17T02:10:14Z

src/Controllers/LinkFieldController.php

+        $request = $this->getRequest();
+        $ownerClass = $request->getVar('ownerClass') ?: $request->postVar('OwnerClass');
+        $ownerRelation = $this->ownerRelationFromRequest();
+        $isReadOnly = Injector::inst()->get($ownerClass)->getCMSFields()->dataFieldByName($ownerRelation)?->isReadonly();


Suggested change

$isReadOnly = Injector::inst()->get($ownerClass)->getCMSFields()->dataFieldByName($ownerRelation)?->isReadonly();

return (bool) Injector::inst()->get($ownerClass)->getCMSFields()->dataFieldByName($ownerRelation)?->isReadonly();

This doesn't take into account scenarios where the form was set to readonly, which I believe is what happens when the user doesn't have edit permission for the owner record (e.g. a page).

emteknetnz · 2024-01-17T02:10:25Z

src/Controllers/LinkFieldController.php

+
+        return $isReadOnly ?? false;


Suggested change

return $isReadOnly ?? false;

emteknetnz

I'm still able to sort links in a MultiLinkField when it's in a readonly state

Also needs a rebase

emteknetnz · 2024-01-19T03:26:45Z

src/Controllers/LinkFieldController.php

            $form->makeReadonly();
+            $form->setMessage(_t(__CLASS__ . '.READ_ONLY_MESSAGE', 'You do not have enough permissions to edit this form'), 'info');


I'm not sure we should be setting a form message here. Can you find any other examples of us doing this?

If we do keep it, the message is wrong when the form has been manually set to read only rather than because of a lack of permissions

@emteknetnz, I could not find any similar example. I think it was new requirements from Design team. I moved logic only for readonly field.

emteknetnz · 2024-01-19T03:27:35Z

client/src/components/LinkPicker/tests/LinkPickerTitle-test.js

@@ -89,3 +91,17 @@ test('LinkPickerTitle main button should not fire the onClick callback while loa
  fireEvent.click(container.querySelector('button.link-picker__button'));
  expect(mockOnClick).toHaveBeenCalledTimes(0);
 });
+
+test('LinkPickerTitle render() should have readonly class if cannot edit', () => {


Suggested change

test('LinkPickerTitle render() should have readonly class if cannot edit', () => {

test('LinkPickerTitle render() should have readonly class if set to readonly', () => {

emteknetnz · 2024-01-19T03:27:50Z

client/src/components/LinkPicker/tests/LinkPickerTitle-test.js

+  expect(container.querySelectorAll('.link-picker__link--readonly')).toHaveLength(1);
+});
+
+test('LinkPickerTitle render() should not have readonly class if can edit', () => {


Suggested change

test('LinkPickerTitle render() should not have readonly class if can edit', () => {

test('LinkPickerTitle render() should not have readonly class if set to readonly', () => {

emteknetnz · 2024-01-23T21:22:08Z

src/Controllers/LinkFieldController.php

            $form->makeReadonly();
        }

+        if ($this->isReadOnlyField()) {


This message feels even more out of place now

It will now only show if $this->isReadOnlyField() is explicitly called, though not if $this->canEdit() is false or if the owner page/element canEdit() returns false which seems worse because it's inconsistent.

We also don't show this message anywhere else in the CMS as far as we know

I think we should just remove this message, it's seems pretty obvious that the form is readonly because the background of all the form elements if grey

Perhaps raise a new card to add this to globally add this message to forms through the CMS instead?

emteknetnz

Merge conflicts, needs a rebase + run yarn build

sabina-talipova force-pushed the pulls/4/readonly-link branch 3 times, most recently from 237f785 to f34fcee Compare January 16, 2024 19:33

sabina-talipova marked this pull request as ready for review January 16, 2024 19:35

sabina-talipova mentioned this pull request Jan 16, 2024

Read only #11

Closed

3 tasks

sabina-talipova force-pushed the pulls/4/readonly-link branch 2 times, most recently from c98121f to 1cf61bb Compare January 16, 2024 20:41

sabina-talipova commented Jan 16, 2024

View reviewed changes

sabina-talipova force-pushed the pulls/4/readonly-link branch from 562eccf to 70d245d Compare January 17, 2024 01:06

emteknetnz requested changes Jan 17, 2024

View reviewed changes

sabina-talipova force-pushed the pulls/4/readonly-link branch 4 times, most recently from 7ea3d91 to 512f05b Compare January 17, 2024 23:39

emteknetnz requested changes Jan 19, 2024

View reviewed changes

sabina-talipova force-pushed the pulls/4/readonly-link branch 5 times, most recently from 1994149 to 2192224 Compare January 23, 2024 03:48

emteknetnz requested changes Jan 23, 2024

View reviewed changes

sabina-talipova force-pushed the pulls/4/readonly-link branch from 2192224 to 59d2873 Compare January 23, 2024 21:39

emteknetnz requested changes Jan 23, 2024

View reviewed changes

Sabina Talipova added 3 commits January 24, 2024 11:35

ENH Add Readonly field status

483debe

ENH Add Readonly field status

6a80f83

ENH Add info message for readonly fields

0f62e1d

sabina-talipova force-pushed the pulls/4/readonly-link branch from 59d2873 to 0f62e1d Compare January 23, 2024 22:39

emteknetnz approved these changes Jan 24, 2024

View reviewed changes

emteknetnz merged commit 9bb3a1a into silverstripe:4 Jan 24, 2024
10 checks passed

emteknetnz deleted the pulls/4/readonly-link branch January 24, 2024 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Add Readonly field status #172

ENH Add Readonly field status #172

sabina-talipova commented Jan 16, 2024 •

edited

sabina-talipova Jan 16, 2024 •

edited

sabina-talipova Jan 16, 2024

GuySartorelli commented Jan 16, 2024

emteknetnz left a comment

emteknetnz Jan 17, 2024

sabina-talipova Jan 17, 2024

emteknetnz Jan 17, 2024

sabina-talipova Jan 17, 2024

emteknetnz Jan 17, 2024

sabina-talipova Jan 17, 2024

emteknetnz Jan 17, 2024

sabina-talipova Jan 17, 2024

emteknetnz Jan 17, 2024

sabina-talipova Jan 17, 2024

GuySartorelli Jan 18, 2024

emteknetnz Jan 17, 2024

sabina-talipova Jan 17, 2024

emteknetnz left a comment

emteknetnz Jan 19, 2024

sabina-talipova Jan 22, 2024

emteknetnz Jan 19, 2024

sabina-talipova Jan 22, 2024

emteknetnz Jan 19, 2024

sabina-talipova Jan 22, 2024

emteknetnz Jan 23, 2024

sabina-talipova Jan 23, 2024

emteknetnz left a comment •

edited

	$isReadOnly = Injector::inst()->get($ownerClass)->getCMSFields()->dataFieldByName($ownerRelation)?->isReadonly();
	return (bool) Injector::inst()->get($ownerClass)->getCMSFields()->dataFieldByName($ownerRelation)?->isReadonly();

		$form->makeReadonly();
		$form->setMessage(_t(__CLASS__ . '.READ_ONLY_MESSAGE', 'You do not have enough permissions to edit this form'), 'info');

	test('LinkPickerTitle render() should have readonly class if cannot edit', () => {
	test('LinkPickerTitle render() should have readonly class if set to readonly', () => {

	test('LinkPickerTitle render() should not have readonly class if can edit', () => {
	test('LinkPickerTitle render() should not have readonly class if set to readonly', () => {

ENH Add Readonly field status #172

ENH Add Readonly field status #172

Conversation

sabina-talipova commented Jan 16, 2024 • edited

Description

Test steps

Note

Parent issue

sabina-talipova Jan 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Jan 16, 2024

emteknetnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment • edited

Choose a reason for hiding this comment

sabina-talipova commented Jan 16, 2024 •

edited

sabina-talipova Jan 16, 2024 •

edited

emteknetnz left a comment •

edited