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.

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

UploadField still displays “draft” label after a file has been published #960

Closed
kinglozzer opened this issue Jun 21, 2019 · 25 comments
Closed

Comments

@kinglozzer
Copy link
Member

kinglozzer commented Jun 21, 2019

I’ve seen a few other people report this on Slack. For me, it‘s:

  • A non-versioned DataObject with has_one => File::class
  • private static $owns set up correctly
  • Managed via a standard ModelAdmin interface

Uploading a new file correctly displays as draft. Hitting “Apply Changes” on the DataObject publishes the file as expected, but the DRAFT label remains on the UploadField. Other observations:

  • Clicking the “eye” icon to view the file’s details shows it as published
  • Removing the file, then re-assigning it with “Add from files” results in the DRAFT label being correctly removed
  • Clicking the back arrow (CMS breadcrumbs), then editing the same object does not remove the DRAFT label
  • Clicking the next/prev “better buttons” and then going back to the original DataObject does remove the DRAFT label
  • Any other “hard” reload does remove the DRAFT label

Notes

  • Would be interesting to see if this is still an issues in 4.4.x
  • Sounds like the fix would involved forcing a refresh of the redux state for the react components (like uploadfield)
stevie-mayhew added a commit to stevie-mayhew/silverstripe-asset-admin that referenced this issue Aug 12, 2019
@brynwhyman brynwhyman added this to the Sprint 46 milestone Oct 15, 2019
@dnsl48 dnsl48 self-assigned this Oct 22, 2019
@brynwhyman brynwhyman removed this from the Sprint 46 milestone Dec 16, 2019
@brynwhyman
Copy link

I'm still seeing a similar issue in 4.5:

Steps:

  1. I have an UploadField owned by a page
  2. Upload a new image, see it recorded as 'Draft' in the UploadField
  3. Publish the page
  4. (Bug) See that the 'Draft' label stays in in the UploadField
  5. Click the 'eye' icon to access the Files area, and confirm the image IS recorded as published

@purplespider
Copy link
Contributor

Yep, seems to happen for any standard UploadField. e.g. on the new Silverstripe demo, just add a new blog post, upload a "Featured Image", then click Publish. It will still show the image as "Draft" but if you refresh the page the label dissapears.

@sunnysideup
Copy link

bump?

@purplespider
Copy link
Contributor

Any chance of the fix for this in 4.6? I've had quite few CMS editors get confused by this bug.

@brynwhyman
Copy link

This has been high on our product team's list to get a fix in for 4.6 but unfortunately it's not looking like we're going to be able to get to it before the change freeze cut-off, although we'll keep pushing.

Also noting that there's a similar issue that hasn't been linked before: #1001

@emteknetnz
Copy link
Member

emteknetnz commented Jun 17, 2020

I've been looking into this today, some notes

