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

Support User: Add SupportUser component. #1202

Closed
wants to merge 14 commits into from
Closed

Conversation

jmdodd
Copy link
Member

@jmdodd jmdodd commented Dec 2, 2015

This PR adds a component for providing support across REST API queries.

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.

Edit (@jordwest): This PR is being divided into smaller PRs for easier review:

@jmdodd jmdodd added Framework [Status] In Progress [Feature Group] Support All things related to WordPress.com customer support. labels Dec 2, 2015
@jmdodd jmdodd self-assigned this Dec 2, 2015
supportToken = '';
},
req: {
del: function( ...args ) {
Copy link
Member

Choose a reason for hiding this comment

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

After a bit of digging, I came to learn that functions in JavaScript can only ever be bound a single time. Every bind/call/apply after that won't change this. I've setup a test demonstrating this behavior and also found it in MDN's documentation on bind, stating that the this "cannot be overridden".

I'm not sure of why this was chosen to be the case, but it jumps out to me here where we double-bind. If the behavior is incorrect with a different iteration of the code, we probably have something strange going on and should fix it otherwise we might discover some crazy bugs.

Probably we could rewrite this with a simple call:

req: {
    del: ( ...args ) => del( ...( extendRequest( 'del', request ) ) ),
}

Or with null as the this to make it clearer than using wpcom

req: {
    del: ( ...args ) => del.apply( null, extendRequest( 'del', args ) )
}

These should be identical and I'd be very interested to find out.

Copy link
Member Author

Choose a reason for hiding this comment

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

This now uses the spread operator on extendRequest, which clarifies that post et al. are bound once to a const and then called, without needing to apply them.

@jordwest
Copy link
Contributor

I've just added some changes to give the support user feedback in two cases:

  1. When their access token has expired (or is otherwise invalid)
  2. When an invalid password is entered.

When one of these errors occurs, they're displayed on the entry screen along with the error message. Currently the message is displayed as the class support-user__error, but I haven't added any styles.

@jmdodd jmdodd added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 6, 2016
@jordwest jordwest force-pushed the add/support-user branch 2 times, most recently from 93bde26 to c6178f3 Compare January 11, 2016 05:54
@jordwest jordwest removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 13, 2016
This change disables the 'Change user' button while the switching
is in progress. Part of this change includes waiting until the token
has been successfully received and the user information has been
updated before altering the UI (including Masterbar color) to
show that support user is active.
This change reloads the user settings when activating and
deactivating support user mode. This should prevent some
issues with sticky data on the user profile page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Support All things related to WordPress.com customer support. Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants