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
start FxA fixes for new templates #669
Conversation
Set But, I don't see a report email in my Inbox, so there may still be a bug to work out. |
a2d5797
to
335fd33
Compare
Example of full FxA profile JSON I was able to request and store:
To display the profile picture, we will probably have to add |
a0d002b
to
0bf8d10
Compare
exports.up = function(knex, Promise) { | ||
return knex.schema.table("subscribers", table => { | ||
table.string("fxa_refresh_token"); | ||
table.json("fxa_profile_json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use table.jsonb
here, as knex/knex#991 was fixed in knex/knex#1044
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to jsonb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, Travis CI really doesn't like jsonb
columns. Not the default postgres, nor specifically setting it to 9.4 or 9.6 seems to work. @jgmize - any ideas for jsonb
columns on Travis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this should work in 9.4, crowdAI/crowdai#68 (comment) needed to switch to Postgres 9.6 (and ubuntu trusty) to get jsonb working on travisci.
04b867f
to
9e0ce4c
Compare
9e0ce4c
to
86ad731
Compare
Got Travis CI fixed. @pdehaan - did you want to give this a once-over? I'm feeling pretty good about it now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was on PTO and missed this... LGTM.
db/DB.js
Outdated
const verifiedSubscriber = await this._verifySubscriber(emailHash); | ||
return verifiedSubscriber[0]; | ||
const verified = await this._verifySubscriber(emailHash); | ||
const verifiedSubscriber = verified[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this is the same as the old code behavior, but should we check that verified[0]
(or verifiedSubscriber
) is non-null before using it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll add a check, and I'll actually add a low-level try-catch
block around the subscription to basket since we may get errors beyond our control there, and we don't need/want to show those low-level errors to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular issues on the l10n bits
locales/en/app.ftl
Outdated
@@ -19,6 +19,7 @@ about-firefox-alerts = About Firefox Alerts | |||
give-feedback = Give Feedback | |||
terms-and-privacy = Terms and Privacy | |||
|
|||
error-could-not-add-email = Could not add email to database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
email -> email address?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
b53bafd
to
b6959e1
Compare
We still need to ...
profile
scope and not justprofile:email
(needs FxA team to approve)profile
JSON in our DBaccess_type=offline
in our query params to get a long-life refresh token