Adding the following within the $('.js-injector-boot input.entwine-uploadfield').entwine({ block in UploadFieldEntwine.js will force a the react component to refresh

    'from .cms-edit-form': {
      onaftersubmitform() {
        $(this).refresh();
      }
    },

However, this does not solve the original issue because entwine and redux provide different sets of file data

The entwine / refresh() code will provide props.data.files in UploadFieldEntwine.js via ReactDOM.render(<UploadField {...props}

Redux will provide a similar looking but ultimately separate props.files in UploadField.js via UploadField::mapStateToProps()

Some of the code within UploadField.js uses props.data.files, some other code uses props.files. 😃

The draft|modified text is derived from the redux props.files version, so forcing a refresh from entwine doesn't work

A couple of options:

a)
Update state in redux. The file updated draft|modified status would need to come from php, though I'm not sure how we'd go about updating redux without forcing a full refresh

b)
Update UploadField.js::render(), changing
{this.props.files.map(this.renderChild)} to
{this.props.data.files.map(this.renderChild)}

Though I'm not sure what side effects this would have. I tried changing it and straight off the bat it was buggy when I attempted to delete an existing upload.

@brynwhyman
Copy link

In case you haven't seen it, just noting that there is an old PR for this issue: #974.

There were concerns raised with how changing redux states might inadvertently affect (inline editable) elemental blocks with upload fields.

@emteknetnz
Copy link
Member

Cool thanks

I'm not sure that particular PR will fix this particular issue though. There seems to be a bit of a deeper issue here where we have two source of truth:

  • what entwine / php backend does
  • what redux does

I suspect something similar is going on elsewhere in asset-admin, for instance that sporadic bug where recently uploaded files show in the wrong folder

I'm fairly worried that any changes we make are going to cause regressions elsewhere. Upload fields basically require behat tests which tend to have the lowest amount of coverage.

@brynwhyman
Copy link

that sporadic bug where recently uploaded files show in the wrong folder

#1025

@emteknetnz emteknetnz removed their assignment Jun 21, 2020
@pjayme
Copy link

pjayme commented Oct 29, 2020

Hi team, this was recently flagged from one of our bespoke projects by the PO, just checking if there's any further developments on this? 🙂

@brynwhyman
Copy link

@pjayme no further developments that I'm aware of. There's been a couple of attempts to resolve this (and related issues) without success. I'll see if we can get back to this in the next month or so...

@JamesDPC
Copy link

Hi, this issue is still present in 4.7 and has been confusing for our content editors, resulting in an uptick in support tickets :(
Maybe someone with react skills could poke this?

In lieu of a fix being available, I applied this workaround to hide the orange DRAFT text entirely, which in our view was better than showing 'Draft' when the file was published.

---
Name: workaound-uploadfieldleftandmain
---
## workarounds.yml
# Ref: https://github.com/silverstripe/silverstripe-asset-admin/issues/960
SilverStripe\Admin\LeftAndMain:
  extra_requirements_css:
    - 'vendor/path/to/leftandmain.css'
// leftandmain.css
// Ref: https://github.com/silverstripe/silverstripe-asset-admin/issues/960
.uploadfield-item__meta .uploadfield-item__status {
  visibility : hidden;
}

We also applied an extension to the Upload Field, adding a right title text snippet with words to the effect of "Uploaded files remain in the draft state until they or this record is published"

@prij
Copy link

prij commented Sep 29, 2021

This issue is also present in 4.8 - I usually add a description to the upload field asking the content administrator to refresh the CMS page if the image is still appearing with the draft label in the CMS. Will this persist in future releases?

@sunnysideup
Copy link

@kinglozzer this is the biggest bug in the SS CMS right now. Is there a plan to fix it?

@kinglozzer
Copy link
Member Author

I’m on the core committers team, but I don’t work for SS Ltd (and unfortunately I’m not familiar enough with Redux to have a go at fixing this myself) so I’m afraid I don’t know. I agree this issue should have received more attention than it has so far.

@sunnysideup
Copy link

sunnysideup commented Oct 5, 2021

@kinglozzer - sorry, I just saw your name. Can you maybe raise it at the next meeting or ping the person in charge? Every site we deliver for our clients, we have to explain that this is a known bug, but we have explained this for over two years now and it really ought to get a bit of love.

@JamesDPC
Copy link

This is still a thing in 4.9, hiding the orange "Draft" label per prev. workaround is still in place but that's not a permanent fix.

There is an open pull request at #974 - how do we move this along?

@sunnysideup
Copy link

@here - it would be great to see this resolved.

@prij
Copy link

prij commented Jan 27, 2022

I haven't seen this mentioned in the 4.10.0 release notes from Jan 25 so I'm assuming it remains an issue in 4.10 @unclecheese? I was really hoping for a fix this release

@RVXD
Copy link

RVXD commented Mar 21, 2022

Niklas Forsdahl suggested on Slack to change getSchemaStateDefaults() method on UploadField.

"What I did was subclass UploadField, and override getSchemaStateDefaults to force react to always render the field by mismatching it's files state"

This can be tested like this:

injector.yml

---
Name: uploadfield-injector
---
SilverStripe\Core\Injector\Injector:
  SilverStripe\AssetAdmin\Forms\UploadField:
    class: SomeNameSpace\Injector\MyUploadField

Added subclass of UploadField:

<?php

namespace SomeNameSpace\Injector;

use SilverStripe\AssetAdmin\Forms\UploadField;

class MyUploadField extends UploadField
{

    public function getSchemaStateDefaults()
    {
        $state = parent::getSchemaStateDefaults();
        // force react component to always update when we render the field,
        // by making value.Files and data.files mismatch in the state
        $state['value']['Files'][] = -1;
        return $state;
    }

}

Tried it on a couple of sites and it works as a temporary fix.

@RVXD
Copy link

RVXD commented Mar 22, 2022

So can we simply change the getSchemaStateDefaults method in UploadField?
Current version:

    public function getSchemaStateDefaults()
    {
        $state = parent::getSchemaStateDefaults();
        $state['data']['files'] = $this->getEncodedItems();
        $state['value'] = $this->Value() ?: ['Files' => []];
        return $state;
    }

To this:

    public function getSchemaStateDefaults()
    {
        $state = parent::getSchemaStateDefaults();
        $state['data']['files'] = $this->getEncodedItems();
        $state['value'] = $this->Value() ?: ['Files' => []];
        // force react component to always update when field is rendered,
        // by making value.Files and data.files mismatch in the state
        $state['value']['Files'][] = -1;
        return $state;
    }

@RVXD
Copy link

RVXD commented Mar 22, 2022

#1261

@RVXD
Copy link

RVXD commented Apr 1, 2022

Can anyone assist in this change? The PR has failed checks, not sure why. Let's get this issue resolved.

@michalkleiner
Copy link
Contributor

@RVXD can you please tests kinglozzer's PR to see if it still works for you as expected?

kinglozzer added a commit to kinglozzer/silverstripe-asset-admin that referenced this issue Apr 4, 2022
kinglozzer added a commit to kinglozzer/silverstripe-asset-admin that referenced this issue Apr 8, 2022
GuySartorelli added a commit that referenced this issue Apr 14, 2022
FIX: Update UploadField redux state when form schema data changes (fixes #960)
@GuySartorelli
Copy link
Member

Fixed by #1263
Note that there is a deeper underlying redux issue as indicated by #1001, but that's beyond the scope of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.