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

Expose whether a version is soft or hard-blocked in the blocked page #13285

Merged
merged 6 commits into from
Oct 29, 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
110 changes: 57 additions & 53 deletions src/amo/pages/Block/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,52 +83,26 @@ export class BlockBase extends React.Component<InternalProps> {
);
}

renderDateAndURL(): React.Node | Array<React.Node | string> {
renderURL(): React.Node | Array<React.Node | string> {
const { block, i18n } = this.props;

if (!block) {
return <LoadingText />;
}

const content: Array<React.Node | string> = [
i18n.sprintf(i18n.gettext('Blocked on %(date)s.'), {
date: i18n.moment(block.created).format('ll'),
}),
];

if (block.url) {
content.push(
' ',
return (
<a key={block.url.url} href={block.url.outgoing} title={block.url.url}>
{i18n.gettext('View block request')}
</a>,
'.',
</a>
);
}

return content;
}

renderVersions(): React.Node | string {
const { block, i18n } = this.props;

if (!block) {
return <LoadingText />;
}

if (block.is_all_versions) {
return i18n.gettext('Versions blocked: all versions.');
}

const versions = block.versions.join(', ');

return i18n.sprintf(i18n.gettext('Versions blocked: %(versions)s.'), {
versions,
});
return null;
}

render(): React.Node {
const { block, errorHandler, i18n } = this.props;
const { block, errorHandler, i18n, match } = this.props;

if (errorHandler.hasError()) {
log.warn(`Captured API Error: ${errorHandler.capturedError.messages}`);
Expand All @@ -140,15 +114,40 @@ export class BlockBase extends React.Component<InternalProps> {
return <ServerErrorPage />;
}

const title =
block && block.name
? i18n.sprintf(
i18n.gettext(`%(addonName)s has been blocked for your protection.`),
{
addonName: block.name,
},
)
: i18n.gettext(`This add-on has been blocked for your protection.`);
const isSoftBlocked =
block?.soft_blocked.length &&
(block?.soft_blocked.includes(match.params.versionId) ||
!block?.blocked.length);
Comment on lines +117 to +120
Copy link
Member

Choose a reason for hiding this comment

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

This should be a selector (e.g. isSoftBlocked(guid, versionId) defined in the reducer file) or at least computed in mapStateToProps IMO.

let title;
Copy link
Member

Choose a reason for hiding this comment

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

Should we expose versionId somewhere in the title or copy? (To me, it's expected that if we're rendering the content based on the versionId, we would mention that versionId somewhere on the page).

Copy link
Member Author

Choose a reason for hiding this comment

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

We specifically dropped the "Versions blocked" text partly because it could be confusing, I fear this could have similar issues: if you show the version number alone, it doesn't say anything about the other versions, and that could be misleading ? I've asked in the Add-ons UX slack channel though, and I'll file and implement a follow-up if we decide we want the version in the title/headline.

if (isSoftBlocked) {
title =
block && block.name
? i18n.sprintf(
i18n.gettext(
`%(addonName)s is restricted for violating Mozilla policies.`,
),
{
addonName: block.name,
},
)
: i18n.gettext(
`This add-on is restricted for violating Mozilla policies.`,
);
} else {
title =
block && block.name
? i18n.sprintf(
i18n.gettext(
`%(addonName)s is blocked for violating Mozilla policies.`,
),
{
addonName: block.name,
},
)
: i18n.gettext(
`This add-on is blocked for violating Mozilla policies.`,
Copy link
Member

Choose a reason for hiding this comment

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

The title doesn't appear to have a period at the end in the spec.

);
}

return (
<Page>
Expand All @@ -159,13 +158,13 @@ export class BlockBase extends React.Component<InternalProps> {
</Helmet>

<Card className="Block-content" header={title}>
<h2>{i18n.gettext('Why was it blocked?')}</h2>
<h2>{i18n.gettext('Why did this happen?')}</h2>
<p
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={sanitizeHTML(
i18n.sprintf(
i18n.gettext(`This add-on violates %(startLink)sMozilla's
Add-on Policies%(endLink)s.`),
i18n.gettext(`This extension, theme, or plugin violates
%(startLink)sMozilla's Add-on Policies%(endLink)s.`),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
%(startLink)sMozilla's Add-on Policies%(endLink)s.`),
%(startLink)sMozilla's add-on policies%(endLink)s.`),

{
startLink: `<a href="${POLICIES_URL}">`,
endLink: '</a>',
Expand All @@ -178,8 +177,16 @@ export class BlockBase extends React.Component<InternalProps> {

<h2>{i18n.gettext('What does this mean?')}</h2>
<p>
{i18n.gettext(`The problematic add-on or plugin will be
automatically disabled and no longer usable.`)}
{isSoftBlocked
? i18n.gettext(`Until the violation is resolved, this add-on
won't be available for download from addons.mozilla.org.
If it’s already installed, it will be disabled and users
will be informed about the violation. They may choose to
enable the add-on again at their own risk.`)
: i18n.gettext(`Until the violation is resolved, this add-on
won't be available for download from addons.mozilla.org.
It will be automatically disabled and no longer usable in
Firefox.`)}
</p>
<p
// eslint-disable-next-line react/no-danger
Expand All @@ -189,9 +196,10 @@ export class BlockBase extends React.Component<InternalProps> {
or other third-party software that seriously compromises
Firefox security, stability, or performance and meets
%(criteriaStartLink)scertain criteria%(criteriaEndLink)s,
the software may be blocked from general use. For more
information, please read %(supportStartLink)sthis support
article%(supportEndLink)s.`),
the software may be blocked or restricted from general
use. For more information, please read
%(supportStartLink)sthis support article%(supportEndLink)s.
`),
{
criteriaStartLink: `<a href="${CRITERIA_URL}">`,
criteriaEndLink: '</a>',
Expand All @@ -202,11 +210,7 @@ export class BlockBase extends React.Component<InternalProps> {
['a'],
)}
/>
<p className="Block-metadata">
{this.renderVersions()}
<br />
{this.renderDateAndURL()}
</p>
<p className="Block-metadata">{this.renderURL()}</p>
</Card>
</div>
</Page>
Expand Down
3 changes: 2 additions & 1 deletion src/amo/reducers/blocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ export type ExternalBlockType = {|
created: string,
guid: string,
id: number,
versions: [string],
soft_blocked: [string],
blocked: [string],
is_all_versions?: boolean,
modified: string,
reason: string | null,
Expand Down
141 changes: 71 additions & 70 deletions tests/unit/amo/pages/TestBlock.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
createFakeBlockResult,
createLocalizedString,
dispatchClientMetadata,
fakeI18n,
getElement,
renderPage as defaultRender,
screen,
Expand Down Expand Up @@ -129,16 +128,14 @@ describe(__filename, () => {
expect(
within(screen.getByClassName('Block-reason')).getAllByRole('alert'),
).toHaveLength(1);
// 1. versions blocked
// 2. date and URL
expect(
within(screen.getByClassName('Block-metadata')).getAllByRole('alert'),
).toHaveLength(2);
).toHaveLength(1);
});

it('renders a generic header/title when the block has no add-on name', async () => {
const block = _createFakeBlockResult({ addonName: null });
const title = 'This add-on has been blocked for your protection.';
const title = 'This add-on is blocked for violating Mozilla policies.';
store.dispatch(loadBlock({ block }));
render();

Expand All @@ -155,7 +152,7 @@ describe(__filename, () => {
const block = _createFakeBlockResult({
addonName: createLocalizedString(name),
});
const title = `${name} has been blocked for your protection.`;
const title = `${name} is blocked for violating Mozilla policies.`;
store.dispatch(loadBlock({ block }));
render();

Expand All @@ -168,85 +165,40 @@ describe(__filename, () => {
expect(screen.getByText(title)).toBeInTheDocument();
});

it('renders a paragraph with the reason when the block has one', () => {
const reason = 'this is a reason for a block';
const block = _createFakeBlockResult({ reason });
store.dispatch(loadBlock({ block }));
render();

expect(screen.getByText(reason)).toBeInTheDocument();
expect(screen.getByText(reason)).toHaveAttribute('lang', 'en-US');
});

it('does not render a reason if the block does not have one', () => {
const block = _createFakeBlockResult({ reason: null });
store.dispatch(loadBlock({ block }));
render();

expect(screen.queryByClassName('Block-reason')).not.toBeInTheDocument();
});

it('renders "all versions" when "is_all_versions" is true', () => {
it('renders a generic soft-block header/title when the block has no add-on name', async () => {
const block = _createFakeBlockResult({
is_all_versions: true,
addonName: null,
soft_blocked: ['42.0'],
blocked: [],
});
const i18n = fakeI18n();
const title = 'This add-on is restricted for violating Mozilla policies.';
store.dispatch(loadBlock({ block }));
render();

// The version info and the block date are inside the same tag, separated
// by a </br>.
expect(
screen.getByTextAcrossTags(
`Versions blocked: all versions.Blocked on ${i18n
.moment(block.created)
.format('ll')}.`,
await waitFor(() =>
expect(getElement('title')).toHaveTextContent(
`${title} – Add-ons for Firefox (${lang})`,
),
).toBeInTheDocument();
);
expect(screen.getByText(title)).toBeInTheDocument();
});

it('renders the versions if "is_all_versions" is false', () => {
const v1 = '12';
const v2 = '34';
const block = _createFakeBlockResult({
versions: [v1, v2],
is_all_versions: false,
});
const i18n = fakeI18n();
it('renders a paragraph with the reason when the block has one', () => {
const reason = 'this is a reason for a block';
const block = _createFakeBlockResult({ reason });
store.dispatch(loadBlock({ block }));
render();

// The version info and the block date are inside the same tag, separated
// by a </br>.
expect(
screen.getByTextAcrossTags(
`Versions blocked: ${v1}, ${v2}.Blocked on ${i18n
.moment(block.created)
.format('ll')}.`,
),
).toBeInTheDocument();
expect(screen.getByText(reason)).toBeInTheDocument();
expect(screen.getByText(reason)).toHaveAttribute('lang', 'en-US');
});

it('renders the versions if "is_all_versions" is missing', () => {
const v1 = '12';
const v2 = '34';
const block = _createFakeBlockResult({
versions: [v1, v2],
is_all_versions: undefined,
});
const i18n = fakeI18n();
it('does not render a reason if the block does not have one', () => {
const block = _createFakeBlockResult({ reason: null });
store.dispatch(loadBlock({ block }));
render();

// The version info and the block date are inside the same tag, separated
// by a </br>.
expect(
screen.getByTextAcrossTags(
`Versions blocked: ${v1}, ${v2}.Blocked on ${i18n
.moment(block.created)
.format('ll')}.`,
),
).toBeInTheDocument();
expect(screen.queryByClassName('Block-reason')).not.toBeInTheDocument();
});

it('renders the reason with HTML tags removed', () => {
Expand Down Expand Up @@ -307,7 +259,56 @@ describe(__filename, () => {
});

expect(
screen.getByText(`${name} has been blocked for your protection.`),
screen.getByText(`${name} is blocked for violating Mozilla policies.`),
).toBeInTheDocument();
expect(
screen.getByText(
/It will be automatically disabled and no longer usable in Firefox./,
),
).toBeInTheDocument();
});

it('renders a soft-block page when the block has only soft-blocks', () => {
const name = 'some-addon-name';
const block = _createFakeBlockResult({
addonName: createLocalizedString(name),
soft_blocked: ['42.0'],
blocked: [],
});
store.dispatch(loadBlock({ block }));

render();

expect(
screen.getByText(`${name} is restricted for violating Mozilla policies.`),
).toBeInTheDocument();
expect(
screen.getByText(
/They may choose to enable the add-on again at their own risk./,
),
).toBeInTheDocument();
});

it('renders a soft-block page when a soft-blocked versionId is present in the URL', () => {
const name = 'some-addon-name';
const block = _createFakeBlockResult({
addonName: createLocalizedString(name),
soft_blocked: ['42.0'],
});
store.dispatch(loadBlock({ block }));

defaultRender({
initialEntries: [`${getLocation()}42.0/`],
store,
});

expect(
screen.getByText(`${name} is restricted for violating Mozilla policies.`),
).toBeInTheDocument();
expect(
screen.getByText(
/They may choose to enable the add-on again at their own risk./,
),
).toBeInTheDocument();
});

Expand Down
3 changes: 2 additions & 1 deletion tests/unit/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -1312,7 +1312,8 @@ export const createFakeBlockResult = ({
created: '2020-01-22T10:09:01Z',
modified: '2020-01-22T10:09:01Z',
guid,
versions: ['0.1', '4.56'],
blocked: ['0.1', '4.56'],
soft_blocked: [],
is_all_versions: false,
addon_name: addonName,
reason,
Expand Down