Remove Olark #21488

jsnajdr · 2018-01-12T14:13:12Z

In this PR, I'm removing Olark from Calypso in small, easy-to-review steps.

I start with the following observations:

the olark-events module doesn't ever emit any events. All listeners are noops.
many actions in olark-store/actions are noops.
most other actions in olark-store/actions are never dispatched and most olark-store properties keep their initial value forever.

Based on this, I can gradually remove large parts of code that are provably never called and I'm close to being able to remove the whole lib/olark, lib/olark-store, lib/olark-events and state/ui/olark libraries.

There's only one bit of code that still does something: the /help/olark/mine REST endpoint returns a isUserEligible flag that is used to decide if HappyChat should be displayed to the user.

The user is eligible if the following conditions hold:

they have admin access to at least one paid blog with live-support sticker
they are not graylisted and haven't been blocked by the operator
there are quotas in place to send certain percentage of business/premium/personal plan users to HappyChat. Currently the quotas are all set to 100% on the livechat_options admin page, but that might change.

The endpoint is queried on Calypso startup, and then when a purchase is removed. It's not queried when a purchase is added, which looks like a bug -- if I add a paid plan purchase to a user account that didn't have any, HappyChat won't be available until page reload.

Then Calypso has some further conditions before HappyChat is offered to the user -- the connection to the server must be active, there must be a HE available and the site that help is requested for must be on a paid plan.

@dllh is the author of the wpcom code for /help/olark/mine. I assume that endpoint still needs to be consulted for HappyChat eligibility?

Fixes #21153

If the isUserEligible issue is resolved, removing the rest of Olark from Calypso is going to be trivial.

matticbot · 2018-01-12T14:13:14Z

Test live: https://calypso.live/?branch=remove/olark

dllh · 2018-01-12T14:28:12Z

Yes, I think the mine endpoint probably needs to continue to be consulted, and there remain entanglements with some old Olark code if I'm not mistaken (at any rate, there remain naming inconsistencies). For the Calypso changes, I've cc'd @omarjackman, who has probably lived with most of that code more than anybody else and who thus is probably the best fit to raise any concerns re the removals.

jsnajdr · 2018-01-12T14:37:14Z

Yes, I think the mine endpoint probably needs to continue to be consulted, and there remain entanglements with some old Olark code if I'm not mistaken (at any rate, there remain naming inconsistencies).

The entanglements are on the backend, is that what you mean? I haven't found any on Calypso side.

The best course of action seems to be to move the mine endpoint querying to state/happychat. That's the only part of lib/olark we still need and the rest can be removed.

dllh · 2018-01-12T14:49:45Z

Right, the entanglements I'm thinking of are on the back end (if indeed they're still intact).

omarjackman · 2018-01-12T18:19:27Z

The best course of action seems to be to move the mine endpoint querying to state/happychat. That's the only part of lib/olark we still need and the rest can be removed.

@jsnajdr Thats how I see it. I don't think /happychat/mine exists but we'd either want to reuse it or create a new endpoint to return that isUserEligible field using the business logic we already have in place. That'd also include renaming the wp filters from olark_* to happychat_*

jsnajdr · 2018-01-12T23:49:13Z

The PR is ready for review! 🎉

I added a state.happychat.user.isEligible flag to Redux and set it by requesting from the /help/olark/mine endpoint.

All the other commits are just deleting stuff. I recommend to review commit-by-commit. I divided the task into small easy-to-review bites and there is a detailed description in each commit message.

Here are some stats on how the size of Javascript we ship to our users decreased:
http://iscalypsofastyet.com/branch?branch=remove/olark

It's been replaced with `HelpContactClosed` and is no longer used. Removing it removes one of remaining usages of `lib/olark`.

This patch removes the only usage of `OlarkChatbox` component from `HelpContact`. The `canShowChatbox` method always returns `false`, because: 1. `isChatEnded` initial value is `false` and no Olark event is ever dispatched to set it to `true` -- `onCommandFromOperator` is never called. 2. `olark.details.isConversing` is always undefined, because `olark.details` is initialized to empty object and the `OLARK_DETAILS` action is never dispatched. After removing the only remaining usage, we also remove the component itself.

As no Olark events are ever emitted, we can remove listeners and all code used only by them from the `HelpContact` component.

