fix: Avoid raising 404 when file cannot generate a thumbnail #642

DaveSawyer · 2021-12-04T00:10:44Z

for issue #641. For some files that cannot generate thumbnails we should not attempt to download from the url field. If we do, an application may interpret the 404 not found as similar to "try again in case the thumbnail was in the process of being generated." We can also avoid an extra network call that we know will fail.

I made this intentionally narrow (as opposed to status != 'success') because some failures might be solved by waiting and retrying or actually have a valid URL.

Co-authored-by: Marcin Hagmajer <mhagmajer@users.noreply.github.com>

This limit is set, because >=3.5.0 versions require Rust

Closes: SDK-1666

…older (box#637) Closes: SDK-1815

Closes: SDK-1573

coveralls · 2021-12-04T00:23:24Z

Pull Request Test Coverage Report for Build 2739

6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.003%) to 96.751%

Totals
Change from base Build 2733:	0.003%
Covered Lines:	3186
Relevant Lines:	3293

💛 - Coveralls

antusus · 2021-12-06T07:37:49Z

Hi @DaveSawyer ,

Thanks for this contribution! We will review it ASAP!

@antusus

lukaszsocha2 · 2021-12-06T09:51:34Z

boxsdk/object/file.py

-        if representation:
-            url = representation[0]['content']['url_template']
+        representations = self.get_representation_info(rep_hints)
+        if len(representations):


An empty list is treated as False, so if representations: is enough

antusus

Looks OK. I have some questions.

boxsdk/object/file.py

antusus · 2021-12-07T05:47:09Z

Please wait with merging until we solve the travis build issue.

antusus

One thing that I overlooked - could you ad an entry to HISTORY.srt

lukaszsocha2 · 2021-12-07T14:02:27Z

Also pylint is failing with messages:
`test.unit.object.test_file:904:11: [C0122(misplaced-comparison-constant), test_get_thumbnail_representation_not_found] Comparison should be thumb == ''

test.unit.object.test_file:950:11: [C0122(misplaced-comparison-constant), test_get_thumbnail_representation_not_available] Comparison should be thumb == ''`

lukaszsocha2 · 2021-12-07T14:03:18Z

test/unit/object/test_file.py

@@ -901,4 +901,50 @@ def test_get_thumbnail_representation_not_found(
        headers={'X-Rep-Hints': '[{}?dimensions={}]'.format(extension, dimensions)},
        params={'fields': 'representations'},
    )
-    assert thumb == b''
+    assert b'' == thumb


assert thumb == b''

They are equivalent and in general it's better to put a constant on the left so you don't accidentally have an assignment from a typo. This is the existing code.

I see your point, but according to pylint the other way around it preferred, see: https://vald-phoenix.github.io/pylint-errors/plerr/errors/basic/C0122 . Without fixing it the build will be still failing.

lukaszsocha2 · 2021-12-07T14:03:26Z

test/unit/object/test_file.py

+        headers={'X-Rep-Hints': '[{}?dimensions={}]'.format(extension, dimensions)},
+        params={'fields': 'representations'},
+    )
+    assert b'' == thumb


assert thumb == b''

lukaszsocha2 · 2021-12-08T11:12:01Z

Hi, @DaveSawyer thanks for contributing! I took over your pr because for today a new release is planned and we wanted to include your change in it.

DaveSawyer changed the title ~~Avoid raising 404 when file cannot generate a thumbnail~~ Fix raising 404 when file cannot generate a thumbnail Dec 4, 2021

DaveSawyer changed the title ~~Fix raising 404 when file cannot generate a thumbnail~~ Fix: raising 404 when file cannot generate a thumbnail Dec 4, 2021

DaveSawyer changed the title ~~Fix: raising 404 when file cannot generate a thumbnail~~ fix: raising 404 when file cannot generate a thumbnail Dec 4, 2021

DaveSawyer changed the title ~~fix: raising 404 when file cannot generate a thumbnail~~ fix: avoid raising 404 when file cannot generate a thumbnail Dec 4, 2021

DaveSawyer and others added 21 commits December 3, 2021 16:18

Avoid raising 404 when file cannot generate a thumbnail

dc03a81

