Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

Added a UI for the uploader end, made stylistic changes, implemented deleting #28

Merged
merged 7 commits into from Jun 1, 2017

Conversation

abhinadduri
Copy link
Collaborator

No description provided.

Copy link
Contributor

@dannycoates dannycoates left a comment

Choose a reason for hiding this comment

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

This looks good, just a couple nits before we merge 👍


app.get('/download/:id', function(req, res) {
res.sendFile(path.join(__dirname + '/public/download.html'));
app.get("/download/:id", function(req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can either do #29 now or in a follow up PR.

app.js Outdated

client.hset(id, "filename", filename);
client.hset(id, "expiration", 0);
client.hset(id, "delete", uuid);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using hmset (m for multi) will set all these values in one command to redis

app.js Outdated
let id = req.params.id;
client.hset(id, "filename", filename, redis.print);
client.hset(id, "expiration", 0, redis.print);
let uuid = Math.floor(Math.random() * 10000000).toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use crypto.randomBytes(4).toString('hex') here

app.js Outdated
client.hset(id, "delete", uuid);

// delete the file off the server in 24 hours
setTimeout(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not ideal to have a bunch of timeout callbacks hanging around like this. We'll need to find another way to do this before launch. Maybe we can rely on expiring them in S3, otherwise a separate process from the server to do the cleanup would be ok.

return iv;
}

function returnBindedLI(a_element, name, link, li) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The ui code here is fine for now, but eventually we'll want to refactor both upload and download to return objects that emit their own events so we can decouple them from the ui.

@ericawright
Copy link
Contributor

ericawright commented Jun 1, 2017

nit: I noticed all the changes to double quotes -- I assume this is because of the proposed eslint rule
quotes: [error, single]
this actually means that in javascript we want to only use single quotes. But also, it's probably not worth going back and changing them all at the moment.

@dannycoates
Copy link
Contributor

@abhinadduri let's add eslint asap after this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants