Support User: Add Support User libraries #2368
Conversation
jordwest
added
Framework
[Status] In Progress
[Feature Group] Support
jordwest
force-pushed
the
add/support-user-libs
branch
from
84bd378 to
844938a
Compare
jordwest
added
[Status] Needs Review
|
I've tested this a bit and it seemed to work as expected. Elsewhere I've noted some js errors that began appearing in the console after I do the logout. I'm not 100% sure they're even related to this code but figured I'd bring 'em up just in case. I'm squinting at code in two different tabs and trying to sort out whether the feedback offered here has been dealt with. There's a followup comment suggesting that it has, but the code looks kind of the same to me. I may be just failing to properly read the diff github offers, so if I'm off the mark here, just ignore the comment. The code looks reasonable to me, but I think it'd be good to get another set of eyes on it before we merge. |
| /** | ||
| * Expose `User` | ||
| */ | ||
| module.exports = User; | ||
|
|
||
| require( './dev-support-user' ); |
There was a problem hiding this comment.
I'm not a fan of the require here. Is it possible to move this to a more appropriate boot section?
There was a problem hiding this comment.
Moved to /client/boot/index.js -> boot()
|
👍 Works as advertised in Chrome |
jordwest
force-pushed
the
add/support-user-libs
branch
from
a3a4656 to
ed3efab
Compare
|
Thanks for the feedback! I've made some changes to clarify the code a bit, but I'm still thinking about ways to improve it some more. While pondering, I realized this whole process of intercepting every single Calypso request and extending it would be much simpler and less breakage prone if @dllh: Strange, I know @jmdodd and I worked on that section and made some changes, but it seems to be showing the new copy on GitHub. There was a force push in there though so GitHub may have lost the original code we discussed. There's no double binding happening at the moment, as removing the bind call causes many errors. I've just done a refactor that tidies that section up and hopefully makes it a bit easier to follow. |
|
After spending most of the day digging deep into @dllh, @gwwar: It'd be much appreciated if you could have another look over it. The changes have all occurred in |
|
This looks like a nice improvement to me, but I'd like to get @gwwar's final stamp of approval before we merge. |
| }, | ||
| request: ( params, callback ) => { | ||
| if ( supportUser && supportToken ) { | ||
| return request( addSupportData( params ), callback ); |
There was a problem hiding this comment.
👍 Great improvement here. This is much more readable.
|
|
| return Object.assign( {}, params, { | ||
| query: qs.stringify( query ) | ||
| } ); | ||
| }; |
There was a problem hiding this comment.
Using qs.parse instead of a hacked-in version is smart; if the params change, we won't have to rewrite parts of addSupportData.
Squashed: Support User: Spread returned array into appropriate parameters Support User: Add development conditional
This change simplifies the injection of the credentials by overriding the wpcom request function at a lower level. By injecting the token directly into the query string directly before the XHR call is made, previous logic that determined the type and format of the request is no longer necessary.
jordwest
force-pushed
the
add/support-user-libs
branch
from
ee1bdd3 to
da75b57
Compare
Support User: Add Support User libraries
scruffian
removed
the
[Status] Needs Review
This PR adds libraries that will be used in the support user functionality (ref p195om-2GO-p2)
An access token is granted when a support user and password are provided; the token can be revoked during a restore operation, or will expire after a set period of time. Once granted, the support username and token are passed along all queries to the API.
This is a subset of the original PR #1202 (props to @jmdodd), separated out for easier progressive review.