Add commands for upper staff to add custom avatars #5515
Conversation
I was told basing off of other's code was fine if i credited as so, please excuse me if i'm wrong otherwise.
There was a problem hiding this comment.
Quick first pass. Given the code you're attempting to get included upstream is also MIT licensed and youve included attribution I feel its OK, but including @CreaturePhil in the discussion to make sure he's aware and can weigh in.
Ultimately, @Zarel is going to need to weigh in on whether the feature is desirable in the first place, as he's currently the only one uploading avatars to the best of my knowledge, and I believe that he'd probably want to have every avatar run by him anyway to be able to maintain quality standards and visual cohesiveness, so I don't know how much of an impact this would be on the custom avatar process because Zarel's attention would be required either way.
(Thank you opening a PR though! Worth starting a discussion over)
server/chat-plugins/customavatars.js
Outdated
| let type = response.headers['content-type'].split('/'); | ||
| if (type[0] !== 'image') return; | ||
|
|
||
| response.pipe(FS(AVATAR_PATH + name + extension).createWriteStream()); |
There was a problem hiding this comment.
all the string concatenations in this file can be template strings instead
| const https = require('https'); | ||
|
|
||
| function downloadImage(image_url, name, extension) { | ||
| let req = https.get(image_url, res => { |
There was a problem hiding this comment.
should probably be returning a promise
| }); | ||
| }); | ||
| req.on('error', e => { | ||
| console.error(e); |
There was a problem hiding this comment.
not sure this is the correct error handling mechanism
There was a problem hiding this comment.
Might be better to throw the error to crashlogger in hindsight.
I need to review the plugin as a whole when I have time, haven't done much other than offer advice on a few issues.
There was a problem hiding this comment.
Error handling and logging seems to be quite badly implemented.
|
resolving as i'm fixing, will commit once everything's done so it's less spammy. |
forEach is also WIP, but is taking longer.
|
|
||
| let name = toID(parts[0]); | ||
| let avatarUrl = parts[1]; | ||
| if (!/^https?:\/\//i.test(avatarUrl)) avatarUrl = 'http://' + avatarUrl; |
There was a problem hiding this comment.
URLs don't necessarily end in the extension of the actual file they point at
| */ | ||
| 'use strict'; | ||
|
|
||
| /** @type {typeof import('../../lib/fs').FS} */ |
There was a problem hiding this comment.
This is in the wrong place, it should be above the importing of 'FS'.
| if (err) console.log("Error loading custom avatars: " + err); | ||
| if (!files) files = []; | ||
| files | ||
| .filter(file => VALID_EXTENSIONS.includes(path.extname(file))) |
There was a problem hiding this comment.
Should probably blacklist __proto__ and constructor
|
This implementation uses Also, since it might not be obvious for those who find this PR: |
| let type = response.headers['content-type'].split('/'); | ||
| if (type[0] !== 'image') return; | ||
|
|
||
| response.pipe(fs(AVATAR_PATH + name + extension).createWriteStream()); |
There was a problem hiding this comment.
should probably be returning a promise
Resolve the promise on finish event of the fs write stream.
This is purely for side servers, which are allowed to set their own custom avatars without oversight. The main server already uses a completely different mechanism for custom avatars (which is what Slayer95 means by "this implementation is invalid for the Smogon server"), mainly because we have access to more durable avatar storage (which lets our custom avatars be visible in replays). For side servers, yes, I'm completely OK with this, once the code quality issues get sorted out. |
|
This looks abandoned. Feel free to reopen if you're still planning to work on this, but otherwise I've moved it to #2444. |
I was told basing off of other's code was fine if i credited as so, please excuse me if i'm wrong otherwise.