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.

Sign up for GitHub

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

Do not provide share link if (annotation or document) URL is not available on the web #2871

Closed
wants to merge 1 commit into from
Closed

Do not provide share link if (annotation or document) URL is not available on the web #2871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/sidebar/components/annotation-action-bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import propTypes from 'prop-types';

import uiConstants from '../ui-constants';
import { useStoreProxy } from '../store/use-store';
import { isShareable, shareURI } from '../util/annotation-sharing';
import { sharingEnabled, shareURI } from '../util/annotation-sharing';
import { isPrivate, permits } from '../util/permissions';
import { withServices } from '../util/service-context';

Expand Down Expand Up @@ -53,7 +53,7 @@ function AnnotationActionBar({
// Only authenticated users can flag an annotation, except the annotation's author.
const showFlagAction =
!!userProfile.userid && userProfile.userid !== annotation.user;
const showShareAction = isShareable(annotation, settings);
const showShareAction = sharingEnabled(settings) && shareURI(annotation);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The overall refactor of annotation-sharing logic made it more coherent to have this logic in two discrete functions instead of one. There is some complexity because "is sharing allowed" is not a single question, but, kind of, four:

  • Should we show the icon for sharing this page's annotations? The logic we use (unchanged) is "Are we first-party?" This is in AnnotationActionBar at present.
  • Should we show the icon for sharing a single annotation? (This line here). The logic we use (unchanged) is: is there anything in the app's config that says we shouldn't allow annotation sharing, and does this annotation have a valid single-annotation URL in its links?
  • Which UI should we be providing within the "share all annotations on this page" panel, once it's open? (New). If the page's URI is shareable (i.e. web-accessible), we render the main, pre-existing UI that provides a bouncer link. If not, we render the new UI path explaining why the page's annotations are not shareable (with no link).
  • Which UI should we be providing within the "share this annotation" panel, once it's open? (New). If the annotation's URI is shareable (i.e. web-accessible), we render the main, pre-existing UI that provides a bouncer link. If not, we render the new UI path explaining why this annotation is not shareable (with no link).


const onDelete = () => {
if (window.confirm('Are you sure you want to delete this annotation?')) {
Expand Down
67 changes: 41 additions & 26 deletions src/sidebar/components/annotation-share-control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { createElement } from 'preact';
import classnames from 'classnames';
import { useEffect, useRef, useState } from 'preact/hooks';
import propTypes from 'prop-types';

import { isShareableURI } from '../util/annotation-sharing';
import { copyText } from '../util/copy-to-clipboard';
import { isPrivate } from '../util/permissions';
import { withServices } from '../util/service-context';
Expand Down Expand Up @@ -48,6 +50,7 @@ function AnnotationShareControl({
shareUri,
}) {
const annotationIsPrivate = isPrivate(annotation.permissions);
const allowSharing = isShareableURI(annotation.uri);
const shareRef = useRef(/** @type {HTMLDivElement|null} */ (null));
const inputRef = useRef(/** @type {HTMLInputElement|null} */ (null));

Expand All @@ -66,8 +69,8 @@ function AnnotationShareControl({

if (isOpen && !selectionOverflowsInputElement()) {
// Panel was just opened: select and focus the share URI for convenience
inputRef.current.focus();
inputRef.current.select();
inputRef.current?.focus();
inputRef.current?.select();
}
}
}, [isOpen]);
Expand Down Expand Up @@ -118,38 +121,50 @@ function AnnotationShareControl({
isExpanded={isOpen}
/>
{isOpen && (
<div className="annotation-share-panel">
<div
className={classnames('annotation-share-panel', {
'annotation-share-panel--shareable': allowSharing,
})}
>
<div className="annotation-share-panel__header">
<div className="annotation-share-panel__title">
Share this annotation
</div>
</div>
<div className="annotation-share-panel__content">
<div className="u-layout-row">
<input
aria-label="Use this URL to share this annotation"
className="annotation-share-panel__form-input"
type="text"
value={shareUri}
readOnly
ref={inputRef}
/>
<Button
icon="copy"
title="Copy share link to clipboard"
onClick={copyShareLink}
className="annotation-share-panel__icon-button"
{allowSharing ? (
<div className="annotation-share-panel__content">
<div className="u-layout-row">
<input
aria-label="Use this URL to share this annotation"
className="annotation-share-panel__form-input"
type="text"
value={shareUri}
readOnly
ref={inputRef}
/>
<Button
icon="copy"
title="Copy share link to clipboard"
onClick={copyShareLink}
className="annotation-share-panel__icon-button"
/>
</div>
<div className="annotation-share-panel__details">
{annotationSharingInfo}
</div>
<ShareLinks
shareURI={shareUri}
analyticsEventName={analytics.events.ANNOTATION_SHARED}
/>
</div>
<div className="annotation-share-panel__permissions">
{annotationSharingInfo}
) : (
<div className="annotation-share-panel__content">
<div className="annotation-share-panel__details">
This annotation cannot be shared because it was made on a
document that is not available on the web.
</div>
</div>
<ShareLinks
shareURI={shareUri}
analyticsEventName={analytics.events.ANNOTATION_SHARED}
className="annotation-share-control__links"
/>
</div>
)}
<SvgIcon
name="pointer"
inline={true}
Expand Down
41 changes: 25 additions & 16 deletions src/sidebar/components/share-annotations-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import propTypes from 'prop-types';

import { useStoreProxy } from '../store/use-store';
import uiConstants from '../ui-constants';
import { getSharingLink, isShareableURI } from '../util/annotation-sharing';
import { copyText } from '../util/copy-to-clipboard';
import { withServices } from '../util/service-context';

Expand All @@ -18,7 +19,7 @@ import SvgIcon from '../../shared/components/svg-icon';
*/

/**
* A panel for sharing the current group's annotations.
* A panel for sharing the current group's annotations on the current document.
*
* Links within this component allow a user to share the set of annotations that
* are on the current page (as defined by the main frame's URI) and contained
Expand All @@ -30,24 +31,24 @@ function ShareAnnotationsPanel({ analytics, toastMessenger }) {
const store = useStoreProxy();
const mainFrame = store.mainFrame();
const focusedGroup = store.focusedGroup();

const groupName = (focusedGroup && focusedGroup.name) || '...';
const panelTitle = `Share Annotations in ${groupName}`;

// Generate bouncer sharing link for annotations in the current group.
// Not all URIs are valid to share: the document needs to be available
// on the web.
const uriShareable = mainFrame?.uri && isShareableURI(mainFrame?.uri);
lyzadanger marked this conversation as resolved.
Show resolved Hide resolved

// Generate bouncer sharing link.
// This is the URI format for the web-sharing link shown in the input
// and is available to be copied to clipboard
const shareURI = ((frame, group) => {
return group && frame
? `https://hyp.is/go?url=${encodeURIComponent(frame.uri)}&group=${
group.id
}`
: '';
})(mainFrame, focusedGroup);
// and is available to be copied to clipboard. We need the frame's URI
// (document URL) and the focused group to be available before this link
// can be generated.
const shareURI =
focusedGroup && mainFrame && getSharingLink(mainFrame.uri, focusedGroup.id);

const copyShareLink = () => {
try {
copyText(shareURI);
copyText(shareURI ?? '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appeasing the TS gods

Copy link
Member

Choose a reason for hiding this comment

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

Even though it is a bit verbose I would prefer to use a cast here (/** @type {string} */ (shareURI)). This avoids adding an untested path to the code and also makes it more obvious to future readers that this is purely to keep TS happy.

As an aside, this is one area where TypeScript's native syntax is much more succinct (just add an exclamation mark after the expression - shareURI!).

toastMessenger.success('Copied share link to clipboard');
} catch (err) {
toastMessenger.error('Unable to copy link');
Expand All @@ -59,10 +60,10 @@ function ShareAnnotationsPanel({ analytics, toastMessenger }) {
title={panelTitle}
panelName={uiConstants.PANEL_SHARE_ANNOTATIONS}
>
{focusedGroup && mainFrame && (
{shareURI && uriShareable && (
<div className="share-annotations-panel">
<div className="share-annotations-panel__intro">
{focusedGroup.type === 'private' ? (
{focusedGroup?.type === 'private' ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS again...

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to resolve this by changing the conditional to shareURI && uriShareable && focusedGroup even though the last clause is redundant. It generates less code and avoids confusion for the reader about whether focusedGroup can in fact be null in these various cases.

I found an existing issue in TypeScript about this: microsoft/TypeScript#24865.

<p>
Use this link to share these annotations with other group
members:
Expand All @@ -87,15 +88,15 @@ function ShareAnnotationsPanel({ analytics, toastMessenger }) {
/>
</div>
<p>
{focusedGroup.type === 'private' ? (
{focusedGroup?.type === 'private' ? (
<span>
Annotations in the private group <em>{focusedGroup.name}</em>{' '}
are only visible to group members.
</span>
) : (
<span>
Anyone using this link may view the annotations in the group{' '}
<em>{focusedGroup.name}</em>.
<em>{focusedGroup?.name}</em>.
</span>
)}{' '}
<span>
Expand All @@ -110,6 +111,14 @@ function ShareAnnotationsPanel({ analytics, toastMessenger }) {
/>
</div>
)}
{shareURI && !uriShareable && (
<div className="share-annotations-panel">
<p>
These annotations cannot be shared because this document is not
available on the web.
</p>
</div>
)}
</SidebarPanel>
);
}
Expand Down
21 changes: 15 additions & 6 deletions src/sidebar/components/test/annotation-action-bar-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ describe('AnnotationActionBar', () => {
let fakePermits;
let fakeSettings;
// Fake dependencies
let fakeIsShareable;
let fakeShareURI;
let fakeSharingEnabled;
let fakeStore;

function createComponent(props = {}) {
Expand Down Expand Up @@ -76,7 +77,8 @@ describe('AnnotationActionBar', () => {
fakePermits = sinon.stub().returns(true);
fakeSettings = {};

fakeIsShareable = sinon.stub().returns(true);
fakeSharingEnabled = sinon.stub().returns(true);
fakeShareURI = sinon.stub().returns('http://share.me');

fakeStore = {
createDraft: sinon.stub(),
Expand All @@ -89,8 +91,8 @@ describe('AnnotationActionBar', () => {
$imports.$mock(mockImportedComponents());
$imports.$mock({
'../util/annotation-sharing': {
isShareable: fakeIsShareable,
shareURI: sinon.stub().returns('http://share.me'),
sharingEnabled: fakeSharingEnabled,
shareURI: fakeShareURI,
},
'../util/permissions': { permits: fakePermits },
'../store/use-store': { useStoreProxy: () => fakeStore },
Expand Down Expand Up @@ -236,8 +238,15 @@ describe('AnnotationActionBar', () => {
assert.isTrue(wrapper.find('AnnotationShareControl').exists());
});

it('does not show share action button if annotation is not shareable', () => {
fakeIsShareable.returns(false);
it('does not show share action button if sharing is not enabled', () => {
fakeSharingEnabled.returns(false);
const wrapper = createComponent();

assert.isFalse(wrapper.find('AnnotationShareControl').exists());
});

it('does not show share action button if annotation lacks sharing URI', () => {
fakeShareURI.returns(undefined);
const wrapper = createComponent();

assert.isFalse(wrapper.find('AnnotationShareControl').exists());
Expand Down
26 changes: 23 additions & 3 deletions src/sidebar/components/test/annotation-share-control-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ describe('AnnotationShareControl', () => {
let fakeToastMessenger;
let fakeGroup;
let fakeIsPrivate;
let fakeIsShareableURI;
let fakeShareUri;
let fakeIsIOS;

Expand Down Expand Up @@ -62,6 +63,7 @@ describe('AnnotationShareControl', () => {
group: 'fakegroup',
permissions: {},
user: 'acct:bar@foo.com',
uri: 'http://www.example.com',
};

fakeAnalytics = {
Expand All @@ -81,11 +83,13 @@ describe('AnnotationShareControl', () => {
type: 'private',
};
fakeIsPrivate = sinon.stub().returns(false);
fakeIsShareableURI = sinon.stub().returns(true);
fakeShareUri = 'https://www.example.com';
fakeIsIOS = sinon.stub().returns(false);

$imports.$mock(mockImportedComponents());
$imports.$mock({
'../util/annotation-sharing': { isShareableURI: fakeIsShareableURI },
'../util/copy-to-clipboard': fakeCopyToClipboard,
'../util/permissions': { isPrivate: fakeIsPrivate },
'./hooks/use-element-should-close': sinon.stub(),
Expand Down Expand Up @@ -131,6 +135,15 @@ describe('AnnotationShareControl', () => {
assert.isTrue(inputEl.prop('readOnly'));
});

it('does not render the share URI in an input if URI is not shareable', () => {
fakeIsShareableURI.returns(false);
const wrapper = createComponent();
openElement(wrapper);

const inputEl = wrapper.find('input');
assert.isFalse(inputEl.exists());
});

describe('copying the share URI to the clipboard', () => {
it('copies the share link to the clipboard when the copy button is clicked', () => {
const wrapper = createComponent();
Expand Down Expand Up @@ -195,13 +208,20 @@ describe('AnnotationShareControl', () => {
const wrapper = createComponent({ isPrivate: testcase.isPrivate });
openElement(wrapper);

const permissionsEl = wrapper.find(
'.annotation-share-panel__permissions'
);
const permissionsEl = wrapper.find('.annotation-share-panel__details');
assert.equal(permissionsEl.text(), testcase.expected);
});
});

it('renders an explanation if annotation cannot be shared', () => {
fakeIsShareableURI.returns(false);
const wrapper = createComponent();
openElement(wrapper);

const detailsEl = wrapper.find('.annotation-share-panel__details');
assert.include(detailsEl.text(), 'This annotation cannot be shared');
});

it('focuses the share-URI input when opened on non-iOS', () => {
const wrapper = createComponent();
openElement(wrapper);
Expand Down
Loading