[GRIDFIELD] GridField needs a useful readonly view (currently just showing "none")

sunnysideup commented 10 years ago

Acceptance Criteria

I can click through GridField rows in readonly view (without action buttons)
I can perform standard GridField view operations (pagination, search, sorting)
I can click into a row of a readonly GridField to see it's edit view (which might also be readonly)
Edit views on rows in a readonly GridField are also marked readonly (regardless of DataObject->canEdit()) - this is important for history viewer
No state-altering actions are shown (create buttons, batch actions)

Notes

Related: Diff view for related records
If all records in the GridField aren't editable, it still shows an editable GridField
Components should be able to self determine if and how they're shown in readonly views
Track under ORB-175

PR

[x] https://github.com/silverstripe/silverstripe-framework/pull/8535

Historic Notes

I would like to lodge the following bug just in case it is one ...

Hi,

I have a very strange situation. I dont know if it is stupidity (80% sure it is!!!) or something else - I am just posting this here, because it may be of interest to some of you and because I am not sure if it is a bug or something else.

This code works fine on one data object and not on another:

function getCMSFields(){
        ....
        $config = GridFieldConfig_RecordViewer::create();
        $dataList = Member::get();
        $gridField = new GridField('AA','AA'.$dataList->count(), $dataList, $config);
        $fields->addFieldToTab("Root.BB", $gridField);
        ....
}

On DataObject 1 it returns a normal gridfield - its title is AA7 (i.e. there are seven members), but on dataobject 2 it returns as field that says:

AA7 (none)

This is unexpected.

What I know:

it only is "funny" on one dataobject

it does not matter if I use Member or Page or MyOtherDataObject as datalist source, it always return as "none"

the other cms fields are not causing the error (I removed all the other fields and I ended up with the same results).

also with an actual relation (this is what I am working with e.g. Players on Team), then it still returns as "none", despite their being record available.

I am using framework 3.1.5 and I have not experienced this problem before.

It is an extensive application with plenty of code so some magic word or unexpected far-flung bug may cause the error.

in the getCMSFields, I dont use parent::getCMSFields, but even if I do, I get the same error. I did a search for the word "none" in /framework/forms/ and framework/templates/ without any meaningful results (only match was for the LookupField, but I cant imagine how a gridfield would become a lookup field) . I also searched for (none) throughout the ENTIRE application and got the same result: the lookup field.

the html looks as follows:

67 members!!!

sunnysideup commented 10 years ago

I also searched for "lookup" in /framework/forms/ to see if a gridfield would ever be replaed by a lookup field and this also does not seems to be the case.

Any help appreciated.

sunnysideup commented 10 years ago

Here is an updated definition of the bug:

I am a little confused by the way canEdit impacts on the CMS.

Here is an example:

I have an object SchoolClass which has_one teacher and has_many students.

In the CMS, this is represented in model admin as a list of school classes. You can edit each class individually (using the model admin detail form), and there are grid fields for the teacher (only ever showing one record) and for students (showing many of them). You can click on each teacher / student to edit these records. So far no surprises.

What really surprised me is when I change the "canEdit" return value for SchoolClass to false. At this moment, the form field for the teacher and the students no longer show a gridfield. Instead it just says (none).

e.g.

Teacher ........................ (none) Students ....................... (none)

even though the class has a teacher and many students.

The return values for the CanView and CanEdit methods on Teacher / Students both return true no matter what.

I am wondering what the logic is behind this set up ( here is hoping I made a mistake ).

Just to be clear - what I dont understand is

a. why can't I edit the teacher / students even though I have the permissions to do so?

b. why do teacher and students show as "(none)" when there are clearly records? This is very confusing to the user.

I am using SS 3.1 branches (maybe that is the problem), but it appears the same problem occurred on 3.1.5.

sunnysideup commented 10 years ago

I have created an example here:

https://drive.google.com/file/d/0B9HUrSaBxJY8LVBtRFlXbFM3TE0/edit?usp=sharing. I reckon this is a major bug because you can be left with the false impression that there are no child records for an object, where there maybe many... If you rely on the CMS information to make decision or reply to a customer etc... then this could have serious consequences.

Also see here: https://groups.google.com/forum/#!topic/silverstripe-dev/m8PmPGpt6D4

sunnysideup commented 10 years ago

@tractorcow

Hi Damian

Since you were able to help me so well in the past, I was wondering if you could give me some feedback with this issue. If you dont have time then could you please nomiate nominate someone else to help?

I have had a look at the code now. Basically, the performReadonlyTransformation method does not exist in GridField and so the FormField method is used. I have pasted the FormField method below FYI:


    /**
     * Returns a readonly version of this field
     */
    public function performReadonlyTransformation() {
        $copy = $this->castedCopy('ReadonlyField');
        $copy->setReadonly(true);
        return $copy;
    }

