Add support for JPEG deck covers by marccarre · Pull Request #49 · floscha/tinycards-python-api

marccarre · 2019-03-13T21:46:01Z

Fixes #47.

Screenshots:

After deck creation:

After deck update:

coveralls · 2019-03-13T22:32:48Z

Pull Request Test Coverage Report for Build 255

29 of 30 (96.67%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.0%) to 88.347%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tinycards/networking/rest_api.py	11	12	91.67%

Totals
Change from base Build 250:	1.0%
Covered Lines:	417
Relevant Lines:	472

💛 - Coveralls

floscha · 2019-03-15T22:18:51Z

Hi @marccarre, thanks for the PR. Creating new decks with a cover image works like a charm for me. However, when trying to change the cover of an existing deck and calling update_deck(), the change will simply be ignored by the Tinycards backend. While technically this goes beyond #47, which was only about creating decks with a cover, I still believe the option to change covers after the deck's creation could still be helpful for many. What do you think?

marccarre · 2019-03-15T22:44:47Z

Fair point, I'll update this PR accordingly.
EDIT: rebased on latest master.

It previously was just ignored, and left unused.

It previously was ignored, but with a commented-out line which suggested this is where this should be added.

…e endings of the actual form

The current test really is just testing requests_toolbelt.multipart.encoder.MultipartEncoder, and has no real reason to remain as this MultipartEncoder is already heavily tested upstream. Note that prior to removal, tests were passing under some versions of Python, but not others, due to different orderings of the form values: - https://travis-ci.org/floscha/tinycards-python-api/jobs/506018179 - https://travis-ci.org/floscha/tinycards-python-api/builds/506018178 Besides, this logic is also covered by integration tests.

marccarre · 2019-03-16T00:05:50Z

FYI:

when the cover isn't changed, updating the deck sends a PATCH with content-type: application/json,
when the cover is changed, updating the deck sends a PATCH with content-type: multipart/form-data, and an imageFile part containing the image's binary data, just like when creating a deck. The payload doesn't change otherwise.

This is a more robust test, albeit a bit more expensive, as the default cover could also eventually be hosted by cloudfront.

Otherwise, the reason of the failure is masked, which hinders debugging.

Previously, the following would be added to the payload, resulting in errors server side, since invalid JSON: ``` Content-Disposition: form-data; name="cards"\r\n \r\n [{\'creationTimestamp\': 1552700740000, \'sides\': [{\'concepts\': [{\'fact\': {\'text\': \'front test 1\', \'type\': \'TEXT\'}}]}, {\'concepts\': [{\'fact\': {\'text\': \'back test 1\', \'type\': \'TEXT\'}}]}]}, {\'creationTimestamp\': 1552700740000, \'sides\': [{\'concepts\': [{\'fact\': {\'text\': \'front test 2\', \'type\': \'TEXT\'}}]}, {\'concepts\': [{\'fact\': {\'text\': \'back test 2\', \'type\': \'TEXT\'}}]}]}]\r\n ``` Indeed, when updating a deck's cover in the web UI, cards are encoded like this: ``` Content-Disposition: form-data; name="cards"\r\n \r\n [{"sides":[{"concepts":[{"fact":{"text":"Q1","type":"TEXT"}}]},{"concepts":[{"fact":{"text":"A1","type":"TEXT"}}]}]},{"sides":[{"concepts":[{"fact":{"type":"TEXT","text":"Q2"}}]},{"concepts":[{"fact":{"type":"TEXT","text":"A2"}}]}]}]\r\n ```

marccarre · 2019-03-16T03:01:44Z

Done:

updating a deck now also uploads its cover,
testing improved,
documentation improved,
PR updated with post-deck-update screenshot.

@floscha, PTAL 🙂

floscha · 2019-03-16T14:27:14Z

The code and tests look good so far. When toying around with the functionality a bit, I found that while adding cover images from local files works as expected, using a URL for images that have been uploaded to the internet already, is simply disregarded by Tinycards. When checking the requests used by the Tinycards web page, you see that it also sends the cover image in binary format, even when selecting an image from a URL.

