Custom User Profile Pictures

[ar7em]

Adds avatar url field at profile page, that replaces gravatar icon. New user validate method check_external_avatar checks if avatar extension does match one of permitted extension in kandan_settings.yml and does not exceed size limit.

[jrgifford]

restarting the travis build, i think I fixed the problems on our end.

[coveralls]

Coverage decreased (-0%) when pulling da19fdd on ar7em:kandan-176-external-avatar into 89ef4d9 on kandanapp:master.

[jrgifford]

@fusion94 can you check the CLAHub stuff?

[fusion94]

@jrgifford CLAHub is broken atm go ahead and merge

[jrgifford]

@ar7em this looks good, before I merge, can you add some tests for this behavior? If you need some help, you can send me an email (its in my GitHub profile). 🤘

[ar7em]

Will do this ASAP. I'm not really familiar with Ruby, so 'll have to look through some guides on testing.

[jrgifford]

OK. We use rspec - if you need help, let me know. 😃

[donthorp]

test'll be good. I'm not sure this will correctly handle invalid non zero length inputs. e.g. " ", "myimage.jpg", "//no.protocol.com/myimage.jpg" or other typos. Would be good to have those tested.

[ar7em]

I'm sorry for taking so long to response. I tried to provide some testing to new functionality. Could you please spare couple of minutes and review the code?

[coveralls]

Coverage remained the same when pulling 6641773 on ar7em:kandan-176-external-avatar into 89ef4d9 on kandanapp:master.

[coveralls]

Coverage remained the same when pulling 4d1802f on ar7em:kandan-176-external-avatar into 89ef4d9 on kandanapp:master.

[jrgifford]

I'll see if i can take a look at this today.

[guilhermesimoes]

I think you should squash a bunch of these commits.

And I see I'm late to the party but have you considered using the fastimage gem?

It's faster than the standard Net::HTTP request and will identify the image type for you (not simply checking the extension of the file). The downside is that you won't be able to determine the image size, but maybe you could check its width and height instead? I'm not sure if the maintainers of the project will agree.

Also, there shouldn't be any need to check for zero length inputs. Just make a request to the input provided (or use fastimage) and if an image is found then the input is valid.

[ar7em]

GuilhermeSimoes, thanx for your reply.
Could you please guide me in commit squashing best practises? I'm not really sure i should do with those commits of mine.
I'll consider using fastimage gem, seems like it's really something i should have used on that case. But i'm not sure if any additional dependecies are okay for such minority functionality. Is that appropriate practice for ruby projects?

[guilhermesimoes]

Ok, so here's the gist:

git rebase -i means "I want to rewrite history". But you have to tell git how far back in history you want to go. So

git rebase -i HEAD~6 means "I want to check the last 6 commits and possible change them".

A console text editor will then open. Depending on your OS, this will be either Vim or Nano. Just search for these text editors to understand the basic stuff.

You then have the chance to edit, delete, reorder and squash (join) commits. All you have to do, is change the first word before each commit.

For example:

• Changing "pick" to "edit" will allow you to edit a commit.
• Reordering the lines will reorder the commits

In this PR, in the commit "Removed obsolete schema part", you should change "pick" to "fixup". This will squash this commit into the previous commit and discard its commit message. This way the history of the project will look a lot cleaner.

Finally, you have to push your changes. But since you are rewriting history, you'll have to use the force option.

git push origin kandan-176-external-avatar --force

[coveralls]

Coverage remained the same when pulling a4acf62 on ar7em:kandan-176-external-avatar into 71b806d on kandanapp:master.

[coveralls]

Coverage remained the same when pulling dac2f26 on ar7em:kandan-176-external-avatar into 71b806d on kandanapp:master.

[ar7em]

First of all, thank you, GuilhermeSimoes, for you kind answer and definite guide. I've played around with git rebase and ended up with single commit. I'll surely pay more attention on how to structure my commits in future.

I'll also consider using fastimage gem, but that's something that i'd like to hear from project maintainers.

At last, speaking of avatar url length in the helper method: external avatar should be optional to use. That's why i removed the check for empty string from validation method and appropriate test in last commit (yet again i'm not sure if i should squash it too). If no url is specified in corresponding field, gravatar image is used.

[coveralls]

Coverage remained the same when pulling fffd470 on ar7em:kandan-176-external-avatar into 71b806d on kandanapp:master.

[guilhermesimoes]

You don't have to always squash everything. I just suggested squashing that specific commit because you were just deleting stuff in that commit that you had added in the previous commit. In that case, you should just squash it so the history of the project is not polluted.

In this last commit, I think it makes sense to leave it as it is, no need to squash it. I think it shows the rationale behind the change.

And indeed you are right about the avatar url length. I don't know what I was thinking 😄

Might I suggest just 2 more things? Check this:

if self.avatar_url.nil? or self.avatar_url.empty?
  return
end

Rails has a method called blank? that checks if a string is nil or empty. And you can actually return on the same line, so I'd change that to:

return if self.avatar_url.blank?

And in the helper method:

if user.avatar_url and user.avatar_url.length > 0
  user.avatar_url
else
  Kandan::Config.options[:avatar_url].gsub(/%{hash}/, user.gravatar_hash).
    gsub(/%{size}/, (options[:size] || 30).to_s).
    gsub(/%{fallback}/, options[:fallback] || Kandan::Config.options[:avatar_fallback] || 'mm')
end

I'd use the blank method and switch the if block with the else block. Like this:

if user.avatar_url.blank?
  Kandan::Config.options[:avatar_url].gsub(/%{hash}/, user.gravatar_hash).
    gsub(/%{size}/, (options[:size] || 30).to_s).
    gsub(/%{fallback}/, options[:fallback] || Kandan::Config.options[:avatar_fallback] || 'mm')
else
  user.avatar_url
end

If nobody says anything about fastimage in the meantime, I think this looks pretty good to go!

[ar7em]

blank? method is most useful, thank you once again for your guidance!

[coveralls]

Coverage remained the same when pulling 189d43b on ar7em:kandan-176-external-avatar into 71b806d on kandanapp:master.

[jrgifford]

So my question is, why are the tests failing here? Do they work locally?

[fusion94]

This works and i'm less worried about the tests due to the age of the PR