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

API changes to accommodate 3rd party clients#683

Merged
dannycoates merged 2 commits intomozilla:masterfrom
ehuggett:issue618
Jan 11, 2018
Merged

API changes to accommodate 3rd party clients#683
dannycoates merged 2 commits intomozilla:masterfrom
ehuggett:issue618

Conversation

@ehuggett
Copy link
Copy Markdown
Contributor

Resolves #618

Copy link
Copy Markdown
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.

Thanks @ehuggett. These look like good changes, I just wonder about the types

const meta = await storage.metadata(id);
res.set('WWW-Authenticate', `send-v1 ${meta.nonce}`);
res.send({
password: meta.pwd
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

meta.pwd is a string iirc, '0' or '1'. It might be better to make the public type a boolean (or really anything besides '0' and '1') 😆

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh dear that was a mess, string zero for false 😱 (I was ignorant about how redis stores values and assumed I would be retrieving an int, but I should have noticed the quotes in the output/json).

res.send({
metadata: meta.metadata,
dtotal: meta.dl,
dlimit: meta.dlimit,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as meta.pwd -- dtotal and dlimit should be numbers

@ehuggett
Copy link
Copy Markdown
Contributor Author

Perhaps I should remove the changes to the metadata until #661 is resolved?

@dannycoates dannycoates merged commit 50ba8be into mozilla:master Jan 11, 2018
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.

2 participants