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: Add concierge offer to contact form #16144

Merged
merged 8 commits into from
Jul 14, 2017

Conversation

jeremeylduvall
Copy link
Contributor

@jeremeylduvall jeremeylduvall commented Jul 12, 2017

This adds a screenshare offer to the contact form for Business users as part of the All Hands Business Concierge experiment that will be running in Happiness from 0 UTC on 7/19 to 0 UTC on 7/21. You can read more about that experiment in p7DVsv-2II-p2. Please do not merge this until 0 UTC on 7/19.

Business users should see the screenshare offering. Non-Business users should see a chat closure notice indicating when chat will reopen. They'll still be able to submit a ticket. Text is as follows:

  • Biz: Chat with us over screenshare! Click here to get one-on-one help with a Happiness Engineer.
  • Non-Biz: Chat is temporarily closed. We're still available over email in the meantime. Chat will be back on Friday, July 21st!

A bit of background work on this in 251-gh-hg.

Testing

  1. Load up this branch.
  2. Visit http://calypso.localhost:3000/help/contact from a non-Business plan account. Note that you see the correct messaging. The message should not be clickable. Chat will likely still be available as it hasn't been turned off yet.
  3. Visit the same page through a user account with the Business plan. You should see the concierge offer. Clicking the concierge offer should record calypso_help_calendly_offer_click via Tracks (you can log Tracks events to your console using localStorage.setItem( 'debug', 'calypso:analytics*' );).

Screenshots

Biz
screen shot 2017-07-12 at 8 39 32 am

Non-Biz
screen shot 2017-07-12 at 8 39 24 am

cc @dllh

This adds a screenshare offer to the contact form for Business users.
For non-Business plan users, it adds a "Chat temporarily closed"
message. These updates are for the all hands business onboarding
experiment run in Happiness from 7/19-7/20. We'll reverse these changes
afterwards.
@jeremeylduvall jeremeylduvall added [Feature Group] Support All things related to WordPress.com customer support. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 12, 2017
@jeremeylduvall jeremeylduvall self-assigned this Jul 12, 2017
@matticbot matticbot added the [Size] M Medium sized issue label Jul 12, 2017
@matticbot
Copy link
Contributor

@dllh
Copy link
Member

dllh commented Jul 12, 2017

Since this is time constrained (meaning it needs to be active for a constrained time window), we should probably build that into the code rather than relying on somebody deploying to turn it on or off.

@dllh
Copy link
Member

dllh commented Jul 12, 2017

@southp can you add the timing to this based on my comment above, make any other changes you think a review would call for, and kick it back to somebody on our team for final review? I'd like to consider this a handoff to our team, and I'd like us to land it this week.

@dllh dllh assigned southp and unassigned southp and jeremeylduvall Jul 12, 2017
@matticbot matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels Jul 14, 2017
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Jul 14, 2017
@southp
Copy link
Contributor

southp commented Jul 14, 2017

@dllh @jeremeylduvall
I have updated the code, in general the following:

  1. Extract the logic as a selector, isBusinessPlanUser
  2. Extract a new component to encapsulate the rendering code, ChatBusinessConciergeNotice
  3. Make the from / to date parts of the above component so that it only shows within the range so we can merge it safely.

cc @omarjackman @unDemian for further testing and review :)

@southp southp self-assigned this Jul 14, 2017
};

render = () => {
const { translate } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just nitpicking around, not a blocker but I guess we could destructure all used props here instead of using this.props

Copy link
Contributor

@unDemian unDemian left a comment

Choose a reason for hiding this comment

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

Code looks good, thanks for adding the selector for business users. Tests are running successfully. Tracks are recorded.

I tested with Business and Premium users before and after the time interval the notifications are not visible.

In the selected timeframe correct notifications are displayed to all types of users.

export default connect(
( state ) => ( {
isBusinessPlanUser: isBusinessPlanUser( state ),
} ),
Copy link
Contributor

Choose a reason for hiding this comment

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

This trailing comma doesn't cause a JS error?

Copy link
Contributor

@unDemian unDemian Jul 14, 2017

Choose a reason for hiding this comment

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

Nice catch, no it's not throwing an error for me and the linter is not complaining about it because it is a comma between connect parameters. The second parameter will be undefined with or without the comma so we could just remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I always thought trailing commas only worked for arrays and I maybe import definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's standard for function calls, I mean connect( parameter, ) will work but looks weird. I'll remove it.

Copy link
Contributor

@omarjackman omarjackman left a comment

Choose a reason for hiding this comment

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

Found a typo but this looks great

import { PLAN_BUSINESS } from 'lib/plans/constants';

/**
* Returns an boolean flag indicating if the current user is a business plan user.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Returns an boolean flag"

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated.

@unDemian unDemian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 14, 2017
@omarjackman
Copy link
Contributor

LGTM 🚢

@unDemian unDemian merged commit d9a07e8 into master Jul 14, 2017
@alisterscott alisterscott deleted the try/calendly-notif-contact-form branch October 24, 2017 04:43
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. [Size] XL Probably needs to be broken down into multiple smaller issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants