Add fake avatars by mrcasals · Pull Request #334 · decidim/decidim

mrcasals · 2016-12-12T14:47:12Z

🎩 What? Why?

This PR adds avatars to users, and sets a default avatar to:

📌 Related Issues

There's no issue for this, but a discussion for this arose in #250. I needed this too in #309.

📋 Subtasks

Add code
Add tests

📷 Screenshots (optional)

None

mention-bot · 2016-12-12T14:47:13Z

@mrcasals, thanks for your PR! By analyzing the history of the files in this pull request, we identified @oriolgual, @josepjaume and @beagleknight to be potential reviewers.

houndci-bot · 2016-12-12T14:47:21Z

decidim-core/lib/decidim/avatar_generator.rb

Freezing immutable objects is pointless.

houndci-bot · 2016-12-12T14:47:21Z

decidim-core/lib/decidim/avatar_generator.rb

Freezing immutable objects is pointless.

houndci-bot · 2016-12-12T14:47:21Z

decidim-core/lib/decidim/avatar_generator.rb

Redundant curly braces around a hash parameter.

houndci-bot · 2016-12-12T14:47:21Z

decidim-core/lib/decidim/avatar_generator.rb

Freeze mutable objects assigned to constants.
Use %w or %W for an array of words.

houndci-bot · 2016-12-12T14:47:21Z

decidim-core/lib/decidim/avatar_generator.rb

Extra empty line detected at class body beginning.

houndci-bot · 2016-12-12T14:47:22Z

decidim-core/config/initializers/dragonfly.rb

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-12-12T14:47:22Z

decidim-core/config/initializers/dragonfly.rb

Align the parameters of a method call if they span more than one line.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-12-12T14:47:22Z

decidim-core/config/initializers/dragonfly.rb

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-12-12T14:47:22Z

decidim-core/config/initializers/dragonfly.rb

Missing frozen string literal comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2016-12-12T14:50:29Z

decidim-core/lib/decidim/avatar_generator.rb

Closing array brace must be on the same line as the last array element when opening brace is on the same line as the first array element.

houndci-bot · 2016-12-12T14:50:29Z

decidim-core/lib/decidim/avatar_generator.rb

Align the elements of an array literal if they span more than one line.

houndci-bot · 2016-12-12T14:50:29Z

decidim-core/lib/decidim/avatar_generator.rb

Align the elements of an array literal if they span more than one line.

houndci-bot · 2016-12-12T14:50:30Z

decidim-core/lib/decidim/avatar_generator.rb

Align the elements of an array literal if they span more than one line.

houndci-bot · 2016-12-12T14:50:30Z

decidim-core/lib/decidim/avatar_generator.rb

Align the elements of an array literal if they span more than one line.

houndci-bot · 2016-12-12T14:50:30Z

decidim-core/lib/decidim/avatar_generator.rb

Within %w/%W, quotes and ',' are unnecessary and may be unwanted in the resulting strings.

houndci-bot · 2016-12-12T14:51:14Z

decidim-core/config/initializers/dragonfly.rb

Align the parameters of a method call if they span more than one line.

beagleknight

Wow, amazing job dude!

codecov-io · 2016-12-12T15:13:49Z

Current coverage is 60.81% (diff: 72.22%)

Merging #334 into master will decrease coverage by 0.19%

@@             master       #334   diff @@
==========================================
  Files          3053       3004    -49   
  Lines        123661     122998   -663   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          75435      74796   -639   
+ Misses        48226      48202    -24   
  Partials          0          0

Powered by Codecov. Last update 5aba6f3...b17ce1c

josepjaume · 2016-12-12T15:29:25Z

decidim-core/config/initializers/dragonfly.rb

WUUUT nooooo monkeypatchingggg. Why can't you explicitly add this on the models that need this?

That was added by the Dragonfly generator, man. Anyway, I'm dropping all of this

josepjaume · 2016-12-12T15:29:45Z

