Skip to content
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.

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

Resize maximum dimensions for pictures coming from SocialApiService #2745

Merged
merged 1 commit into from
May 17, 2022

Conversation

tcitworld
Copy link
Member

Closes #2743

Make sure every picture fetched from SocialAPIService has a maximum size of 256 pixels.

Should we add an extra condition to reject on size being to high?

Would be nice to test ImageResizer with a few test pictures too.

@tcitworld tcitworld added the 3. to review Waiting for reviews label May 16, 2022
@tcitworld tcitworld added this to the v4.2.0 milestone May 16, 2022
@tcitworld tcitworld requested a review from juliusknorr May 16, 2022 17:48
@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #2745 (b16b3a1) into main (938bf0c) will increase coverage by 50.72%.
The diff coverage is 69.23%.

❗ Current head b16b3a1 differs from pull request most recent head b6dd2e3. Consider uploading reports for the commit b6dd2e3 to get more accurate results

@@              Coverage Diff              @@
##               main    #2745       +/-   ##
=============================================
+ Coverage     18.04%   68.76%   +50.72%     
- Complexity      247      251        +4     
=============================================
  Files            97       22       -75     
  Lines          3170      714     -2456     
  Branches        439        0      -439     
=============================================
- Hits            572      491       -81     
+ Misses         2405      223     -2182     
+ Partials        193        0      -193     
Impacted Files Coverage Δ
lib/Service/SocialApiService.php 89.61% <66.66%> (-0.94%) ⬇️
lib/Service/ImageResizer.php 71.42% <71.42%> (ø)
src/mixins/PropertyMixin.js
src/files-action.js
src/models/rfcProps.js
src/views/Processing/ImportView.vue
src/services/parseVcf.js
...c/components/AppNavigation/GroupNavigationItem.vue
src/utils/fileUtils.js
src/services/checks/duplicateTypes.js
... and 68 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 938bf0c...b6dd2e3. Read the comment docs.

@tcitworld tcitworld force-pushed the resize-pictures-from-social-api-service branch 2 times, most recently from 4ef3551 to 0a4d8d5 Compare May 16, 2022 18:01
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You get bonus points for declare(strict_types=1); ;-)

@tcitworld tcitworld force-pushed the resize-pictures-from-social-api-service branch from 0a4d8d5 to b6dd2e3 Compare May 16, 2022 18:10
Copy link
Member

@call-me-matt call-me-matt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea.

@call-me-matt
Copy link
Member

But I am not sure it closes #2743 completely -
I would like to make sure there are not images piling up instead of being replaced by newer versions.

@ChristophWurst ChristophWurst merged commit 9c74648 into main May 17, 2022
@ChristophWurst ChristophWurst deleted the resize-pictures-from-social-api-service branch May 17, 2022 06:41
@ChristophWurst
Copy link
Member

/backport to stable4.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contact has 161 MB because of (automated) social media avatar
4 participants