search

akolpakov / django-unused-media

Remove unused media files from Django project
MIT License
125 stars 25 forks source link

Improvements and bugfixes #9

Closed vartagg closed 7 years ago

vartagg commented 7 years ago
  1. Application should manipulate absolute paths instead of relative paths. The main reason of this is the fact that application don't working now with MEDIA_ROOT declared as absolute path. Absolute path of MEDIA_ROOT usually always used on server, especially on production environments, and it's confused when command removes all media when I tried this on project having absolute MEDIA_ROOT.
  2. remove_empty_dirs optional argument added instead of force usage of this behaviour. I added this because not everyone wants to remove empty dirs. Sometimes it can be useful, sometimes not. In my opinion, it would be good to make this behaviour as optional argument.
  3. Fixed incorrect option character in command output. This was y in output but real character is uppercase - Y
  4. Tests fixed after replacing relative paths to absolute paths
  5. Django v1.11 passed to install_requires. I tried this app on my dev and production environments, so it's working well on Django==1.11.
akolpakov commented 7 years ago

As declared in Docs MEDIA_ROOT is "Absolute filesystem path to the directory that will hold user-uploaded files.". And plugin is working with absolute path, even in tests absolute path is using.

Please, can you explain with example what is your problem?

And better to create separate pull requests for different fixes. Easier to review and merge.

dperetti commented 7 years ago

I can confirm than I ran the command, and ALL the content of my media folder (not just the the extra files that were properly identified) was deleted... 😱

vartagg commented 7 years ago

@akolpakov

And plugin is working with absolute path, even in tests absolute path is using.

Yes, it is. But unfortunately, tests do not cover this bug. If you test with at least such parameters:

I sure that the last is a key parameter, because I saw in Debugger when images/1/2017/image.jpg (stored in fileobj.url) is comparing with something like ../../../images/1/2017/image.jpg ("relative path" of file provided by os.path module when MEDIA_ROOT path was higher then django application directory in filesystem tree) so comparison function didn't detect this because ../../../images/1/2017/image.jpg is not a real path bound with some django file field object. So, everything in MEDIA_ROOT (after confirmation and on test environment, what is good) is deleted.

vartagg commented 7 years ago

And better to create separate pull requests for different fixes. Easier to review and merge.

Unfortunately I don't have time for create multiple pull-requests, so I haven't see this condition for contributors in your README file when created my pull-request. I just used your library on a real project, fixing all found bugs, fixing tests for these changes, and sent you feedback with PR. In addition, the library is not that big, and for my opinion multiple pull-requests for every change is overkill.

akolpakov commented 7 years ago

Thanks @vartagg! I divided it to several commits and implemented everything except optional removing of empty dirs. It will be implemented in #12