Video & Music files Check (PR #2347)

When uploading a video or music file while creating a topic, automatic embedding does not happen due to the original way of checking file extensions and concatenating the relevant HTML tags. Now we check for valid video and music file (that are compatible with HTML5 tags) and simply paste the full URL in lieu of a link to the file for download. This allows onebox to do its own magic on the links.

GitHub

You’ve signed the CLA, Doppp. Thank you! This pull request is ready for review.

I don’t think this works. upload.url is a relative url.

Silly me, I’ll append window.location.protocol instead.

I wonder if this test case could be more readable if we use the _.map along with _.each.

_(["mp4", "webm", "ogg"]).map(function (ext) {
  return "video." + ext
}).each(function (filename) {
  ok(utils.isAVideo(filename), filename + "is recognized as a video")
  ok(utils.isAVideo("http://foo.bar/path/to/" + filename), filename + "is recognized as a video")
});

The assertion message repetition feels itchy.

I personally try to avoid if/else/if/else because it tends to be hard to read. Its not awful right now, but I believe could be better. If we had method that return the type of media, we could use a switch statement and improve the legibility imo.

// Discourse.Utilities.getMediaType returns a string representing the type of file.

return switch (Discourse.Utilities.getMediaType(upload.original_filename)) {
  case 'image':
    '<img src="' + upload.url + '" width="' + upload.width + '" height="' + upload.height + '">';
    break;
  case 'video':
  case 'audio':
    Discourse.getURL(upload.url);
    break;
  case 'link':
    '<a class="attachment" href="' + upload.url + '">' + upload.original_filename + '</a> (' + I18n.toHumanSize(upload.filesize) + ')';
    break;
}

@ZogStriP, how does this look?

@ZogStriP is out for a bit, pretty sure this is not going to work if you have S3 setup though. Discourse.URL is only going to point locally.

You’re right. Is there an elegant way to get the location protocol then? I had a solution initially to return '\n\n' + window.location.protocol + upload.url; but it wasn’t approved.

@SamSaffron, ping.

@SamSaffron, let me know if this PR is rejected or accepted. I’ll close if it’s the former.

@ZogStriP can you review this, we need to make a call.

@ZogStriP any word on this?

Closing this as this requires more work (handle S3 properly & update/fix the onebox)