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

Functionality to delete slide from file system #341

Merged
merged 36 commits into from
Apr 14, 2020
Merged

Functionality to delete slide from file system #341

merged 36 commits into from
Apr 14, 2020

Conversation

Vedant1202
Copy link
Contributor

This fix contains:

  1. Call to delete slide from file system.
  2. Appropriate alerts and callbacks for successful deletion of file.
  3. Update to deleteSlide function by chaining it to 'delete from file system' function.

This fix solves #324 .

@cjchirag7
Copy link
Contributor

cjchirag7 commented Apr 7, 2020

@Vedant1202 , Chances are there that due to some reason, the data is not deleted from DB, but the image file gets deleted, with this approach. It would be better that the file is deleted using the node backend only (Caracal repository) and only when the delete from MongoDB database is successful. I have made this feature in PR-23 of Caracal repository.

@Vedant1202
Copy link
Contributor Author

@cjchirag7, I've already considered this issue and have chained the processes to each other by promises. So, unless one promise is handled successfully, the other does'nt get executed.

I've looked at your PR and there are some serious issues in it please look into it here.

@Vedant1202
Copy link
Contributor Author

Also, putting all file access code in SlideLoader is much better just to make sure all related code stays in one place.

@cjchirag7
Copy link
Contributor

@Vedant1202 , yeah I didn't notice you're chaining the promises correctly. But your PR #25 has some major problems with the route security, which I have commented on the PR. These should be looked into, carefully.
I have fixed all those issues with #23 . You may review the code once :)

@Vedant1202
Copy link
Contributor Author

@cjchirag7 I did the updates in PR-25. You can have a look.

@birm
Copy link
Member

birm commented Apr 11, 2020

I think this looks good; just will have to wait to co-test this with camicroscope/SlideLoader#25

@birm birm self-requested a review April 12, 2020 23:32
Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

It's clear that a lot of work has gone into this, thank you very much for your time and effort! Approved, pending backend change.

@birm birm merged commit 51dee23 into camicroscope:develop Apr 14, 2020
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.

4 participants