…lp-contact Many Olark actions are defined as noop function, we can remove the calls. The conditions `olark.isOperatorAvailable` and `olark.details.isConversing` are always false: their initial value in the Olark store is `false` and the actions to update them are never dispatched.

This variation is no longer ever used, so we can remove all code that depends on it.

The `QueryOlark` components dispatches a `requestOlark` action, which in turn dispatches `olarkTimeout` -- setting Olark status to `timeout` immediately without attempting any real request. The Olark info is no longer needed in `hasDataToDetermineVariation` and it would be always `true` anyway, because `olarkTimedOut` is set to `true` by `QueryOlark`. All the code mentioned above can be removed.

…e it in Redux Use the Olark REST endpoint to fetch `isUserEligible` property and store it in Redux in `state.happychat.user.isEligible`. Replace places where the eligibility is retrieved using the `lib/olark` library with the new Redux actions and selectors. The eligibility is requested on Calypso startup and updated after removing a purchase. It's used to determine the value of `isHappychatAvailable` and the `HelpContact` components waits until it's fetched (i.e., becomes not `null`) before deciding which support variation to use. This patch removes the last usage of `lib/olark` in Calypso.

lamosty

Went through the code and so far, it's looking good. I'm going to smoke test a bit.

Btw, I found one more occurrence of "olark" in codebase in client/boot/README. You can remove that as well. :)

Btw 2, nice commits. :)

lamosty · 2018-01-15T15:12:19Z

client/state/purchases/actions.js

-	};
+export const clearPurchases = () => dispatch => {
+	dispatch( { type: PURCHASES_REMOVE } );
+	dispatch( requestHappychatEligibility() );


Is the TODO message about side effects not relevant anymore?

Yes, I believe it's not relevant any more.

Before this patch, the Olark function was called in the action creator -- you didn't even need to dispatch the action to execute it (!). Very hacky. Now it's a classic thunk that dispatches two Redux actions. Nothing unusual any more. Converting the request to data layer would be even better, but I didn't have any energy left to do that :)

jsnajdr · 2018-01-15T15:31:56Z

Btw, I found one more occurrence of "olark" in codebase in client/boot/README.

During boot, the /help/olark/mine request is still being issued. That's the only piece of Olark that's left -- the eligibility info is used for Happychat, too.

lamosty · 2018-01-15T15:41:16Z

During boot, the /help/olark/mine request is still being issued. That's the only piece of Olark that's left -- the eligibility info is used for Happychat, too.

Yes, but it's not "loading various helpers (olark..)" which sounds like something bigger is being loaded.

Anyways, a very minor detail.

lamosty

Tested by opening a chat window. Working fine.

oandregal · 2018-01-15T18:04:12Z

client/state/action-types.js

@@ -219,6 +219,7 @@ export const HAPPINESS_ENGINEERS_FETCH_SUCCESS = 'HAPPINESS_ENGINEERS_FETCH_SUCC
 export const HAPPINESS_ENGINEERS_RECEIVE = 'HAPPINESS_ENGINEERS_RECEIVE';
 export const HAPPYCHAT_BLUR = 'HAPPYCHAT_BLUR';
 export const HAPPYCHAT_FOCUS = 'HAPPYCHAT_FOCUS';
+export const HAPPYCHAT_ELIGIBILITY_SET = 'HAPPYCHAT_ELIGIBILITY_SET';


For context: there is the happychat-client project which is going to be the source of truth for Happychat customer facing code. In a few weeks, Calypso will be modified to use this library and the only code remaining will be Calypso-specific.

In that context, I've been namespacing the action names. At the moment, Calypso code doesn't have any Happychat user actions, but it will, and I've been using HAPPYCHAT_USER_* for them. With that in mind, can this be renamed to HAPPYCHAT_USER_ELIGIBILITY?

oandregal · 2018-01-15T18:08:40Z

client/state/happychat/user/actions.js

+	isEligible,
+} );
+
+export const requestHappychatEligibility = () => dispatch => {


For context: there is the happychat-client project which is going to be the source of truth for Happychat customer facing code. In a few weeks, Calypso will be modified to use this library and the only code remaining will be Calypso-specific.

In that context, I've been decoupling Calypso-specific code into middleware-calypso, all the rest should be Calypso-agnostic. Can this be refactored for the Olark request to be fired on middleware-calypso?

The HAPPYCHAT_USER_ELIGIBILITY actions are still valuable because this is something we'll need in every host, so I'll take care of merging these actions in the consolidated state.user for happychat-client.

Follow-up: the requestChatEligibility could be either a Happychat or a host (Calypso, WordPress, etc) action. I'll probably lean towards that being a host action, but I haven't thought about it deeply yet, so use whatever works for you at the moment - I can refactor that later if needed.

oandregal · 2018-01-15T18:12:32Z

client/state/happychat/user/reducer.js

@@ -29,4 +29,12 @@ export const geoLocation = createReducer(
 	geoLocationSchema
 );

-export default combineReducers( { geoLocation } );
+export const isEligible = createReducer(


As mentioned above: this code is going to be pulled from Calypso in a few weeks to happychat-client. We aren't using the custom Calypso state utils, but the canonical Redux ones. At the moment, we don't have a schema or a mechanism to store the state in the browser localStorage either. Can this be refactored to use the canonical Redux utils instead and remove the schema utils as well?

oandregal · 2018-01-15T18:21:56Z

client/state/happychat/selectors/is-happychat-available.js

@@ -8,12 +8,17 @@ import { get } from 'lodash';
 * Internal dependencies
 */
 import isHappychatClientConnected from 'state/happychat/selectors/is-happychat-client-connected';
+import isHappychatUserEligible from 'state/happychat/selectors/is-happychat-user-eligible';

 /**
 * Returns true if Happychat client is connected and server is available to take new chats
 * @param {Object} state - global redux state
 * @return {Boolean} Whether new chats can be taken
 */
 export default function( state ) {


There are use cases where we'll need happychat availability and user eligibility separately, so we'll need a new selector is-user-eligible-for-happychat (and don't modify is-happychat-available).

Just saw the is-happychat-user-eligible selector. We'll need to revert the changes to is-happychat-available, though.

oandregal · 2018-01-15T18:25:59Z

client/me/help/help-contact/index.jsx

-			olark.isUserEligible &&
-			this.props.isSelectedHelpSiteOnPaidPlan
-		);
+		return this.props.isHappychatAvailable && this.props.isSelectedHelpSiteOnPaidPlan;


This will need the isHappychatUserEligible selector when the changes in isHappychatAvailable are reverted.

oandregal

Thanks for this!

Most my comments address the fact that in a couple of weeks customer facing Happychat code will be removed from Calypso in favor of the happychat-client library. For that reason, in the past months, I've been trying to isolate Calypso-specific code into middleware-calypso. If you don't have the time to address them, no worries, I will in future PRs.
The one thing we need to do before landing this PR is separating user eligibility from the is-happychat-available selector. There may be places where we want to check one but the other - for example: in a pre-sales context, a user may not have any plan yet but we may want to upsell them things in a chat session. That's from a Calypso point-of-view; thinking about the host-agnostic API for Happychat, it makes sense to have both separated as well.
I also saw a couple of comments about Olark that don't hold true anymore in client/boot/README and client/me/help/help-contact/index.jsx. It'd be nice if we could remove them as well.

…ndependent

jsnajdr · 2018-01-15T22:10:01Z

@nosolosw I removed the remaining Olark mentions from comments and readmes, and made the isHappychatAvailable and isHappychatUserEligible selectors completely independent.

There are a few places where a isHappychatAvailable condition should probably be replaced with isHappychatAvailable && isHappychatUserEligible. But I chose to do a refactoring that doesn't change the observable behavior and doesn't try to fix any bugs, real or perceived.

I didn't remove the Calypso-specific combineReducers call that includes support for serialization and schema validation. That's out-of-scope for this PR and should be done as part of the happychat-client migration.

Thanks for your review and please have another look whenever you can.

oandregal

Code looks good. Started a Happychat session, sent a Zendesk ticket, and loaded the Directly widget. Everything works properly. 🚢

jsnajdr added [Status] In Progress [Type] Janitorial [Type] Performance labels Jan 12, 2018

jsnajdr self-assigned this Jan 12, 2018

jsnajdr requested review from mattwondra, oandregal, dllh and lamosty January 12, 2018 14:13

jsnajdr mentioned this pull request Jan 12, 2018

Drop Olark from Calypso #21153

Closed

jsnajdr added the Happychat label Jan 12, 2018

dllh requested a review from omarjackman January 12, 2018 14:26

jsnajdr requested a review from samouri January 12, 2018 23:48

jsnajdr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jan 12, 2018

jsnajdr added 9 commits January 13, 2018 23:00

Remove the ChatClosureNotice component

1717931

It's been replaced with `HelpContactClosed` and is no longer used. Removing it removes one of remaining usages of `lib/olark`.

Remove Olark event listeners from me/help/help-contact

28c8ecd

As no Olark events are ever emitted, we can remove listeners and all code used only by them from the `HelpContact` component.

Remove SUPPORT_LIVECHAT variation from me/help/help-contact

5af894e

This variation is no longer ever used, so we can remove all code that depends on it.

Remove the lib/olark libraries

fa8e81e

Remove state/ui/olark Redux code

3e96b90

jsnajdr added 2 commits January 13, 2018 23:00

Remove olark/habla CSS styles

8378f5d

Remove olark config flags

b8a43c0

jsnajdr force-pushed the remove/olark branch from 811478d to b8a43c0 Compare January 13, 2018 22:01

lamosty reviewed Jan 15, 2018

View reviewed changes

lamosty approved these changes Jan 15, 2018

View reviewed changes

oandregal reviewed Jan 15, 2018

View reviewed changes

oandregal requested changes Jan 15, 2018

View reviewed changes

jsnajdr added 2 commits January 15, 2018 22:34

Remove remaining Olark mentions from documentation and comments

1c95be9

Make the isHappychatAvailable and isHappychatUserEligible selectors i…

6d950c0

…ndependent

oandregal approved these changes Jan 16, 2018

View reviewed changes

jsnajdr merged commit 0cb2876 into master Jan 16, 2018

jsnajdr deleted the remove/olark branch January 16, 2018 10:00

jeremeylduvall mentioned this pull request Jan 17, 2018

Support: Update presales and precancellation toggles to reflect API #21200

Merged

4 tasks

klimeryk mentioned this pull request Mar 2, 2018

Unnecessary API calls when loading a logged-out view #14745

Closed

sirreal removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Olark #21488

Remove Olark #21488

jsnajdr commented Jan 12, 2018

matticbot commented Jan 12, 2018

dllh commented Jan 12, 2018

jsnajdr commented Jan 12, 2018

dllh commented Jan 12, 2018

omarjackman commented Jan 12, 2018

jsnajdr commented Jan 12, 2018

lamosty left a comment •

edited

Loading

lamosty Jan 15, 2018

jsnajdr Jan 15, 2018

jsnajdr commented Jan 15, 2018

lamosty commented Jan 15, 2018

lamosty left a comment

oandregal Jan 15, 2018 •

edited

Loading

oandregal Jan 15, 2018

oandregal Jan 15, 2018

oandregal Jan 15, 2018 •

edited

Loading

oandregal Jan 15, 2018 •

edited

Loading

oandregal Jan 15, 2018

oandregal Jan 15, 2018 •

edited

Loading

oandregal left a comment •

edited

Loading

jsnajdr commented Jan 15, 2018

oandregal left a comment

Remove Olark #21488

Remove Olark #21488

Conversation

jsnajdr commented Jan 12, 2018

matticbot commented Jan 12, 2018

dllh commented Jan 12, 2018

jsnajdr commented Jan 12, 2018

dllh commented Jan 12, 2018

omarjackman commented Jan 12, 2018

jsnajdr commented Jan 12, 2018

lamosty left a comment • edited Loading

Choose a reason for hiding this comment

lamosty Jan 15, 2018

Choose a reason for hiding this comment

jsnajdr Jan 15, 2018

Choose a reason for hiding this comment

jsnajdr commented Jan 15, 2018

lamosty commented Jan 15, 2018

lamosty left a comment

Choose a reason for hiding this comment

oandregal Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

oandregal Jan 15, 2018

Choose a reason for hiding this comment

oandregal Jan 15, 2018

Choose a reason for hiding this comment

oandregal Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

oandregal Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

oandregal Jan 15, 2018

Choose a reason for hiding this comment

oandregal Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

oandregal left a comment • edited Loading

Choose a reason for hiding this comment

jsnajdr commented Jan 15, 2018

oandregal left a comment

Choose a reason for hiding this comment

lamosty left a comment •

edited

Loading

oandregal Jan 15, 2018 •

edited

Loading

oandregal Jan 15, 2018 •

edited

Loading

oandregal Jan 15, 2018 •

edited

Loading

oandregal Jan 15, 2018 •

edited

Loading

oandregal left a comment •

edited

Loading