fix: crash in the file detail view

django-cms / django-filer

File and Image Management Application for django

https://django-filer.readthedocs.io/

Other

1.76k stars 577 forks source link

fix: crash in the file detail view #1395

Closed vinitkumar closed 1 year ago

vinitkumar commented 1 year ago

Description

In a recent fix, we introduced a regression where we started to access width and height of a file. However, this will only work for images and not for normal files like PDF, Docx etc.

In order to fix this, we need to guard and check if the file is actually an image before trying to access the attributes.

Related resources

Github Issue: #1394

Checklist

[x] I have opened this pull request against master
[x] I have added or modified the tests when changing logic
[x] I have followed the conventional commits guidelines to add meaningful information into the changelog
[x] I have read the contribution guidelines and I have joined #workgroup-pr-review on Slack to find a “pr review buddy” who is going to review my pull request.

codecov[bot] commented 1 year ago

Codecov Report

Merging #1395 (2dc96ec) into master (83e209f) will increase coverage by 0.02%. The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1395      +/-   ##
==========================================
+ Coverage   74.16%   74.19%   +0.02%     
==========================================
  Files          75       75              
  Lines        3445     3449       +4     
  Branches      553      554       +1     
==========================================
+ Hits         2555     2559       +4     
  Misses        720      720              
  Partials      170      170

Impacted Files	Coverage Δ
filer/templatetags/filer_admin_tags.py	`89.38% <88.88%> (+0.38%)`	:arrow_up:

marksweb commented 1 year ago

Could you add a test or two to ensure this doesn't regress again?

vinitkumar commented 1 year ago

Could you add a test or two to ensure this doesn't regress again?

Done, please verify if it's correct.

fsbraun commented 1 year ago

@vinitkumar Great PR! Thank you! Just as a side note: You can (and probably should) use self.assertIn(x, y) instead of assert x in y in tests: This will show the error in a more readable way.