decidim-core/config/initializers/dragonfly.rb

Add this in the engine instead of the initializer.

josepjaume · 2016-12-12T15:33:04Z

decidim-core/config/initializers/dragonfly.rb

Please use a cache storage instead of filesystem, as filesystem is volatile in some platforms like heroku.

oriolgual · 2016-12-12T15:38:45Z

decidim-core/lib/decidim/avatar_generator.rb

This seems a potential security risk, I'd rather create the avatar in a callback at the model

Also, I think there are too many public params, and it could also be a way to guess users by ID.

josepjaume · 2016-12-12T15:41:23Z

@oriolgual there's other options like https://github.com/lucek/avatarly

oriolgual · 2016-12-12T15:42:43Z

And https://github.com/loopj/initials-avatar

mrcasals · 2016-12-12T19:56:47Z

@josepjaume @oriolgual avatarly looks good enough, I can add it on user creation so that we have an avatar set. Do you prefer this over opening an endpoint?

mrcasals · 2016-12-13T08:04:05Z

@josepjaume @oriolgual finally going to set a before_create hook on the User model to create the avatar.

josepjaume · 2016-12-13T08:08:21Z

@mrcasals not sure a before_create hook is the way to go, as avatars will get created for each and every user even during tests. What about explicitly creating it on the user creation command, and re-generating it on the user edit command (which we don't have yet).

@oriolgual what do you think about this?

josepjaume · 2016-12-13T08:09:28Z

I mean, one of the reasons the command pattern exists is to be able to make this kind of flows more explicit - callbacks tend to be problematic.

oriolgual · 2016-12-13T08:09:43Z

Works for me @josepjaume

mrcasals · 2016-12-13T08:10:40Z

@oriolgual @josepjaume we don't have a command for user registration, because this is done via Devise automatically.

mrcasals · 2016-12-13T09:10:47Z

Update: in order to generate avatars, it looks like we need rmagick, and having both rmagick and minimagick does not work well. My proposal is using Gravatar, since it gives us some benefits:

If the user already has an avatar set up, it shows this one
If the user has no avatar, Gravatar returns a random one (or one specified by us)
We don't need to save the avatars in our file system
We can redirect the users to Gravatar if they want to change their avatar, so we don't need to handle anything about this.
We can get avatars through a secure connection

Cons:

We don't handle the avatars. I don't know if this can be a problem for any organization

@oriolgual already gave his OK offline. @josepjaume thoughts, please?

oriolgual · 2016-12-13T09:11:37Z

Another benefit: we spend 10x less time on this feature xD

josepjaume · 2016-12-13T09:14:05Z

One of the goals of the project is to have 0 dependencies on external providers, so I'm -1 on this. Let's just put a default avatar placeholder and see if we can improve this later on.

mrcasals · 2016-12-13T09:22:59Z

I'll set the default avatar to this for every user:

mrcasals · 2016-12-13T09:24:06Z

@josepjaume @oriolgual should the system admins also have an avatar, or we can skip them?

josepjaume · 2016-12-13T10:43:31Z

@mrcasals late to the party, but let's skip avatars for system admins xD

houndci-bot · 2016-12-13T11:07:39Z

decidim-core/app/uploaders/decidim/avatar_uploader.rb

Unused method argument - args. If it's necessary, use _ or _args as an argument name to indicate that it won't be used. You can also write as default_url(*) if you want the method to accept any arguments but don't care about them.

houndci-bot · 2016-12-13T11:07:39Z

decidim-core/app/uploaders/decidim/avatar_uploader.rb

Unused method argument - args. If it's necessary, use _ or _args as an argument name to indicate that it won't be used. You can also write as default_url(*) if you want the method to accept any arguments but don't care about them.

josepjaume · 2016-12-13T11:38:30Z

You should talk with @beagleknight to standarize how avatars are shown in comments.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/lib/decidim/avatar_generator.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Freezing immutable objects is pointless.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/lib/decidim/avatar_generator.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Freezing immutable objects is pointless.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/lib/decidim/avatar_generator.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Redundant curly braces around a hash parameter.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/lib/decidim/avatar_generator.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Extra empty line detected at class body beginning.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

mrcasals requested review from beagleknight, josepjaume and oriolgual December 12, 2016 14:47

mrcasals force-pushed the add_fake_avatars branch 2 times, most recently from 0983ca7 to ed14883 Compare December 12, 2016 14:50

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/lib/decidim/avatar_generator.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Align the elements of an array literal if they span more than one line.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/lib/decidim/avatar_generator.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Align the elements of an array literal if they span more than one line.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/lib/decidim/avatar_generator.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Align the elements of an array literal if they span more than one line.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/lib/decidim/avatar_generator.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Align the elements of an array literal if they span more than one line.

houndci-bot reviewed Dec 12, 2016

View reviewed changes

mrcasals force-pushed the add_fake_avatars branch from ed14883 to e7b0ce1 Compare December 12, 2016 14:51

houndci-bot reviewed Dec 12, 2016

View reviewed changes

decidim-core/config/initializers/dragonfly.rb Outdated

Copy link
Copy Markdown

houndci-bot Dec 12, 2016

Choose a reason for hiding this comment

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

Align the parameters of a method call if they span more than one line.

mrcasals force-pushed the add_fake_avatars branch from e7b0ce1 to b31dd01 Compare December 12, 2016 14:53

beagleknight approved these changes Dec 12, 2016

View reviewed changes

mrcasals mentioned this pull request Dec 12, 2016

Use user avatars in comments #336

Closed

mrcasals force-pushed the add_fake_avatars branch from b31dd01 to 6882949 Compare December 12, 2016 15:13

josepjaume suggested changes Dec 12, 2016

View reviewed changes

josepjaume approved these changes Dec 12, 2016

View reviewed changes

oriolgual suggested changes Dec 12, 2016

View reviewed changes

mrcasals force-pushed the add_fake_avatars branch from 6882949 to d422e58 Compare December 13, 2016 11:07

houndci-bot reviewed Dec 13, 2016

View reviewed changes

Add default avatars for users

9e2dd91

mrcasals force-pushed the add_fake_avatars branch from d422e58 to 9e2dd91 Compare December 13, 2016 11:08

Cache .yarn-cache folder

221b867

josepjaume approved these changes Dec 13, 2016

View reviewed changes

oriolgual approved these changes Dec 13, 2016

View reviewed changes

Fix tests

b17ce1c

mrcasals merged commit e7e6c02 into master Dec 13, 2016

mrcasals deleted the add_fake_avatars branch December 13, 2016 12:46

beagleknight mentioned this pull request Dec 15, 2016

Use avatarUrl from api to display author's avatars #362

Merged

aitorlb pushed a commit to CodiTramuntana/decidim that referenced this pull request Aug 8, 2019

Fix translation issue decidim#334

fc2d3fa

ahukkanen mentioned this pull request Feb 6, 2023

Fix ImageMagick errors when trying to identify image dimensions #10343

Merged

Uh oh!

Conversation

mrcasals commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

mention-bot commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beagleknight left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 60.81% (diff: 72.22%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josepjaume commented Dec 12, 2016

Uh oh!

oriolgual commented Dec 12, 2016

Uh oh!

mrcasals commented Dec 12, 2016

Uh oh!

mrcasals commented Dec 13, 2016

Uh oh!

josepjaume commented Dec 13, 2016

Uh oh!

josepjaume commented Dec 13, 2016

Uh oh!

oriolgual commented Dec 13, 2016

Uh oh!

mrcasals commented Dec 13, 2016

Uh oh!

mrcasals commented Dec 13, 2016

Uh oh!

mrcasals commented Dec 12, 2016 •

edited

Loading

codecov-io commented Dec 12, 2016 •

edited

Loading

josepjaume commented Dec 13, 2016 •

edited

Loading