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

Fix avatars not displaying via webhooks #511

Merged
merged 5 commits into from
Dec 24, 2019
Merged

Conversation

Miosame
Copy link
Contributor

@Miosame Miosame commented Sep 23, 2019

Avatars that were 2048x2048 would break webhooks displaying avatars, defaulting to 128x128 fixes that.

Avatars that were 2048x2048 would break webhooks displaying avatars, defaulting to 128x128 fixes that.
@Miosame
Copy link
Contributor Author

Miosame commented Sep 23, 2019

Fixed all of the relevant CI issues, the remaining ones are to be fixed by you, because they expect the old 2048x2048 size:

AssertionError: expected '/avatars/123/avatarURL.png?size=128' to equal '/avatars/123/avatarURL.png?size=2048'

@Throne3d
Copy link
Collaborator

It's very strange to claim the remaining CI issues are to be fixed by us – it's low effort to fix the tests to expect the right results, and the code change you're proposing does break old expected behavior, so the remaining issues are, relevantly, yours to fix.

Can you provide reproduction steps for the issue this fixes?

@Miosame
Copy link
Contributor Author

Miosame commented Dec 14, 2019

@Throne3d I'm not sure why the note about the tests set you off? at the time I wasn't simply aware that the tests are hosted on this repository too and noted that the tests would need to be modified for the new case, as I assumed those are hosted on travis internal configurations in some way.

I will re-test if this is still necessary as a fix and document further in the coming hours.

@Miosame
Copy link
Contributor Author

Miosame commented Dec 14, 2019

@Throne3d tested it just now and the issue still persists, here are the steps to reproduce it:

  1. Post in IRC with a normal avatar that does not exceed the size => the bot displays the avatar
  2. Change the avatar to a very large image, e.g.: https://w.wallhaven.cc/full/od/wallhaven-odjx85.jpg
  3. Post again in IRC => bot attempts to fetch, showing the default discord avatar and then goes to a transparent image like this:

image

I will push the test fixes shortly.

@coveralls
Copy link

coveralls commented Dec 14, 2019

Coverage Status

Coverage remained the same at 97.126% when pulling 983eada on Miosame:master into 03d6ac2 on reactiflux:master.

@Miosame
Copy link
Contributor Author

Miosame commented Dec 14, 2019

@Throne3d submitted what looked like failing tests purely based on avatar size and now the tests pass. The above coverage comment was before I merged upstream.

Edit: coveralls edited its post.

@Throne3d
Copy link
Collaborator

@Throne3d I'm not sure why the note about the tests set you off?

I didn't mean to come across as being set off – your comment about the tests just came off as very strange =P

Thank you for this code change! It's really strange that I couldn't find anything about this in Discord's documentation, but I'm happy to merge this as-is. I'm slightly concerned that 128px might be too small, but if anyone notices a visual change, I'm hoping to get an issue and we can look into bumping it up.

@Throne3d Throne3d merged commit 88fecf3 into reactiflux:master Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants