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

[WIP] API Key flow #4

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

[WIP] API Key flow #4

wants to merge 7 commits into from

Conversation

albertcui
Copy link
Member

No description provided.

@albertcui albertcui requested a review from howardchung March 18, 2018 18:25
.modal(style="display:none;")
.jumbotron
ol
li Getting an API key requires a linked-credit card.
Copy link
Member

Choose a reason for hiding this comment

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

no hyphen

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

views/api.jade Outdated
ol
li Calls over 25000/month will require an API key.
li Keyless API calls are limited to 1 call per second. Calls with API keys are limited to 3 calls per second.
li Getting an API key requires a linked-credit card. We'll automatically charge the card at the beginning of the month for any fees owed.
Copy link
Member

Choose a reason for hiding this comment

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

no hyphen

views/api.jade Outdated
h4 Details
ol
li Calls over 25000/month will require an API key.
li Keyless API calls are limited to 1 call per second. Calls with API keys are limited to 3 calls per second.
Copy link
Member

Choose a reason for hiding this comment

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

60/180 calls per minute

table.table
tr
th First 25000 Calls/month
th Any Calls >25000/month
Copy link
Member

Choose a reason for hiding this comment

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

should be 25000+ calls/month?

Copy link
Member Author

@albertcui albertcui Mar 31, 2018

Choose a reason for hiding this comment

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

Well, even if they use 26000 calls, they'd only be paying for 1000 calls, so I think this is accurate wording.

@@ -99,6 +102,9 @@ module.exports = function (db, redis) {
currency: 'usd',
source: token.id,
description: `Buying ${amount} cheese!`,
metadata: {
user: req.user ? req.user.account_id : "annon",
Copy link
Member

Choose a reason for hiding this comment

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

anon?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

'Did you enter the details correctly?';
} else {
return 'There was a problem processing your request. ' +
'If you keep getting errors, please contact us for support.';
Copy link
Member

Choose a reason for hiding this comment

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

provide an email?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.


const api_key = uuid();

const query = `INSERT INTO api_keys (account_id, api_key, customer_id)
Copy link
Member

Choose a reason for hiding this comment

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

parameterize the values instead of using string interpolation


db.raw(
`
SELECT
Copy link
Member

Choose a reason for hiding this comment

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

parameterize the values

@@ -0,0 +1,160 @@
const config = require('./config');
Copy link
Member

Choose a reason for hiding this comment

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

how is this script run?

Copy link
Member Author

Choose a reason for hiding this comment

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

Manually. We could probably set up a cron job, but I'd rather supervise it.

invoice.js Outdated
api_key
FROM api_key_usage
WHERE
timestamp <= '${invoiceMonth.endOf('month').format("YYYY-MM-DD")}'
Copy link
Member

Choose a reason for hiding this comment

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

parameterize

invoice.js Outdated

if (results.rows.length === 1 && results.rows[0].usage_count === e.usage_count) {

e.usage_count = 25001;
Copy link
Member

Choose a reason for hiding this comment

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

guessing this is just for testing

invoice.js Outdated
);
} else {
console.error(
"[FAILED] Usage did not match count ad end of month. api_key",
Copy link
Member

Choose a reason for hiding this comment

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

at?

@@ -71,29 +72,14 @@ app.use((req, res, cb) => {
});

app.use('/', donate(db, redis));
app.use('/api', api(db, redis));
Copy link
Member

Choose a reason for hiding this comment

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

should the api endpoints be implemented in core?

@@ -26,6 +26,9 @@ const defaults = {
SESSION_SECRET: 'secret to encrypt cookies with', // string to encrypt cookies
COOKIE_DOMAIN: '',
GOAL: 5, // The cheese goal
API_PRICE: 1,
API_UNIT: 1,
Copy link
Member

Choose a reason for hiding this comment

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

what is API_UNIT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Price = # calls / API_UNIT * API_PRICE

@@ -26,6 +26,9 @@ const defaults = {
SESSION_SECRET: 'secret to encrypt cookies with', // string to encrypt cookies
COOKIE_DOMAIN: '',
GOAL: 5, // The cheese goal
API_PRICE: 1,
Copy link
Member

Choose a reason for hiding this comment

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

what currency is this in?

Copy link
Member Author

Choose a reason for hiding this comment

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

USD, adding comment.

});
})
} else {
return res.render('api', {
Copy link
Member

Choose a reason for hiding this comment

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

this isn't really an API if you're rendering HTML?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's rendering the Jade file.

@howardchung
Copy link
Member

I feel there is a strong case to be made for porting this legacy code to React. It doesn't have to be the same app as webUI if you feel the domains are different, but I am a little concerned about adding new features to this code that we haven't maintained in years.

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.

2 participants