Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Imaging Uploader] Prevent duplicate insertions #4024

Merged
merged 4 commits into from
Nov 6, 2018

Conversation

davidblader
Copy link
Contributor

Brief summary of changes

Imaging Uploader only checks if a file already exists on the front end. This makes it possible to insert duplicate scans which happens surprisingly often on CCNA. This change adds a backend check for an existing mri_upload entry that is not being intentionally overwritten / updated.

This resolves issue...

mri_upload duplicates

To test this change...

Open the Imaging Uploader Upload tab in two browser tabs. Fill out both simultaneously for the same scan, then submit one after the other. The first submission should go through and the other should throw an error.

@johnsaigle johnsaigle added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label Oct 24, 2018
(ISNULL(Inserting) OR Inserting=0), UploadID, NULL) as UploadID
FROM mri_upload WHERE UploadLocation LIKE :ul",
array('ul' => "%$file_name")
);
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to add a file_exists check here if you have access to the full path to the file. This only checks whether there's a record in the database.

//////// Make sure there's no existing identical entry ////////////////
//////// for "new" scans //////////////////////////////////////////////
///////////////////////////////////////////////////////////////////////
$updateFile = $args['values']['overwrite'] ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this ternary operator is necessary. What's the value of 'overwrite'? Can you just pass it along to L387?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just copied it over from the importFile($file, $args) function. i think it only gets set when the user confirms they want the original upload overwritten

@kongtiaowang kongtiaowang added the Passed Manual Tests PR has undergone proper testing by at least one peer label Nov 5, 2018
@kongtiaowang
Copy link
Contributor

pass test and ready to go.

@nicolasbrossard nicolasbrossard self-assigned this Nov 5, 2018
@nicolasbrossard
Copy link
Contributor

I opened two tabs in Google Chrome and tried uploading the same file in both of these tabs: one of them reported an upload failure, saying that the file was already uploaded (as expected) 👍

///////////////////////////////////////////////////////////////////////
//////// Make sure there's no existing identical entry ////////////////
//////// for "new" scans //////////////////////////////////////////////
///////////////////////////////////////////////////////////////////////
Copy link
Collaborator

Choose a reason for hiding this comment

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

How long did it take you to type all those slashes?

@driusan driusan merged commit e330f70 into aces:bugfix Nov 6, 2018
@ridz1208 ridz1208 added this to the 20.0.3 milestone Dec 3, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants