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

License activation: Add helpful error messages & display help steps on error response. #39333

Merged
merged 6 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: changed

Add more helpful error messages and help steps on license key activation error.
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import { JetpackLogo, Spinner } from '@automattic/jetpack-components';
import { Button, TextControl, SelectControl } from '@wordpress/components';
import { createInterpolateElement } from '@wordpress/element';
import { sprintf, __ } from '@wordpress/i18n';
import { Icon, warning } from '@wordpress/icons';
import PropTypes from 'prop-types';
import React, { useCallback, useEffect, useMemo, useState } from 'react';

import ActivationScreenError from '../activation-screen-error';
import { LICENSE_ERRORS } from '../activation-screen-error/constants';
import './style.scss';

/**
Expand Down Expand Up @@ -145,15 +145,26 @@ const ActivationScreenControls = props => {
licenseError,
onLicenseChange,
} = props;
const hasLicenseError = licenseError !== null && licenseError !== undefined;

useEffect( () => {
jetpackAnalytics.tracks.recordEvent( 'jetpack_wpa_license_key_activation_view' );
}, [] );

const className = hasLicenseError
? 'jp-license-activation-screen-controls--license-field-with-error'
: 'jp-license-activation-screen-controls--license-field';
const errorTypeMatch = licenseError?.match( /\[[a-z_]+\]/ );
const errorType = errorTypeMatch && errorTypeMatch[ 0 ];

const { ACTIVE_ON_SAME_SITE } = LICENSE_ERRORS;
const isLicenseAlreadyAttached = ACTIVE_ON_SAME_SITE === errorType;
const className = useMemo( () => {
if ( ! licenseError ) {
return 'jp-license-activation-screen-controls--license-field';
}
if ( isLicenseAlreadyAttached ) {
return 'jp-license-activation-screen-controls--license-field-with-success';
}

return 'jp-license-activation-screen-controls--license-field-with-error';
}, [ licenseError, isLicenseAlreadyAttached ] );

const hasAvailableLicenseKey = availableLicenses && availableLicenses.length;

Expand Down Expand Up @@ -189,11 +200,8 @@ const ActivationScreenControls = props => {
value={ license }
/>
) }
{ hasLicenseError && (
<div className="jp-license-activation-screen-controls--license-field-error">
<Icon icon={ warning } />
<span>{ licenseError }</span>
</div>
{ licenseError && (
<ActivationScreenError licenseError={ licenseError } errorType={ errorType } />
) }
</div>
<div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@


.jp-license-activation-screen-controls--license-field,
.jp-license-activation-screen-controls--license-field-with-error {
.jp-license-activation-screen-controls--license-field-with-error,
.jp-license-activation-screen-controls--license-field-with-success {
max-width: 500px;
.components-input-control__label.components-input-control__label.components-input-control__label {
font-weight: 600;
Expand Down Expand Up @@ -67,21 +68,10 @@
}
}

.jp-license-activation-screen-controls--license-field-error {
display: flex;
flex-direction: row;
align-items: flex-start;
color: var(--jp-red);
max-width: 500px;

svg {
margin-right: 4px;
fill: var(--jp-red);
min-width: 24px;
}

span {
font-size: var(--font-body);
.jp-license-activation-screen-controls--license-field-with-success {
input.components-text-control__input,
select.components-select-control__input {
border: 1px solid var(--jp-green);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ describe( 'ActivationScreenControls', () => {
expect( node ).toBeInTheDocument();
expect(
// eslint-disable-next-line testing-library/no-node-access
node.closest( '.jp-license-activation-screen-controls--license-field-error' )
node.closest( '.activation-screen-error__message' )
).toBeInTheDocument();
} );
} );
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export const LICENSE_ERRORS = {
NOT_SAME_OWNER: '[not_same_owner]',
ACTIVE_ON_SAME_SITE: '[active_on_same_site]',
ATTACHED_LICENSE: '[attached_license]',
PRODUCT_INCOMPATIBILITY: '[product_incompatibility]',
REVOKED_LICENSE: '[revoked_license]',
INVALID_INPUT: '[invalid_input]',
MULTISITE_INCOMPATIBILITY: '[multisite_incompatibility]',
} as const;
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import jetpackAnalytics from '@automattic/jetpack-analytics';
import { Icon, warning, check } from '@wordpress/icons';
import React, { useEffect } from 'react';
import { LICENSE_ERRORS } from './constants';
import { useGetErrorContent } from './use-get-error-content';
import type { FC } from 'react';

import './style.scss';

type LicenseErrorKeysType = keyof typeof LICENSE_ERRORS;
type LicenseErrorValuesType = ( typeof LICENSE_ERRORS )[ LicenseErrorKeysType ];

interface Props {
licenseError: string;
errorType: LicenseErrorValuesType;
}

const ActivationScreenError: FC< Props > = ( { licenseError, errorType } ) => {
useEffect( () => {
if ( licenseError ) {
jetpackAnalytics.tracks.recordEvent( 'jetpack_wpa_license_activation_error_view', {
error: licenseError,
error_type: errorType,
} );
}
}, [ licenseError, errorType ] );

const { errorMessage, errorInfo } = useGetErrorContent( licenseError, errorType );

if ( ! licenseError ) {
return null;
}

const { ACTIVE_ON_SAME_SITE } = LICENSE_ERRORS;
const isLicenseAlreadyAttached = ACTIVE_ON_SAME_SITE === errorType;

const errorMessageClass = isLicenseAlreadyAttached
? 'activation-screen-error__message--success'
: 'activation-screen-error__message--error';

return (
<>
<div className={ `activation-screen-error__message ${ errorMessageClass }` }>
<Icon icon={ isLicenseAlreadyAttached ? check : warning } size={ 20 } />
<span>{ errorMessage }</span>
</div>
{ errorInfo && <div className="activation-screen-error__info">{ errorInfo }</div> }
</>
);
};

export default ActivationScreenError;
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
.activation-screen-error__message {
display: flex;
flex-direction: row;
align-items: flex-start;
max-width: 500px;
margin-top: calc(var(--spacing-base)*.5);

svg {
margin-left: -3px;
}

span {
font-size: 13px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we have a --font-body-extra-small which is 12px and --font-body-small which is 14px. My gut tells me we should use one of those just to avoid having too many different font sizes. I understand if design thinks differently though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my only hesitation here is that I believe Design used this specific font size along with the reduced letter spacing so that all errors remain within one line (on desktop anyway). If I change the font size up to 14px, a couple of the errors span over 2 lines (with orphan), and if I drop down the size to 12px, then the error seems a bit too small.
I think i'll stick with the Design specified font size in this instance.. Just because of the reason above, and also because (as mentioned in another comment), this change may not remain in the codebase too long due to the upcoming related project.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my experience updating font size just to avoid lines overlapping is not best practices, but like you said this will be changing soon so I think it's fine for now 😅

line-height: 20px;
font-weight: 500;
letter-spacing: -0.044em;
}
}

.activation-screen-error__message--error {
color: var(--jp-red);
svg {
fill: var(--jp-red);
}
}

.activation-screen-error__message--success {
color: var(--jp-green);
svg {
fill: var(--jp-green);
}
}

.activation-screen-error__info {
background-color: var(--jp-gray-0);
color: var(--jp-gray-80);
font-size: var(--font-body-small);
line-height: calc(var(--font-title-small) - 2px);
border: 1px solid var(--jp-green-0);
border-radius: var(--jp-border-radius);
padding: var(--jp-modal-padding-small);
margin: 32px 0 8px;

> p {
margin: 0 0 1em;
font-size: var(--font-body-small);
}
> p:last-child {
margin-bottom: 0;
}

ol > li::marker {
font-weight: bold;
}

a {
color: var(--jp-green-50);
}
a:hover,
a:active {
color: var(--jp-green-70);
}
}

.jp-license-activation-screen-controls {
.activation-screen-error__info {
> p {
margin: 0 0 1em;
font-size: var(--font-body-small);
}
> p:last-child {
margin-bottom: 0;
}
}
}
Loading
Loading