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

Fix #783 duplicate uploading same asset name #853

Merged
merged 6 commits into from
Dec 14, 2017

Conversation

Dammmien
Copy link
Contributor

#783 duplicate uploading same asset name

- Summary

Fixes #783 Duplicate uploading same asset name

- Test plan

Upload an image with the same name, it will ask you to replace the existing one.

- Description for the changelog

Juste check existing files name

@erquhart
Copy link
Contributor

erquhart commented Nov 29, 2017

Thanks for digging into this, @Dammmien! Definitely something the CMS needs. So there's a bit of complexity to consider:

  1. Some CMS implementations utilize a custom asset store that returns paginated results, which the media library accesses via infinite scroll. That means we won't always have all of the files on hand to compare.

  2. If assets are stored in Git, you can expect unique filenames, but not all assets will be stored in Git. In such cases, filenames aren't guaranteed to be unique, and an id is used instead.

We refer to the non-Git asset stores as "integrations", and your fix would need to be applied only in the absence of an asset integration. We also probably want this functionality in a Redux action rather than in the component itself.

Here's the associated action:
https://github.com/netlify/netlify-cms/blob/master/src/actions/mediaLibrary.js#L100-L138

You'll notice a bunch of it is conditionally run if there's an integration, and the rest runs if not - that last part is where you'll want to run your check and throw the prompt. I know GitHub won't paginate file results, and we can worry about other backends (e.g. GitLab/Bitbucket) as they get merged in.

If you can make that change, we should be in good shape to merge. Thanks again for your work!

@Dammmien
Copy link
Contributor Author

@erquhart, not sure to understand, something like the last commit ?

return dispatch(mediaPersistFailed({ privateUpload }));
}

const deletedFile = await deleteMedia(existingFile, { privateUpload });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteMedia returns a function, why not just dispatch it?

@erquhart
Copy link
Contributor

Yep, and I linked to the wrong action, but I see that didn't throw you off :)

Just one comment in my review and this should be good!

@erquhart
Copy link
Contributor

Hmm no deploy preview for some reason. Going to sanity check it locally before approving, but looks good - thanks @Dammmien!

@erquhart
Copy link
Contributor

erquhart commented Dec 6, 2017

@Dammmien I pushed an update to get all existing file logic into the action. There's still an outstanding issue where the last file uploaded in the current session doesn't get caught in the check, we should get that straightened before merging.

@verythorough
Copy link
Contributor

verythorough commented Dec 6, 2017

Deploy preview ready!

Built with commit 8711fa2

https://deploy-preview-853--netlify-cms-www.netlify.com

@erquhart
Copy link
Contributor

erquhart commented Dec 8, 2017

Note: we should also fix #540 here by making the check case-insensitive.

@verythorough
Copy link
Contributor

verythorough commented Dec 9, 2017

Deploy preview ready!

Built with commit 8711fa2

https://deploy-preview-853--cms-demo.netlify.com

@Dammmien
Copy link
Contributor Author

Dammmien commented Dec 9, 2017

@erquhart I pushed an update to compare file name and existing files names in lower case to make the check case-insensitive.

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

Successfully merging this pull request may close these issues.

None yet

3 participants