Log on invalid project thumbnail image URLs #51981
Conversation
bencodeorg
changed the title
| # /v3/files/<channel_id>/.metadata/thumbnail.png | ||
| # I observed thumbnail URLs of remixed projects having having the channel ID of the parent project, | ||
| # so we assert on the start/end of the URL | ||
| valid_thumbnail_url = thumbnail_url.start_with?('/v3/files/') && thumbnail_url.end_with?('.metadata/thumbnail.png') |
There was a problem hiding this comment.
somehow, My brains ask me to ask you to create a method for this
def is_valid_thumbnail_url(thumbnail_url)
thumbnail_url.start_with?('/v3/files/') && thumbnail_url.end_with?('.metadata/thumbnail.png')
end
The reason is that I have seen many times where the logic for something valid changes overtime, and having this logic wrapped is always nice and creates higher confidence when updating
There was a problem hiding this comment.
Yep that makes sense to me! Will update.
| if !valid_thumbnail_url && DCDO.get('log_thumbnail_url_validation', false) | ||
| # raise ValidationError | ||
| Honeybadger.notify( | ||
| error_class: 'Project::ValidationError', |
There was a problem hiding this comment.
Is this why you created a new error? so we can have it mapped on honeybadger
I am curious, because I think it is a good idea. I just wonder how it could be useful in honeybadger or it will be consumed compared to an already existing error.
This is me trying to learn here
There was a problem hiding this comment.
I don't have a ton of experience here so I wouldn't take too much from my process, but I guess I was thinking that there wasn't a good existing error type to represent the type of error I'm trying to track here. I saw the existing pattern used to subclass a Sinatra error (NotFound) and followed it. And yeah, being able to distinguish this error type easily (particularly while I'm logging, rather than erroring and blocking project saves) is helpful.
That said, I think the "right" place for this would be as validation in Rails as part of our Project model, but realized working on this that we unfortunately do not actually use Rails (ie, we don't instantiate a Project, check validation before saving, etc -- you probably know this :)). I checked in with a couple folks on we could migrate these routes to Rails and do validation there, and it sounds like that work is possible now, time permitting (which I think would be a lot cleaner than what I have here).
Log to Honeybadger if we observe unexpected URLs used in project thumbnails. This is a precursor to actually blocking saves of projects with unexpected thumbnail URLs. I'd like to enable this logging for a couple of days before actually blocking saves, such that we don't unexpectedly affect user ability to save projects. The code to actually block projects being saved if they use an invalid thumbnail URL is in this PR (it's just commented out). I wrote tests as well that are not in this PR that I'll add back in when we actually are blocking saves.
More details on the context around this change in Jira ticket.
Links
Testing story
Using an Applab project locally (and the experiment flag enabled), I've tested manually in the console that hitting the project update route (/v3/channels/<channel_id>) with a "good" thumbnail URL does nothing, and a "bad" URL tries to log to Honeybadger.
One of the only places I could find the POST
/v3/channels/(no channel ID) was in the share dialog at/s/artist/lessons/1/levels/10, when you click "Finish", then "Publish" in the share dialog. Saw the same behavior as above.I have unit tests ready to go for this as well, but I removed them from this PR for now for readability. They're in this commit if curious, but can be reviewed when the actual "block project saves" PR goes out.
Deployment strategy
I'll turn on the flag (
log_thumbnail_url_validation) once this is deployed, such that I can monitor (and turn off if it floods Honeybadger).Follow-up work
Actually start blocking saves (and enable tests) once we've validated existing projects via the logging introduced here.