search

camicroscope / Caracal

Conslidated Backend, Auth, and Security Services for caMicroscope
GNU General Public License v3.0
15 stars 94 forks source link

Delete the slide image locally when the slide data is deleted successfully from DB [Feature] #23

Closed cjchirag7 closed 4 years ago

cjchirag7 commented 4 years ago

I have used the 'fs' module to delete the file simply using the asynchronous unlink() function, when data gets deleted from DB successfully. But this requires ./images to be mounted in 'ca-back' container. For this, PR-118 of Distro repository should be merged first.

cjchirag7 commented 4 years ago

Fixes Issue-324 of caMicroscope repository, which reads 'File already exists error even after the slide is deleted'

cjchirag7 commented 4 years ago

@birm The functional test for 'Delete a slide' step has been updated, to test this feature.

Vedant1202 commented 4 years ago

@cjchirag7, Using system calls (such as 'rm' call to remove files, in this situation) is never advised to be implemented and is always kept as a last resort. This is not supported by all operating systems and putting up a condition to check the operating system and then executing respective remove commands is also not feasible.

Also error handling of system calls becomes very difficult in OS like windows and shared server based PCs.

A better fix was already done at PR-341 and PR-25.

Please consider the above problems before merging.

cjchirag7 commented 4 years ago

@Vedant1202 , yeah, you 're right. I have fixed this by using 'fs' module to remove the file. This would perform the delete operation, irrespective of the OS. I think, now there are no major issues with this PR. #25 has some major problems with the route security, which I have commented on the PR. These should be looked carefully, before performing any merge.

@birm , what are your thoughts ?

cjchirag7 commented 4 years ago

I don't understand, why the travis build is not being shown now ? :confused:

birm commented 4 years ago

ah, I have no idea. This may mean we have to go back to the stone age and check out the code and run tests manually :laughing:

birm commented 4 years ago

Closed in favor of #24 and its linked PRs. Thank you for the work in any case.