So I figured one possible solution to this could be trying to download the image from the specified URL and if successful (i.e. getting a 2** HTTP response), submitting the downloaded image as bytes just like when using a local file. One problem to keep in mind with this approach though is to make sure that the image is only downloaded and updated when it was actually changed and not with every update made to the deck. One solution to this could be using the coverImageUrl field (which unfortunately is not included in the Python Deck class yet). This field appears to be set by the Tinycards backend only after the image was updated (i.e. the local file path has been replaced with the URL to the uploaded version of the image on cloudfront.net). Therefore, it should be possible to simply check if imageUrl (i.e. the cover property of the Deck class) equals coverImageUrl.

marccarre · 2019-03-16T21:12:22Z

Sure, sounds good to me!
Would it be fine to do it in a follow-up PR and merge this one @floscha? (It has diverged from master quite a bit already, and it would make it easier to parallelise work streams, e.g. #50.)

marccarre · 2019-03-16T21:56:40Z

Issue created. I'll reply there to avoid mixing discussion threads.

floscha · 2019-03-16T21:57:40Z

Thanks a lot! 🙂

Also, I'll bump the version of the library to 0.27 after merging so that your changes are also available when pip-installing the library.

marccarre · 2019-03-16T22:14:12Z

Thanks! 🙂

marccarre · 2019-03-16T22:40:27Z

Looks like #53 and #49 clashed. Opening an issue and will fix soon.

marccarre mentioned this pull request Mar 13, 2019

Feature request: upload of deck's cover #47

Closed

marccarre force-pushed the issues/47-upload-deck-cover branch from a54fdc2 to aa643be Compare March 13, 2019 22:31

marccarre force-pushed the issues/47-upload-deck-cover branch from 29a3d95 to c224621 Compare March 13, 2019 22:40

floscha mentioned this pull request Mar 15, 2019

Improvement: nicer API when adding multiple facts on the back of cards #51

Closed

marccarre force-pushed the issues/47-upload-deck-cover branch 2 times, most recently from b3995ae to f019f54 Compare March 15, 2019 23:45

marccarre added 6 commits March 16, 2019 08:45

Add requests-toolbelt 0.9.1 as a dependency

34dfc0c

Deck now sets the provided cover input argument as a field

84e1128

It previously was just ignored, and left unused.

deck_to_json now sets Deck#cover under imageFile

898e0db

It previously was ignored, but with a commented-out line which suggested this is where this should be added.

Add upload of jpeg deck cover (+integration test)

3d58a6c

test_simple_multipart_form_parsing now UTF8-decodes and Unix-ises lin…

2f19917

…e endings of the actual form

marccarre force-pushed the issues/47-upload-deck-cover branch from f019f54 to 7667128 Compare March 15, 2019 23:46

marccarre added 5 commits March 16, 2019 09:24

Actually test that the uploaded cover matches byte for byte

29a7f5a

This is a more robust test, albeit a bit more expensive, as the default cover could also eventually be hosted by cloudfront.

Add error details in the exception raised on failed deck update

3da9261

Otherwise, the reason of the failure is masked, which hinders debugging.

Add support for update of decks' covers (+tests)

de6524b

Document cover in Deck.__init__'s docstring

6550856

marccarre mentioned this pull request Mar 16, 2019

Add support for deck covers of any file type, from URL #54

Closed

floscha merged commit a24eb63 into floscha:master Mar 16, 2019

marccarre deleted the issues/47-upload-deck-cover branch March 16, 2019 22:14

marccarre mentioned this pull request Mar 16, 2019

Bug: AttributeError: 'Deck' object has no attribute 'user_id' #55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for JPEG deck covers#49

Add support for JPEG deck covers#49
floscha merged 11 commits intofloscha:masterfrom
marccarre:issues/47-upload-deck-cover

marccarre commented Mar 13, 2019 •

edited

Loading

Uh oh!

coveralls commented Mar 13, 2019 •

edited

Loading

Uh oh!

floscha commented Mar 15, 2019

Uh oh!

marccarre commented Mar 15, 2019 •

edited

Loading

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019 •

edited

Loading

Uh oh!

floscha commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

floscha commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

marccarre commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots:

Uh oh!

coveralls commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 255

💛 - Coveralls

Uh oh!

floscha commented Mar 15, 2019

Uh oh!

marccarre commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

floscha commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

floscha commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

marccarre commented Mar 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marccarre commented Mar 13, 2019 •

edited

Loading

coveralls commented Mar 13, 2019 •

edited

Loading

marccarre commented Mar 15, 2019 •

edited

Loading

marccarre commented Mar 16, 2019 •

edited

Loading