HistoryViewer for Files - files before a 'replace' are not shown

[pjayme]

On CWP2.2.3 / SS4.3.3 when this config is enabled:

SilverStripe\AssetAdmin\Forms\FileFormFactory:
  show_history: true

we get a HistoryViewer for Files like so:

Now the issue is that when a file is replaced using the Replace file function via the CMS the HistoryViewer component fails to display the correct image for an older version of the file, it instead displays the image of the newly replaced file.

We noticed that in terms of the backend, all the data in regards to file versions is there, it correctly creates a new version along with a new hash, an updated image and so forth, the backend is well intact.

It is the mapping between the front-end component and back-end that seems to be the issue where the front-end component is not getting the updated set/list of images from its history data.

[ScopeyNZ]

I wonder what's expected if historic files aren't being kept? (cc @silverstripeux)

[clarkepaul]

Mainly who has done what, and when (the meta-information which could be everything except the file). I would hope that there would be a way of keeping the files if someone chose to do so but that it wouldn't be the default.

[ScopeyNZ]

Sorry, I meant there is already config to either keep files or not. What should we display for the image if the file is not configured to be kept?

[clarkepaul]

Right you mean whether we need a placeholder (file not available/viewable) type thing or just show nothing at all. I think the former would be good, we can provide some visuals for this.

[pjayme]

https://recordit.co/6aXh1grd0R

For better clarity, the clip above demonstrates when a file is replaced with a new file, notice how the previous history records loses track of the original file/image when clicked on. It appears that once a file is replaced every single record in the history has its file/image updated to the new file.

[sachajudd]

Note: 4.3.4 or newer removes the History tab altogether. We do although have designs we want to implement in the future which will make this a more seamless experience see https://github.com/silverstripe/silverstripe-asset-admin/issues/581.

What should we display for the image if the file is not configured to be kept?

I think we could add what @clarkepaul suggested 'No preview available'.

I meant there is already config to either keep files or not.

@ScopeyNZ are you able to help @pjayme out with achieving this?

[samthejarvis]

PreviewImageField can't currently handle versioned records as the only method for setting the file record used is via ->setRecordID, which doesn't contain any reference to a file's version. We have a full record definition to use in FileHistoryFormFactory, can't foresee any problem with introducing a ->setRecord method for this purpose.

[Jianbinzhu]

when a file is replaced using the Replace file function via the CMS the HistoryViewer component fails to display the correct image for an older version of the file, it instead displays the image of the newly replaced file.

Hi, this is still happening on cwp/cwp-recipe-cms: 2.5.2 / silverstripe/asset-admin: 1.5.1

[emteknetnz]

By default the HistoryViewer component is not available for images, it must be switched on with the following config:

SilverStripe\AssetAdmin\Forms\FileFormFactory:
  show_history: true

When a file is replaced, the old physical file is deleted from the file system. This limitation means that you cannot roll back to a previous version of a physical file, though the metadata associated with that file in the database such as image title is available

I'm going to assume this limitation is somewhat behind the decision to remove the image history tab in the first place

I think the best we can do here is to not display the preview image for previous versions of image, and only display the metadata.

@clarkepaul ?

[brynwhyman]

@emteknetnz I think you can opt-in to retain files that are replaced, see: https://docs.silverstripe.org/en/4/developer_guides/files/file_storage/#archived

Presumably projects that want to enable version history for files would also consider opting in to retaining the file versions. @Jianbinzhu has your project opted into this following the docs above? E.g.:

SilverStripe\Assets\Flysystem\FlysystemAssetStore: keep_archived_assets: true

Re:

I think the best we can do here is to not display the preview image for previous versions of image, and only display the metadata.

Isn't this what we've already got designs for? See: https://github.com/silverstripe/silverstripe-asset-admin/issues/962#issuecomment-525136722

[emteknetnz]

Note: I've previously raised an issue about the docs regarding the un-clarity of getting file archiving to work

I found I needed this on my local to enable it:

SilverStripe\Assets\File:
  keep_archived_assets: true
SilverStripe\Assets\Flysystem\FlysystemAssetStore:
  keep_archived_assets: true
SilverStripe\Assets\AssetControlExtension:
  keep_archived_assets: true

Given that the documentation is currently poor, coupled the lack of people complaining that file archiving on deletion does not work, it does make me wonder about what the community uptake of this keeping archived files is.

I'm not really sure this proposed feature is worth implementing. To clarify: a) If a physical version of the archived/replaced image exists on the filesystem then show that in the preview image b) If it does not, then show the 'no preview available' placeholder instead

I'm going to make the assumption that in the vast majority of cases it's going to be b), because I'd expect very few sites are keeping archived/replaced images. In this scenario the placeholder is just taking up vertical real estate.

It's also worth noting that even if you are keeping archived images, there's still no "revert" button on the image history viewer. And if you are keeping archived images, you still also need to manually enable the image history viewer via config.

@brynwhyman if you still want to go ahead with this, bear in mind this should be treated as new feature development. I don't think this one can be done quickly as a bugfix

IMO the quick turnaround solution here is just remove the faulty image preview

[clarkepaul]

I'm going to make the assumption that in the vast majority of cases it's going to be b), because I'd expect very few sites are keeping archived/replaced images. In this scenario the placeholder is just taking up vertical real estate.

The historic image (or placeholder) is displayed pretty small. The main concept is to let people know if the historic version visually matches the current version. Sometimes it will match, sometimes it won't match, and sometimes it wont show because we don't have the historic version anymore. Even when we can't show what the file looked like we'd still need to let the viewer know we don't have the file anymore, thats basically what the placeholder area is trying to do.

[emteknetnz]

OK that makes sense

Would it be alright if we remove the preview image for now to fix what is bug, and look to implement the design as a feature at a later date? Even if we didn't remove the preview image now, due to competing priorities this feature work won't be happening short term so it makes sense to me to still remove the preview image. @clarkepaul ?

[clarkepaul]

I guess removing it is better than the current experience, I'd be cool with that. We should follow up with another issue to outline the full experience for all scenarios. If a user looks through the history feed they will see a message saying when it has been replaced.

Next best would be leaving the placeholder area there and only showing the historic file if it matches the current version (ignoring any other scenario, presenting the fallback message).

[emteknetnz]

Fixed in https://github.com/silverstripe/silverstripe-asset-admin/pull/1177

[bergice]

Fixed in silverstripe/silverstripe-framework#1177

@emteknetnz I've merged this.

I guess removing it is better than the current experience, I'd be cool with that. We should follow up with another issue to outline the full experience for all scenarios. If a user looks through the history feed they will see a message saying when it has been replaced.

Next best would be leaving the placeholder area there and only showing the historic file if it matches the current version (ignoring any other scenario, presenting the fallback message).

@clarkepaul do you have designs ready for this?

[clarkepaul]

@bergice there will be some but they won't be up-to-date with how this will look now. Not really designing things that aren't in the base installation by default.

[brynwhyman]

I've raised a follow up issue to discuss adding the preview back in and displaying a message if the file isn't still on the server. I'd say removing the preview is going to be helpful in not sharing inaccurate information, but this is probably a decent feature regression if it's left long-term. Keen to hear other people's view on that.

https://github.com/silverstripe/silverstripe-asset-admin/issues/1203