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

Handle duplicate files #183

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Handle duplicate files #183

wants to merge 6 commits into from

Conversation

gwang0136
Copy link
Member

Status:

🚀 Ready

Description

Handles duplicate file uploads by appending subscripts.

Fixes #59

Todos

  • Tests
  • Documentation

@vercel
Copy link

vercel bot commented Apr 20, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hack4impact/3dp4me/D1FB6JAxg1e7adxbasPURNu1ERX3
✅ Preview: https://3dp4me-git-handle-duplicate-files-hack4impact1.vercel.app

Copy link
Collaborator

@mattwalo32 mattwalo32 left a comment

Choose a reason for hiding this comment

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

Good stuff, just some small changes

backend/routes/api/patients.js Outdated Show resolved Hide resolved
backend/routes/api/patients.js Outdated Show resolved Hide resolved
backend/routes/api/patients.js Outdated Show resolved Hide resolved
backend/routes/api/patients.js Outdated Show resolved Hide resolved
@mattwalo32
Copy link
Collaborator

Does this current version work locally for you. I'm running this locally and it doesn't seem to handle more than 1 duplicate file

@mattwalo32
Copy link
Collaborator

It looks like uploading a duplicated filename doesn't trigger a POST request to upload the file

@gwang0136
Copy link
Member Author

hmm seems to work for me locally. i upload a file, hit save, then upload the file again.

@gwang0136
Copy link
Member Author

gwang0136 commented Apr 23, 2021

wait, it only works when i shuffle between tabs before uploading each subsequent file. the post request is only triggered the first time after i hit the tab, then doesn't trigger when i try uploading more files unless i switch to another tab then switch back

^hm this only happens when I try uploading the same file but works fine for any other file lemme take a look

Copy link

@yousefa00 yousefa00 left a comment

Choose a reason for hiding this comment

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

Left some comments, great work, Gene :D

Comment on lines +173 to +175
var updatedFileName = filename;
var file = '';
var suffix = '';

Choose a reason for hiding this comment

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

Use let here

Comment on lines +177 to +178
file = filename.split('.')[0];
suffix = '.' + filename.split('.')[1];

Choose a reason for hiding this comment

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

This won't work for files with multiple . like .styles.js maybe use lastIndexOf() and substring to split around the last .

file = filename;
}
if (data.filter((e) => e.filename === filename).length > 0) {
var numPrev = 1;

Choose a reason for hiding this comment

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

let :)

} else {
file = filename;
}
if (data.filter((e) => e.filename === filename).length > 0) {

Choose a reason for hiding this comment

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

I would use if (data.some()) instead of filter since it's much clearer

Comment on lines +236 to +239
const modifiedFileName = await modifyFileName(
fileName,
stepData[fieldKey],
);

Choose a reason for hiding this comment

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

If I were looking at this without context, i wouldnt understand what this function does. Think a one line comment explaining what this function does would be super helpful!

@yousefa00
Copy link

Also this is the library i found: https://www.npmjs.com/package/uniquefilename

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.

Handle duplicate file uploads.
3 participants