Skip to content

Create image URLs using the cache optimized URLs #553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Jun 26, 2018
Merged

Create image URLs using the cache optimized URLs #553

merged 7 commits into from
Jun 26, 2018

Conversation

@erral
Copy link
Member

@erral erral commented Jun 23, 2018

This fixes #494

@erral
Copy link
Member Author

erral commented Jun 23, 2018

I had to patch the uuid4 method, and achieved that using mock as dependency, I don't know if that's good practice here... I haven't seen something similar in the codebase.

@coveralls
Copy link

coveralls commented Jun 23, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 96.418% when pulling e00b2ed on issue-494-image-scales into d2a4221 on master.

@erral erral force-pushed the issue-494-image-scales branch 2 times, most recently from dcdb21c to d95bb83 Compare June 24, 2018 07:48
@erral erral force-pushed the issue-494-image-scales branch from d95bb83 to 7130892 Compare June 24, 2018 07:59
@erral erral changed the title [WIP] Create image URLs using the cache optimized URLs Create image URLs using the cache optimized URLs Jun 24, 2018
@erral
Copy link
Member Author

erral commented Jun 24, 2018

I'm not pretty sure if the way I used to create the scale URLs is fully optimal. I called the @@images view and used it...

@davisagli
Copy link
Member

davisagli commented Jun 24, 2018

@erral it looks right to me

@tisto tisto requested review from davisagli, jensens and lukasgraf June 24, 2018 13:18
@tisto tisto added this to the 3.0.0 milestone Jun 24, 2018
@tisto tisto self-requested a review June 24, 2018 19:52
@tisto
Copy link
Member

tisto commented Jun 25, 2018

@erral would you mind adding a short note to our upgrade guide? If you don't have time for it I can also take care of it tomorrow...

@erral
Copy link
Member Author

erral commented Jun 26, 2018

Done.

@tisto
Copy link
Member

tisto commented Jun 26, 2018

@erral the real implementation returns different URLs with different UUIDs for every single scale as described here:

https://github.com/plone/plone.restapi/pull/553/files#diff-772cee6f1ad591450b99fdde08976749

right?

The auto-generated response just has the same URL/UUID for all image scales just because of the mock, is that correct?

@erral
Copy link
Member Author

erral commented Jun 26, 2018

Yes, that's right.

@erral
Copy link
Member Author

erral commented Jun 26, 2018

There is a note on the documentation explaining it: https://github.com/plone/plone.restapi/pull/553/files#diff-c294eb935ad014e7ea890908cbd17e75

@tisto tisto merged commit 1255707 into master Jun 26, 2018
@tisto tisto deleted the issue-494-image-scales branch June 26, 2018 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davisagli davisagli davisagli approved these changes

@jensens jensens jensens approved these changes

@tisto tisto tisto approved these changes

@lukasgraf lukasgraf Awaiting requested review from lukasgraf

Assignees

No one assigned

Labels

24 status: ready 32 needs: review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Image scales are not using cache optimized url form.

8 participants

@erral @coveralls @davisagli @tisto @jensens @datakurre @buchi