Fix Travis CI for Python 2 and Pylint (box#608)

9abbc96

Adding group variant tag for devdocs (box#602)

f8bbc6f

Remove or deprecate insensitive language (box#609)

c1a0a02

Co-authored-by: Marcin Hagmajer <mhagmajer@users.noreply.github.com>

Update README.rst

6124735

Fix PyPy build (box#616)

e154943

Add BoxSign support (box#617)

835b5dc

Fix flaky test (box#619)

26ca810

Upgrade cryptography to version 3 (box#620)

2c47119

Update HISTORY.rst

9763103

Fix conf.py

87059a5

Update HISTORY.rst

f9fd2fd

v2.13.0 (box#622)

1dc5fb7

Add admin_logs_streaming support for events stream (box#623)

7c3d722

Fix SignRequest docs (box#624)

5fd636e

chore: Update Issue Templates (box#626)

a196cb4

Add create_release workflow

14d2e93

dependencies: Limiting cryptography library version to <3.5.0 (box#636)

724f898

This limit is set, because >=3.5.0 versions require Rust

docs: Document "params" usage for making manual API calls (box#635)

cb0bf6c

Closes: SDK-1666

feat: Add vanity_name param for creating shared link to a file or f…

895f09c

…older (box#637) Closes: SDK-1815

feat: New API for get files and file versions under retention (box#633)

3eaa3ed

Closes: SDK-1573

DaveSawyer force-pushed the dave/no_thumbnail_files branch from d4eba16 to 3eaa3ed Compare December 4, 2021 00:20

Merge branch 'main' into dave/no_thumbnail_files

c0905b0

DaveSawyer changed the title ~~fix: avoid raising 404 when file cannot generate a thumbnail~~ fix: Avoid raising 404 when file cannot generate a thumbnail Dec 4, 2021

lukaszsocha2 reviewed Dec 6, 2021

View reviewed changes

antusus reviewed Dec 6, 2021

View reviewed changes

boxsdk/object/file.py Show resolved Hide resolved

drop the len

b3ab4f2

antusus approved these changes Dec 7, 2021

View reviewed changes

antusus requested changes Dec 7, 2021

View reviewed changes

Merge branch 'main' into dave/no_thumbnail_files

14aff0c

lukaszsocha2 requested changes Dec 7, 2021

View reviewed changes

lukaszsocha2 added 3 commits December 8, 2021 11:01

Update HISTORY.rst

fe56909

Fixed pylint warnings

79e8f48

Merge branch 'main' into dave/no_thumbnail_files

a792f0f

lukaszsocha2 approved these changes Dec 8, 2021

View reviewed changes

antusus approved these changes Dec 8, 2021

View reviewed changes

lukaszsocha2 merged commit 07058be into box:main Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Avoid raising 404 when file cannot generate a thumbnail #642

fix: Avoid raising 404 when file cannot generate a thumbnail #642

DaveSawyer commented Dec 4, 2021 •

edited

coveralls commented Dec 4, 2021 •

edited

antusus commented Dec 6, 2021

lukaszsocha2 Dec 6, 2021

antusus left a comment

antusus commented Dec 7, 2021

antusus left a comment

lukaszsocha2 commented Dec 7, 2021

lukaszsocha2 Dec 7, 2021

DaveSawyer Dec 7, 2021 •

edited

lukaszsocha2 Dec 7, 2021

lukaszsocha2 Dec 7, 2021

lukaszsocha2 commented Dec 8, 2021

fix: Avoid raising 404 when file cannot generate a thumbnail #642

fix: Avoid raising 404 when file cannot generate a thumbnail #642

Conversation

DaveSawyer commented Dec 4, 2021 • edited

coveralls commented Dec 4, 2021 • edited

Pull Request Test Coverage Report for Build 2739

💛 - Coveralls

antusus commented Dec 6, 2021

lukaszsocha2 Dec 6, 2021

Choose a reason for hiding this comment

antusus left a comment

Choose a reason for hiding this comment

antusus commented Dec 7, 2021

antusus left a comment

Choose a reason for hiding this comment

lukaszsocha2 commented Dec 7, 2021

lukaszsocha2 Dec 7, 2021

Choose a reason for hiding this comment

DaveSawyer Dec 7, 2021 • edited

Choose a reason for hiding this comment

lukaszsocha2 Dec 7, 2021

Choose a reason for hiding this comment

lukaszsocha2 Dec 7, 2021

Choose a reason for hiding this comment

lukaszsocha2 commented Dec 8, 2021

DaveSawyer commented Dec 4, 2021 •

edited

coveralls commented Dec 4, 2021 •

edited

DaveSawyer Dec 7, 2021 •

edited