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 edge case bug in parsers where avatarUrl is null #1137

Closed

Conversation

CollinBeczak
Copy link
Contributor

Resolves: maproulette/maproulette3#2205

Solution: Make users.avatar_url an option[string] instead of a set string, and then give it an empty string fallback if the column has no value.
I tested this by removing the image of my user and re-rendering the comments:
Screenshot 2024-07-19 at 6 43 26 PM

@CollinBeczak CollinBeczak marked this pull request as ready for review July 19, 2024 23:45
Copy link

sonarcloud bot commented Jul 19, 2024

Copy link
Contributor

@ljdelight ljdelight left a comment

Choose a reason for hiding this comment

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

Updating a parser works, but there are more in the code that hits the avatar_url. Why not create a migration to have the db require not-null avatar URL and set existing nulls to the current default (/assets/images/user_no_image.png)? That was the preferred in the bug writeup maproulette/maproulette3#2205

@CollinBeczak
Copy link
Contributor Author

I can do that, all of the parsers already have avatar_url as an option, so i just converted the remaining ones that i could find to options as well. I'll hold off on merging this one, and look into adding a migration.

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.

Users with null users.avatar_url Causes Error in GET /api/v2/challenge/1/comments Endpoint
2 participants