search

silverstripe / silverstripe-versioned

Versioning for Silverstripe model
BSD 3-Clause "New" or "Revised" License
8 stars 36 forks source link

GridFieldArchiveAction archives the end object in a versioned many_many relation #200

Open UndefinedOffset opened 5 years ago

UndefinedOffset commented 5 years ago

Affected Version

silverstripe/recipe-cms 4.3.0 silverstripe/versioned 1.3.0

Description

In SilverStripe 4.3.0 (at least) when you have a many_many through relationship that is versioned (on both the junction and the end objects) the "Archive" button provided through GridFieldArchiveAction does not archive the junction object it archives the end object. As well the junction object's row remains in the database.

As a side (not sure if it's my install or not it's possible found on a very big site) the alert to confirm the action is empty in Firefox 64 at least.

Expected

The junction object would be removed but the end object of the relationship would not be.

Steps to Reproduce

  1. Have a many_many through relationship that has the junction and end objects versioned
  2. Add one of the end objects to the relationship
  3. Click the archive button
  4. Check the end object and it is now archived

Pull requests

UndefinedOffset commented 5 years ago

Something else perhaps is part of the answer here is that the archive button perhaps should not be present at all (maybe it stays as unlink) when the list is a many_many list even when it's a through relationship. The check in GridFieldArchiveAction will add the archive button to any type of many_many relationship so long as the end object has the Versioned extension and clicking it will archive the end object.

maxime-rainville commented 5 years ago

I've raised a separate issue for the missing confirmation message. https://github.com/silverstripe/silverstripe-versioned-admin/issues/94

As a side note, it might be worthwhile looking at this issue as the same time as they are both dealing with ManyMany through relations. https://github.com/silverstripe/silverstripe-framework/issues/8704

tractorcow commented 5 years ago

Note: I would bake this logic into the gridfield archive action itself, and not try to automate it too much at the ORM level. There is a distinct difference between "archive this object" and "archive this item in a list I've visually identified".

tractorcow commented 5 years ago

This is how I would expect it should work in gridfield:

AljosaB commented 5 years ago

I believe this should be marked with a high impact... editors on one of my projects are archiving important pages instead of unlinking them from not so important relation.

UndefinedOffset commented 5 years ago

@AljosaB we ended up going with the following as a work around until this issue is patched and released. It's kind of brute force and may not be the full solution but it basically tells all GridField's who's list is a ManyManyList or ManyManyThroughList to stay with the old unlink button but it does seem to solve the issue of editors archiving end pages. It's pretty easy to use just swap out SilverStripe\Versioned\VersionedGridFieldArchiveExtension with the below class via injector and you should be good to go. This would solve the issue it sounds like you are having, but it wouldn't solve the issue if the through object is versioned as well I don't think.

MyProject\Extensions\GridFieldArchiveExtension

<?php
namespace MyProject\Extensions;

use MyProject\Forms\GridField\GridFieldArchiveAction;
use SilverStripe\Forms\GridField\GridFieldDeleteAction;
use SilverStripe\Versioned\VersionedGridFieldArchiveExtension;

class GridFieldArchiveExtension extends VersionedGridFieldArchiveExtension {
    public function updateConfig() {
        /** @var GridFieldConfig $owner */
        $owner=$this->getOwner();

        $owner->addComponent(new GridFieldArchiveAction(), GridFieldDeleteAction::class);
    }
}

MyProject\Forms\GridField\GridFieldArchiveAction

<?php
namespace MyProject\Forms\GridField;

use SilverStripe\Forms\GridField\GridField;
use SilverStripe\ORM\ManyManyList;
use SilverStripe\ORM\ManyManyThroughList;
use SilverStripe\Versioned\GridFieldArchiveAction as SS_GridFieldArchiveAction;

class GridFieldArchiveAction extends SS_GridFieldArchiveAction {
    /**
     * Add a column 'Actions'
     * @param GridField $gridField
     * @param array $columns
     */
    public function augmentColumns($gridField, &$columns) {
        if($gridField->getList() instanceof ManyManyList || $gridField->getList() instanceof ManyManyThroughList) {
            return;
        }

        return parent::augmentColumns($gridField, $columns);
    }

    /**
     * Returns the GridField_FormAction if archive can be performed
     *
     * @param GridField $gridField
     * @param DataObject $record
     * @return GridField_FormAction|null
     */
    public function getArchiveAction($gridField, $record) {
        if($gridField->getList() instanceof ManyManyList || $gridField->getList() instanceof ManyManyThroughList) {
            return null;
        }

        return parent::getArchiveAction($gridField, $record);
    }
}

app/config.yml

SilverStripe\Core\Injector\Injector:
    SilverStripe\Versioned\VersionedGridFieldArchiveExtension:
        class: 'MyProject\Extensions\GridFieldArchiveExtension'

For our project that spawned my reporting of the issue we ended up dropping many_many through relationships and going back to using a module I upgraded to work with SilverStripe 4 webbuilders-group/silverstripe-versioned-helpers it was a way I came up with in the SilverStripe 3 days to make many_many relationships at least have a draft and live state. We use it on one of our biggest client sites and at least in the SilverStripe 3 version it appears to work pretty well for them for the last several years. The SilverStripe 4 version hasn't been tested in production just yet but it appears to be also working well in our internal QC.

robbieaverill commented 5 years ago

I've just come across this as well (https://github.com/silverstripe/cwp/issues/179). This cannot be considered as expected behaviour IMO!

robbieaverill commented 5 years ago

Note that this also affects regular many_many relationships where the target class is versioned (e.g. a page).

Example code:

class Page extends SiteTree
{
    private static $many_many = [
        'RelatedPages' => [
            'through' => RelatedPage::class,
            'from' => 'PageA',
            'to' => 'PageB',
        ],
        'RelatedPagesWithoutThrough' => Page::class,
    ];

    public function getCMSFields()
    {
        $this->beforeUpdateCMSFields(function (FieldList $fields) {
            $config1 = GridFieldConfig_RelationEditor::create();
            $config2 = GridFieldConfig_RelationEditor::create();

            $fields->addFieldToTab(
                'Root.RelatedPages',
                GridField::create('RelatedPages', null, $this->RelatedPages(), $config1)
            );
            $fields->addFieldToTab(
                'Root.RelatedPagesWithoutThrough',
                GridField::create('RelatedPagesWithoutThrough', null, $this->RelatedPagesWithoutThrough(), $config2)
            );
        });
        return parent::getCMSFields();
    }
}

use SilverStripe\ORM\DataObject;

class RelatedPage extends DataObject
{
    private static $has_one = [
        'PageA' => Page::class,
        'PageB' => Page::class,
    ];
}

Both are affected.

My thoughts on how this should behave in this example, with emojis indicating the current state of behaviour:

Further behaviour which isn't included in my example above, but noting the way I'd expect to behave anyway:

robbieaverill commented 5 years ago

I think the bug in both many many and many many through examples is that the GridFieldArchiveAction::handleAction() logic is calling $item->doArchive() on the linked object (Page) rather than the link object (RelatedPage or a DB record in many many associated table).

@tractorcow: Note: I would bake this logic into the gridfield archive action itself, and not try to automate it too much at the ORM level. There is a distinct difference between "archive this object" and "archive this item in a list I've visually identified".

+1, but I don't agree with https://github.com/silverstripe/silverstripe-versioned/issues/200#issuecomment-453323765. Perhaps you could assume the item could be archived as well as the join item provided the join item defines $cascade_deletes, which you'd expect to be handled by the ORM rather than the GridField action handler, but I don't think it should work that way by default. It currently does.

thats4shaw commented 5 years ago

Also struck this with the implementation @robbieaverill has outlined with just standard many_many relationships.

ChrissiQ commented 5 years ago

I'm getting this problem with a $has_many relationship. I am not entirely sure how archiving a $has_many should work, since there is no actual link object, rather a column with the foreign ID. Somehow that column will need to be archived, rather than the object itself. I guess a new version of the object with the foreign ID removed should be created, or something like that?

ChrissiQ commented 5 years ago

For others who may be facing this problem, I couldn't find a simple solution for my relation GridFields. For those who just want unlink to work again, this seems to work for me:

        $fields->addFieldsToTab('Root.Main', [
            GridField::create(
                'MyRelation',
                'My Relation',
                $this->MyRelation(),
                $gridFieldConfig = GridFieldConfig_RelationEditor::create()
            ),
        ]);
        $gridFieldConfig->removeComponentsByType(GridField_ActionMenuItem::class);
        $gridFieldConfig->addComponent(new GridFieldDeleteAction(true));

This will remove the 'edit' and 'archive' options in the gridfield, and add an 'unlink' option which mostly works correctly. It doesn't publish the update though when using with $has_many, so you may have to manually publish the record.

phptek commented 5 years ago

@ChrissiQ Your solution works well for constrained scenarios, such as for broken RelatedPages ManyMany relations in CWP 2.2.x versions (I think it's fixed in 2.3.0, but that's no use to me).

Thanks!

[UPDATE] This works in "constrained" scenarios like I said, but in others, it removes all gridfield actions (including "Edit"). Going with @UndefinedOffset's solution, works well and is pretty much what I was trying to do until I saw that Ed had done it already.

vinstah commented 5 years ago

quick fix to change back to unlink is to use GridFieldConfig_RelationEditor::remove_extension('SilverStripe\Versioned\VersionedGridFieldArchiveExtension');

maxime-rainville commented 5 years ago

@chillu This has been reported independently 3-4 times. It's driving people up the wall and it's causing users to accidentally archived records when they mean to unlink them. Can we put this in our backlog and get it fix?

chillu commented 5 years ago

As discussed, please have a look at this on our next bug day (next Friday)

maxime-rainville commented 5 years ago

What's the best default?

I'm really not sure what the most sensible default is here.

"unlink" is the least destructive option here, so you could argue that it should always be the default no matter what.

The way this is set up we could actually display both action in the menu. Although that might introduce more confusion. image

Should ownership play a role here? If the parent object owns the child, should we default to archive/delete under the assumption that the child can't exist without the parent?

Unlinking in a versioned has-many relationship

Unlinking in the context of a versioned has-many relation is actually a bit weird. Because what you'll be doing is creating a draft version of the related object, which you will then have to publish for the unlinking to actually take affect.

Unlinking a versioned many-many-through relationship

If you're unlinking a many-many relationship, what you are really doing is deleting the pivot table entry.

So if you're using a many-many-through and your pivot is versioned, then by extension unlinking in this context means archiving the pivot. The two end of the relationship should remain unchanged, no matter what.

Does that make sense?

AljosaB commented 5 years ago

Default should definitely be unlink, not archive. I would also like to point out that assuming that child can't exist without parent is ok in context of pages and not a good idea in context of dataobjects. Also there should always be an option to unlink in addition (not a replacement) to archiving/deleteing.

normann commented 5 years ago

Could anyone from the Core team give it a fix, please? seriously. Otherwise, we need to go through all my project code to find any mary_many relation and the end objects that are versioned, then remove the Archive button since it is too Danger for a non-technical CMS user.

robbieaverill commented 5 years ago

Hi @normann, this issue is in our list of priorities. Until we get to it, anybody is welcome to have a crack at fixing it!

NightJar commented 4 years ago

I've made a PR where I've chosen the "best default" from @maxime-rainville 's comment above to be:

Show both the unlink and archive options

This only applies if an unlink option was already present. Otherwise the behaviour is as it was before (the archive action is added, and the delete option removed if it existed).

This works well, except that it does not unpublish a many many through join object before deleting it (removing the draft). This leaves a user with no way to unpublish the relationship on the live site.

I've left two options for evaluation on the pull request.

One I feel is a quick win, but more of a technical debt accruing style Two I feel is the more appropriate solution, but will take a bit longer to implement and be a larger change (likely need a minor release).

tractorcow commented 4 years ago

I've merged https://github.com/silverstripe/silverstripe-versioned/pull/246 because I feel it added an improvement to the existing state, but it doesn't completely address this issue. You still need to address the issue with the mapping object.

Leapfrognz commented 4 years ago

Hi all, Can we have an update on this one please?

NightJar commented 4 years ago

Hi @Leapfrognz

The PR referenced was merged, as described above.

However this has really only shifted the issue. While it is now possible to remove a link without deleting the related item... if there is no other method for managing the item, this leaves the content editor with a piece of content on the live site that they have no way of removing (either unpublish or archive).

This is untrue for e.g. CWP's Related Pages feature, as one can still manage pages in the same manner. At least they don't go missing 'randomly' (unexpectedly) anymore. But the wider problem for other content as described is still open.

To my knowledge nothing has been done since the PR merge above.

tristan-mastrodicasa commented 4 years ago

This hack may help some of you create an "acceptable" solution for the time being:

https://github.com/sunnysideup/silverstripe-cms-niceties/blob/master/src/Traits/CMSNicetiesTraitForManyManyGridField.php

chillu commented 4 years ago

I've brought this up outside of a GridField or Versioned context as well (for through relationships only), which is related but in my mind a separate, more focused bug: https://github.com/silverstripe/silverstripe-framework/issues/9612

tractorcow commented 4 years ago

+1, but I don't agree with #200 (comment). Perhaps you could assume the item could be archived as well as the join item provided the join item defines $cascade_deletes, which you'd expect to be handled by the ORM rather than the GridField action handler, but I don't think it should work that way by default. It currently does.

That's a good point.

If that's the case, we could always just archive the relation, (i.e. unlink) and leave the cascading to model configuration.

If this is acceptable do you mind if I go ahead and implement a solution. It's been a while and i looks like no one is going to attack this.

tractorcow commented 4 years ago

normal relation:

versioned relation

Sound acceptable?

tractorcow commented 4 years ago

alternatively, it may be simpler to simply use "unlink" for many_many in all cases.

UndefinedOffset commented 4 years ago

I think unlink for many_many should be the expected behavior in all cases, you probably don't want cascade_delete to also dump the right hand side of the many_many relationship not just the join object/row. Since isn't the point of many_many that an object can belong to many other objects? In which case you wouldn't want to also delete the right side of the many_many which would also remove it from all of the other objects that linked to it. (if that makes sense). In the case of using through objects that are also versioned I would think the action of unlinking should remove that through object from the current stage be it draft or live. Much like if you unlink a traditional many_many it removes that row from the junction table but not the right side object.

Leapfrognz commented 3 years ago

This causes big issues with the 'RelatedPages' functionality included in the CWP recipe. Because the ManyManyThrough object (RelatedPagesLink) is versioend, it never gets removed from the RelatedPagesLink_Live table, meaning authors cannot remove related pages on the live site. We just had an issue where the author couldnt remove incorrect 'related' information becuse of this