Now, I can see two options moving forward (I think the current set up is really bad and confusing and should be fixed ASAP):

(1) on a "performReadonly" for a gridfield to do nothing because the readonly status is controlled by return value of canEdit for the shown dataobjects.

(2) return a gridfield with the "edit this record" button removed (so effectively we have a read-only form - even though the user may be allowed to edit the items anyway. In this case, the Detailed Form should also become read-only.

Option (1) is super easy to code. Option (2) is trickier and could lead to further issues. One of the issues with (2) is that, when we go through to the "detailedform" we are effectively in a new form so the "performreadonlytransformation" may no longer apply or it maybe hard to know if it should be applied.

Does that make sense?

Much appreciate your wisdom in moving forward.

Thank you

Nicolaas

tractorcow commented 10 years ago

Maybe in the performReadonlyTransformation it could disable any gridfield config components that alter the underlying grid? Maybe we need to do a similar transformation on the underlying gridfield config as well. ($this->setConfig($this->getConfig()->performReadonlyTransformation());

sunnysideup commented 10 years ago

Hi Damian

Thank you for your reply.

What do you think about my underlying question:

If "DataObject A" is read-only in a form then should related objects (i.e. the has_one / many_many / has_many objects) also be set to read-only when edited in the context of Dataobject A?

(e.g. if a "school class" edit form is set to read only does that mean that its pupils should also be read-only (even though they maybe editable in general).

I think that if we answer that question first then it will be easier to decide a way forward.

There are three answers: a. yes - within this context the "child" objects should also be read-only b. no - objects are independent and you should be able to edit them even if their "parent" object is read-only c. it depends in the type of objects, etc...

I am curious to know what you think, because the answer to this question is required before working out a fix afaik.

Thank you

Nicolaas

tractorcow commented 10 years ago

Hm, that depends.

If Object A is read only, then I'd probably not expect to add or remove items from relations, although those individual related items will have their own canEdit, so maybe they could be editable.

pschiffe commented 10 years ago

Hi guys, was there any progress with this issue? I really like @tractorcow idea how to fix this. peter

sunnysideup commented 10 years ago

I would love to create a fix for this, @pschiffe , do you have any suggestions?

pschiffe commented 10 years ago

Well, in case user can't edit object A, I wouldn't do the readonly transformation on GridField, instead, still show the GridField but without possibilities to link, unlink, create and delete objects B. Editing of object B can stay as it is, based on the permissions on object B. I think this could be the sane defaults but I'm really no expert in SilverStripe..

pschiffe commented 10 years ago

I found another related issue. If member don't have permission to create object A, but he has permissions to edit object A and create and edit object B, the add button is not available for GridField with objects B, which is another apparent bug. @sunnysideup do you have any workaround for these issues?

sunnysideup commented 9 years ago

@pschiffe , I dont really have a work-around for these issues.

You could try this (untested):


/**
 * GridField with a proper ReadOnly version ...
 */
class GridFieldForReadonly extends GridField {

    /**
     * Returns a readonly version of this field
     * @return GridField
     */
    public function performReadonlyTransformation() {
        $this->getConfig()
            ->removeComponentsByType("GridFieldDeleteAction")
            ->removeComponentsByType("GridFieldAddExistingAutocompleter")
            ->removeComponentsByType("GridFieldAddNewButton");
        return $this;
    }

}

pschiffe commented 9 years ago

Thanks @sunnysideup, I based my workaround on your example. Also note, my previous comment about missing add button was luckily issue in my code.. :-)

irkeepin commented 9 years ago

I'm facing many of these same issues.

It's been a few months since this thread was active - anyone make progress in the interim?

tractorcow commented 9 years ago

This is the solution I used for the blog module.

https://github.com/silverstripe/silverstripe-blog/blob/cba4bd8b59352a206caa578f2d4bd0adda76f6a0/code/extensions/BlogFilter.php#L110

The underlying assumption for this change is, just because the parent object is not editable, that doesn't mean the child objects aren't. In the above case, I wanted blog authors to be able to create new posts via a gridfield, without the ability to edit the actual parent.

In the case that a child object is not editable, gridfield will internally call performReadonlyTransformation on the edit form for that object.

irkeepin commented 9 years ago

That solution worked perfectly for me, thanks. I simply had a read-only data object that included a has-many relationship. I wanted to display the has-many list as a GridField, but couldn't do that in the "view" mode of the CMS. But extending GridField as you suggest solved it.

ntd commented 9 years ago

It took me a while to discover, but once you have the class with read-only transformation enabled (such as the @sunnysideup GridFieldForReadonly above) you can enable it by leveraging the injector, e.g. by putting in your _config.yml:

Injector:
    GridField:
        class: GridFieldForReadonly

After a flush, everything (ModelAdmin included) will pick up the new class instead of the old one.

pschiffe commented 9 years ago

@ntd thanks for the tip, works like a charm.

BTW, any updates on this issue? No general solution yet?

tractorcow commented 9 years ago

Not yet sorry, but this is still a real limitation of the gridfield API. Maybe worth pointing out to @assertchris who has been looking at ways to improve gridfield. :)

dhensby commented 8 years ago

bump

pitchandtone commented 7 years ago

bump

sunnysideup commented 7 years ago

I am not in a good position to work on this right now, hope it gets fixed....

chrispenny commented 6 years ago

Thank you for the workaround @sunnysideup. I was able to implement your solution in my SS4 project just as easily.

robertclarkson commented 6 years ago

Good workaround @sunnysideup thanks for that

tractorcow commented 6 years ago

Will see if we can implement this as a part of http://github.com/silverstripe/silverstripe-versioned/issues/93

clarkepaul commented 6 years ago

Hi, we will create some visuals for this issue with the intention to get everyone on the same page with an approach.

Question first, is there a case when a GridField might contain both readonly and editable items?

At the moment I'm thinking to do a normal looking GridField with a preview type icon instead of an edit icon if items have a readonly state, and when the item is being viewed the fields are readonly without any actions available (possibly with the addition of a "read-only" notification).

chrispenny commented 6 years ago

Hi @clarkepaul,

I think that allowing for a mixture of readonly and editable Objects within a single GridField would be absolutely fantastic.

I could see it being very useful if, say, you're using these modules:

Elemental
Advanced Workflow

And you were allowing Workflows at a Block level, and/or were allowing authors to set Embargo dates at a Block level.

Any time I'm using these modules, I always push for making the Objects readonly while they're in either of these states - otherwise things tend to get very messy (from the Author's pov).

pitchandtone commented 6 years ago

Agree. Needs to be able to have both. Like sitetree some objects could be editable by me, but not all, however I still need to see them.

Sent from my iPhone

On 12/03/2018, at 2:50 PM, Chris Penny notifications@github.com wrote:

Hi @clarkepaul,

I think that allowing for a mixture of readonly and editable Objects within a single GridField would be absolutely fantastic.

I could see it being very useful if, say, you're using these modules:

Elemental Advanced Workflow And you were allowing Workflows at a Block level, and/or were allowing authors to set Embargo dates at a Block level.

Any time I'm using these modules, I always push for making the Objects readonly while they're in either of these states - otherwise things tend to get very messy (from the Authors pov).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

newleeland commented 6 years ago

Potential Designs gridfield_read only_holder

gridfield_read only_listing

clarkepaul commented 6 years ago

@newleeland thanks for the mockups. We've taken a very lightweight approach to showing readonly items, we did consider icons/badges on each readonly item but decided that showing indicators on each item in a list becomes visually crowded. History or the the Archive will be readonly for the whole section. If you don't have permissions to edit items in a GridField your likely not able to edit any of the items but in most cases people should be able to access the data.

The one thing I think the design might be lacking is some information (like an notification) as to why the item is in a readonly state, but its perhaps only required for some cases like:

the GridField is showing a mix of readonly and editable items
the user doesn't have permissions (meaning others would be able to edit them)
the item is in a workflow eg. advanced workflow
this information is not stored on this database (but through an API)
and probably a few more...

clarkepaul commented 6 years ago

I'm suggesting that we go with the proposed design like @newleeland has presented, and when in development we can dig a bit deeper as to all of the reasons why items might be readonly and whether giving anymore detail is beneficial in different scenarios (like those specified in my previous comment).

mlewis-everley commented 6 years ago

One thing I don't understand, doesn't GridField already support edit/view permissions? If I send a list of objects to GridField, it removes any that cannot be viewed.

If any items in the list cannot be edited, clicking them opens them as read only.

Shouldn't a GridField loaded onto a ReadOnly form, still behave in the same fashion?

For example (we have something similar on a current project), a Page might have many notes, a user can login, cannot edit the page, but can create notes and edit only notes they have created?

Possibly this is what you were meaning @clarkepaul?

PS: This issue is driving me nuts at the moment, so :+1: for doing something about it.

clarkepaul commented 6 years ago

I was under the impression it still comes up with "none" by default in place of showing a grid field which is misleading for users, specially if there is nothing private about the information. So if the functionality actually exists to show the data in a readonly state then I think we should be doing that by default. Hopefully I haven't mis-understood the issue. cc @mlewis-everley

sunnysideup commented 6 years ago

from memory, the gridfield looks at canEdit of the "parent" .... e.g. if you are editing Products where products have many Likes then if Products::canEdit returns false then the Likes in the CMS will be shown as (none). That is wrong of course, but that is how it was last time I checked, a long time ago now.

mlewis-everley commented 6 years ago

@clarkepaul Sorry, you are right, it still displays "none" when readonly. What I was meaning was shouldn't the gridfield behave the same way irrespective of if the parent object is readonly or not (I think this is what @sunnysideup is meaning)?

clarkepaul commented 6 years ago

I'm not sure what should be the default, I would think there's an option for gridfields to have to inherit based on whether the parent is readonly, or be just editable unless specified. I think its best that devs make the recommendation on what approach we take based on actual needs.

Wouldn't an inherit model work well here?

clarkepaul commented 6 years ago

Readonly gridfields should also consider the item previews... Related https://github.com/silverstripe/silverstripe-versioned/issues/100

caffeineinc commented 6 years ago

CWP 2.0 Also has a problem viewing Carousel content due to GridField not having a decent readonly state. We're still lacking good solution for this, and right now history is broken for any page with a GridField Present.

purplespider commented 6 years ago

@caffeineinc Is right, the History tab currently fails to load for any page which includes aGridField. Just spent 2 hours trying to work out what was causing it. This needs to be fixed with a workaround ASAP, before implementing a proper read-only state.

caffeineinc commented 6 years ago

So i've got a potential fix, provide GridField with a whitelist of readonly (overloadable) transformation components. That way GridField can handle it's own transformation, and if you've got custom implementations, you can add those.

I'll just create a PR.

GridField doesn't have a valid readonly state if it's value is set to an Object without forTemplate()`. The default behaviour is to render a ReadonlyField, but given GridField is a complex type this isn't suitable.

This bugfix provides a transformation method to render only components that are whitelisted to provide a readonly state.

Hopefully that doesn't affect semver.

NightJar commented 6 years ago

GridField has a valid readonly state in that fields without a performReadonlyTransformation method are converted into empty ReadonlyField objects (which then render as an empty string).

I have never run into this issue (which in itself is very interesting), even when adapting Elemental to have a read only transformation state - before this change pages would simply render read only titles, url segment, and metadata... with a complete void of content (thus the necessity for the change as most versions appeared the same despite radically different content over time).

It appears that something is calling setValue too late in the process of a history view, setting the value of a ReadonlyField to what should be the list for the GridField. To me this should also be looked at, as it seems like a logical error that is usually hidden (i.e. treat the cause, not just the symptom).

Regardless of the bugs above, a GridField should still have a valid read only state (per OP & PR) :>

caffeineinc commented 6 years ago

So do you want me to add a Noop setValue() function to the PR, as something in the history controller is doing that, or find what's setting the value somewhere deep in history controller.

Every formfield should have a value, but GridField is kind of a composite field, and state is stored in GridFieldState (in GridField->components) rather than value, which alone moves away from the FormField interface.

I think we should try and get an actionable plan, on how to move forward. This has been broken for too long, and right now, every 4.2 release has to be forked to resolve the issue, or risk going out broken.

NightJar commented 6 years ago

I dunno bruddah, I'm not core team. Personally I'm fine with a noop, but @silverstripe/core-team may have some feelings about it :)

robbieaverill commented 6 years ago

ping @silverstripe/core-team

caffeineinc commented 6 years ago

It'd be super helpful to get some sort of direction here.

chillu commented 6 years ago

So do you want me to add a Noop setValue() function to the PR, as something in the history controller is doing that, or find what's setting the value somewhere deep in history controller.

Ideally you can track down the root cause. In general, a FormField->setValue() method accepts different types, based on the view representation. GridField doesn't have it's own specific setValue() implementation (or a noop implementation), but rather uses the constructor or setList() to set it's "value". So it'll fall back to FormField->setValue(). As far as I can tell, neither GridField nor GridState are using FormField->value anywhere.

So there's two issues here:

GridField causes readonly views to fatal (impact/critical)
GridField doesn't have a good readonly view (impact/high?)

@caffeineinc I'm assuming your PR in https://github.com/silverstripe/silverstripe-framework/pull/8348 fixes both?

chillu commented 6 years ago

@lukereative Could you assess if Simon's PR in https://github.com/silverstripe/silverstripe-framework/pull/8348 fixes what's been discussed here, and close this issue?

clarkepaul commented 5 years ago

What was the outcome here? e.g. what does the user see?

NightJar commented 5 years ago

The commit (I've not seen this feature as UI) reads that it removes all components from the config that allow editing. This should mean it displays a normal gridfield, but with no ability to edit any entry, although one should be able to view an entry's details (in a read only detail form).

silverstripe / silverstripe-framework