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

"(Edit) Details" button only visible if grid shows the page of selected file #1136

Closed
1 of 2 tasks
sb-relaxt-at opened this issue Sep 10, 2020 · 19 comments
Closed
1 of 2 tasks

Comments

@sb-relaxt-at
Copy link

sb-relaxt-at commented Sep 10, 2020

Given a folder with so many images that the don't fit on one page of the asset admin overlay and an UploadField.

  1. Select an image from the second page in the UploadField and insert
  2. Click on the eye symbol for the just added file in the UploadField in order to edit it
  3. The overlay will show page one (without the prior selected image)
  4. You won't be able to edit the file using the "Details" button as this is only visible if the page with the file is selected

I would expect the overlay to automatically show the relevant page with the selected image. And the "Details" button should be visible independent of the page selected.

PRs

@sb-relaxt-at sb-relaxt-at changed the title "(Edit) Details" button only visible if grid shows the page of the File "(Edit) Details" button only visible if grid shows the page of selected file Sep 10, 2020
@PortableSteve
Copy link

I've spent the last day trying to figure out why the "Details" button was showing for some images and not others - I can confirm it seems to be based on whether the image is on the visible page in the image browser on the left. In fact, if you switch between pages, the "Details" button is displayed/removed as you move on/off the relevant page.

@jules0x
Copy link

jules0x commented Oct 11, 2020

I'm also experiencing this.
I am able to replicate as per sb-relaxt-at and PortableSteve comments above.
This is a high user impact, so I'm unable to upgrade clients to this version/recipes using this version 😢

@Leapfrognz
Copy link

I have had to send an email to several clients highlighting this issue, so In my opinion this is a major.

@brynwhyman
Copy link

FYI, our team are planning to get started on a fix for this in the next 2-3 weeks.

@bergice bergice self-assigned this Oct 20, 2020
@brynwhyman brynwhyman added this to the Sprint 71 milestone Oct 27, 2020
@bergice
Copy link
Contributor

bergice commented Oct 28, 2020

The details button depends on the file being passed to the EditorHeader component. The file is not found when viewing a page without the selected file because the file is looked up from the list of files on the current page (see AssetAdmin.findFile).

The rest of the Editor component renders fine on a different page, but I think that's either because it's rendered using FormBuilder/redux-form or because we override the createFn for the EditorHeader component (see Editor.createFn).

@maxime-rainville
Copy link
Contributor

Just summarising the discussion we had on Slack.

Part of the problem is on this line:

const onDetails = nextType && file && file.type !== 'folder' ?
() => {
actions.modal.stashFormValues(formid, schemaUrl);
actions.modal.pushFormStackEntry(nextType);
} :
undefined;

The purpose of this snippet is to decide what action should happen when you click the "Details" button. If no action is provided, then the button doesn't show at all.

The reason we are checking if the file is a folder is that the insert modal allows you to edit folders, but not to insert them. Se when the Editor loads a folder, it's already editing the "details" of the folder. So there isn't any point in showing the "Details" button.

What file is passed down to the editor is determined by this method in AssetAdmin.

findFile(fileId) {
const allFiles = this.getFiles();
return allFiles.find((item) => item.id === parseInt(fileId, 10));
}

findFile tries to looks through the GraphQL result set and find a file with a specific ID. This works OK until you decide to edit a file that isn't on the current page of results.

Caching the result set is not really an option. GraphQL caching is kind of difficult ... there's ways we could get it done with Apollo, but implementing it has far reaching implication that goes way beyond the scope of this card. Plus, it's not going to solve the problem when you directly access a file for edition that is not on the first page.

There's two approaches we could take to solve this problem:

  • Either we update the Editor component so it detects if it's missing the file metadata it needs and fires it's own GraphQL query to retrieve the specific file it's editing.
  • We update the backend logic that generate the editor file schema so it passes along some file metadata as well. We could then update the Editor to read that information when it's building it's onDetails method. Or we could tell the Editor to always pass down a onDetail prop and tell the EditorHeader component directly if it's being used with a Folder and let it decide itself whether to render the Detail button.

@maxime-rainville
Copy link
Contributor

Just following up. I think a natural way to split this would be to:

  • create a bit of reusable logic that let you fetch a specific file by ID
  • then use that bit of logic to fetch the file meta data in EditorHeader

The first bit is probably a ~5 and could have other uses elsewhere. Second bit is probably something like a ~3.

@jules0x
Copy link

jules0x commented Feb 24, 2021

This is still an issue at version 1.7.1
Our clients are stuck on version 1.5.2 in order to get around it, which over time, is introducing visual issues due to changes in other recipe modules
e.g.
image

@josephlewisnz
Copy link

+1 we have also had to revert our recipe version (on serveral sites) to bring back our asset-admin to v 1.5.2.

Is there any workaround known?

@Leapfrognz
Copy link

We have just had this pop up again from another client. It would be good to get this sorted. Unfortunately, this isnt on a level that we feel we could submit a PR, as it requires a significant amount of work and deep knowledge of the assets backend/client side.

@jules0x
Copy link

jules0x commented Jun 30, 2021

This is still an issue.
We're still having to restrict some clients to version 1.5.2 yet version 1.8.1 is out.
What are the security implications of this?

Given the earlier comment about effort to investigate and solve this, can I petition the team to increase the priority of this issue?

Please and thank you 😅

Just following up. I think a natural way to split this would be to:

create a bit of reusable logic that let you fetch a specific file by ID
then use that bit of logic to fetch the file meta data in EditorHeader
The first bit is probably a ~5 and could have other uses elsewhere. Second bit is probably something like a ~3.

@emteknetnz emteknetnz self-assigned this Jul 2, 2021
@emteknetnz
Copy link
Member

What are the security implications of this?

There's a page on silverstripe.org that lists the patched security issues, including the version number they were fixed:
https://www.silverstripe.org/download/security-releases/

can I petition the team to increase the priority of this issue?

I've picked this issue up to have look at, though I cannot provide an ETA at this stage

@emteknetnz
Copy link
Member

emteknetnz commented Jul 2, 2021

PR #1221

@maxime-rainville
Copy link
Contributor

Got a proof of concept solution #1225

@emteknetnz
Copy link
Member

Linked PR has been merged

@emteknetnz emteknetnz removed their assignment Jul 19, 2021
@maxime-rainville
Copy link
Contributor

Fix has been release in 1.8.3

@quamsta
Copy link

quamsta commented Aug 30, 2021

@maxime-rainville Is there a trick to getting this installed via recipe-cms, which constrains asset-admin to 1.8.0@stable?

@michalkleiner
Copy link
Contributor

michalkleiner commented Aug 30, 2021

@quamsta you'd run composer require "silverstripe/asset-admin" "1.8.3 as 1.8.0" from CLI. That adds/updates just the single module and aliases the newer version as 1.8.0.

@quamsta
Copy link

quamsta commented Aug 31, 2021

Thank you!

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.