search

pinax / pinax-documents

a document management app for Django
MIT License
65 stars 20 forks source link

allow Folder deletion #13

Closed grahamu closed 8 years ago

grahamu commented 8 years ago

Since Documents can be deleted, surely Folders should be deletable as well?

grahamu commented 8 years ago

If we allow folder deletion, what happens when deleting a non-empty Folder?

paltman commented 8 years ago

Since it's destructive, perhaps we should provide a confirmation screen listing all the files that that will be deleted as well.

ethankent commented 8 years ago

I'm not sure how many documents are allowed in a folder. If there is no limit, we might want to consider doing the mass-delete as some sort of job. Just a thought for a future feature.

paltman commented 8 years ago

@ethankent good thinking. can you implement the actual deletion as a hookset method so the site builder could override the default functionality and offline it to whatever queuing system (e.g. celery) they happen to be using or desire to use?

ethankent commented 8 years ago

I'd be happy to take a look at that. However, I'd also like to raise one more question...

Since we're now beginning to raise issues related to recursion, we should at least consider whether (and when) we might want to offload some of these concerns to a library focused on tree structures.

  1. Should we consider incorporating a library like django-mptt or django-treebeard?
  2. If yes, should we consider implementing non-recursive deletion now and implementing recursive deletion with the help of such a library?
paltman commented 8 years ago

@ethankent good points. I'd like @brosner to weigh on this. For now, let's just implement it with the recursion, as horrible as that could get in a large tree, but it have all the logic implemented in the hookset method. It is a bit of kicking the can down the road a bit, but very interested in getting to feature complete with the simplest thing possible for now, while provide some hooks for people to override things while we determine more performant solutions for future releases.

ethankent commented 8 years ago

Hi Everyone,

Just wanted to let you know that I have a PR on the way for this. But I'm trying to get the required PR for pinax-theme-bootstrap first. This order should prevent the build from breaking.

ethankent commented 8 years ago

PR submitted! Let me know if you see any improvements I can make.